-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add unit tests for samples
module
#69
Conversation
samples
module
This all works, but I will have to rethink the Checking whether the input to the matplotlib figures are as expected, instead of the figures themselves, and then testing save_plot separately, makes sense theoretically. But the calls to get the input are really simple, (just Will go back to the drawing board for these tests tomorrow morning. I'll probably refrain from comparing the png's to a reference figure as I do now. It's quite expensive, and I am not 100% convinced of it being a very valuable test. My current thinking goes towards creating a helper function in the method to obtain the profile, calling that function when In that case Either way, I'll look into that tomorrow morning. Exact details in that implementation may differ. The plan is to declare this PR ready for review tomorrow at least (assuming that all goes well). |
This should now be ready and working, currently at 93% coverage. Only thing not being tested right now is nested sampling, but I may add that later in a separate PR. |
Reset back to working dir after test
c7daf25
to
0dad997
Compare
Found why the tests where failing, while testing the main function the module is run from a temporary dir which is deleted after the run. This is because I didn't want the test to generate data in the actual results directory. Then when test_simulate tries to call I now just reworked it to be able to set the save_path when running main, such that it's not possible to keep messing around with |
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.
I see you have upped your mocking game, good work. A few comments here, but mostly very small
You are correct about the blank lines, flake8 detects a lot of them locally (including the double blank lines): I'll probably merge PR #72 first, and then merge the new main into this branch, to get this into the workflow. |
ba73fab
to
17d589c
Compare
Fix quotes
17d589c
to
45e846e
Compare
Merge imaginary SLD into samples test
This all works quite well. The tests take 2 s and 20 ms on my machine, of which roughly 2 s being from the modules were figures are generated. I can consider removing those tests, or reducing the quality of those temporary figures even more, as it just checks if a valid file is generated. At a dpi of 6, the time goes down to 1s 46 ms, and at a dpi of 5 and lower the figure tests start to fail. Tomorrow I'll go take a look if I can just check if save_plot is called successfully for those methods instead. That should definitely be possible with the mock function. Then the actual save_plot function can be tested separately in the |
For the times I really wouldn't worry about a second here or there. If we have system tests they will possibly take minutes, so save your effort and leave it I think |
Okay, I'll keep it as-is for now. A simple alternative would be to use |
There's actually no conflicts at all with the latest main branch, so this is ready for review. |
Adds unit tests for the
samples
module, still.TODO:
SLD_profile
andreflectivity_profile
tests.