Add null checks in assert_almost_* methods - #856
Open
CodeSmith32 wants to merge 3 commits into
Open
Conversation
- pre-check inputs for nulls before running comparison: report nulls as failed tests vs crashing
Owner
|
Needs some tests. I got curious about a couple other conditions and came up with the following test that can be expanded later if needed. Please add the following to var datatype_data = [
[1, 2, null],
[1, null, 3],
[null, 2, 3],
]
func test_datatype_checks(params = use_parameters(datatype_data)):
gr.test.assert_almost_eq(params[0], params[1], params[2])
assert_engine_error_count(0) |
- write tests for assert_almost_eq / assert_almost_ne when inputs are different types or null
Author
|
Thanks @bitwes! I've added those tests in. I made it a little more thorough:
I can change/remove these if needed. Just let me know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This closes issue #855
Please feel free to edit this PR / reply here if I'm missing anything.
e.g., I'd assume that nulls in the inputs should just be reported as failed tests vs. crashing, but if not, that's fine. (Though, I'll at least keep these changes in my own project.)
Note here:
This PR causes passing
nullforgot,expected, orerror_intervalto eitherassert_almost_eqorassert_almost_neto fail the test. I'd assume both cases are correct, but, to be clear, this does mean thatassert_almost_ne(null, null, 1)fails rather than passes. If that sounds wrong, let me know, or just edit it. Either way, maybe we add a note to the docs for these methods?Thanks for this amazing tool!