-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unit test refactor #67
Conversation
f86970d
to
d226908
Compare
d226908
to
355688f
Compare
) | ||
return act | ||
|
||
def test_editor_loads_jscm_parameters_match(self, colormap_file): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives us a set of 3 passing tests which just check attributes are as expected, and 3 xfail tests which check actual values loaded from a jscm
file.
@@ -18,52 +18,74 @@ def approxeq(x, y, *, err=0.0001): | |||
"viscm/examples/sample_diverging_continuous.jscm", | |||
], | |||
) | |||
@pytest.mark.xfail(reason="Test very old; intent unclear") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a test that ensures that the editor can load a .jscm
data file and interpret its values correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a test that ensures that the editor can load a .jscm data file and interpret its values correctly.
Got that; I'm referring to this assertion here when I say "intent unclear":
assert actual_colors[i][z] == np.rint(expected_colors[i][z] / 256)
Right now I'm not sure why this was ever working :)
Make the unit tests a bit easier to understand by splitting the attribute tests from the colormap value tests. Use
expected
/actual
terminology to (IMO) improve test readability.Should be rebased after #66 is mergedStill thinking about: how can these chained/dependent PRs be made easier to manage?