-
Notifications
You must be signed in to change notification settings - Fork 21
Use pytest for test_loaddata.py
#179
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 99.17% 99.56% +0.39%
==========================================
Files 7 7
Lines 242 230 -12
==========================================
- Hits 240 229 -11
+ Misses 2 1 -1
|
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.
Thanks @bobleesj this is really great. Please see my comments. Since we are doing this, let's make a couple of the tests a bit better?
d2c = np.array([[3, 31], [4, 32], [5, 33]]) | ||
|
||
with pytest.raises(IOError): | ||
loadData("doesnotexist") |
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.
do we want to test a preferred error message here?
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.
Seems like we use the default error message that comes along with IOError
, I have also assert assert "No such file or directory" in str(err.value)
shown below:
with pytest.raises(IOError) as err:
loadData("doesnotexist")
assert "No such file or directory" in str(err.value)
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.
Remember that the tests always test what "want" to happen, i.e., desired behavior, not what we actually do..... (of course the test will then fail and we have to modify the code, but then the point is that the code then does what we want it to.....). If I were a user, especially a not very programming savvy one, and got a stack dump with IOError
, a more useful message might be something like File doesnotexist cannot be found. Please rerun the program specifying a valid filename
or something like that?
assert len(hdata) == 14 | ||
|
||
# Check the following are floats | ||
vfloats = ["wavelength", "qmaxinst", "qmin", "qmax", "bgscale"] |
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.
These tests seem a bit weak. Don't we want to actually test that the correct dictionary was loaded?
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.
Right, in the updated code, we compare against the expected output.
expected = {
"wavelength": 0.1,
"dataformat": "Qnm",
"inputfile": "darkSub_rh20_C_01.chi",
"mode": "xray",
"bgscale": 1.2998929285,
"composition": "0.800.20",
"outputtype": "gr",
"qmaxinst": 25.0,
"qmin": 0.1,
"qmax": 25.0,
"rmax": "100.0r",
"rmin": "0.0r",
"rstep": "0.01r",
"rpoly": "0.9r",
}
@sbillinge ready for review. Thank you. |
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.
Thnaks @bobleesj . Please see inline comment
d2c = np.array([[3, 31], [4, 32], [5, 33]]) | ||
|
||
with pytest.raises(IOError): | ||
loadData("doesnotexist") |
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.
Remember that the tests always test what "want" to happen, i.e., desired behavior, not what we actually do..... (of course the test will then fail and we have to modify the code, but then the point is that the code then does what we want it to.....). If I were a user, especially a not very programming savvy one, and got a stack dump with IOError
, a more useful message might be something like File doesnotexist cannot be found. Please rerun the program specifying a valid filename
or something like that?
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.
@sbillinge ready for review.
nicely done there! |
Closes #161 - since we may deprecate
loadData
toload_data
or so, spent 5-10 mins to refactor this code.