minor: add unit tests for monotonicity.rs #14307
Conversation
|
It seems this my local environment was not printing stack trace for error cases and CI does so error matches did not work. I've pushed the fix I expect it to pass now |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks like an improvement to me -- thank you @buraksenn
| upper: f64, | ||
| input_sort: SortProperties, | ||
| expected: Option<SortProperties>, | ||
| expect_err: bool, |
There was a problem hiding this comment.
It would be nice to have the expected error be a string rather than a bool
Something like
| expect_err: bool, | |
| expect_err: Option<&'static str>, |
That way we can ensure the errors aren't changing unexpectedly
There was a problem hiding this comment.
It was like that before but due to some issues in CI and local difference, I took a shortcut there but probably should not have done that. Let me properly fix it. Thanks for the review
There was a problem hiding this comment.
Oh, I got it now. I needed to strip_backtrace() before comparing errors. I think I've fixed it now
buraksenn
force-pushed
the
add-unit-tests-to-monotonicity
branch
from
a94b02e to
23edd68
Compare
berkaysynnada
left a comment
berkaysynnada
left a comment
There was a problem hiding this comment.
Thanks @buraksenn, LGTM
| "Test '{}' failed: got {:?}, expected {:?}", | ||
| tcase.name, a, e | ||
| ), | ||
| (Err(e1), Err(e2)) => { |
| descending: false, | ||
| nulls_first: false, | ||
| }), | ||
| expected: exec_err!("Input range of LN contains out-of-domain values"), |
3 participants
Which issue does this PR close?
Closes #10595
Rationale for this change
These methods are pretty straight forward but they did not have any unit tests. Thus, this PR adds required tests.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
No