Skip to content
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

Test for correct applycutoff #63

Merged
merged 7 commits into from
Nov 11, 2024
Merged

Test for correct applycutoff #63

merged 7 commits into from
Nov 11, 2024

Conversation

sbillinge
Copy link
Contributor

@sbillinge sbillinge commented Nov 9, 2024

Closes #61

Replaces #62 and #47. Please see there for extensive discussion.

@bobleesj @cadenmyers13 I wrote a test for the applycutoff() method.

this is what I wanted to do so that we can catch the right behavior.

tbh I don't like the fourigui code, it is not modular and well separated, making it hard to understand and hard to test, but I don't want to do a big refactor on this code.

Still, take a look at the test and hopefully you can see this as an example of how to test a function/method like this. I hope it is helpful. The key difference from before is that I hand-wrote an array that I KNOW has the behavior that I want. It is an easily understandable 5x5x5 array that is the smallest array that can have both a qmin and qmax.

I guess that for all tests to pass we may have to do @cadenmyers13 fix of the h5 test data which is wrong because it was generated by the code which was wrong. But this shows how testing can catch that. If you test code using data that was generated using the code, of couse it will pass, and you are not testing anything really......

@sbillinge
Copy link
Contributor Author

ok, that got a bit messy with missing merges, my apologies for the ugly commit history, but I finally go there. I am inclined to merge it in this messy way since I don't really want to redo it on a clean branch for this code.

take a look and let me know if you have any questions.

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.78%. Comparing base (908992d) to head (fae69cf).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   93.49%   93.78%   +0.29%     
==========================================
  Files           4        4              
  Lines         169      177       +8     
==========================================
+ Hits          158      166       +8     
  Misses         11       11              
Files with missing lines Coverage Δ
tests/integration_test.py 98.03% <ø> (-0.58%) ⬇️
tests/test_fourigui.py 99.07% <100.00%> (ø)

@bobleesj
Copy link
Contributor

bobleesj commented Nov 9, 2024

@sbillinge Thanks for the demonstration - I haven't used mocking before but I see it can be useful for preventing certain functions like plotting. Yay, no more warnings!

@sbillinge
Copy link
Contributor Author

@sbillinge Thanks for the demonstration - I haven't used mocking before but I see it can be useful for preventing certain functions like plotting. Yay, no more warnings!

tbh, when you have to mock stuff like this it probably means the code design is bad and would benefit from a refactor.....but good to have in the back pocket.

It was ok here because the things I suppressed were functions that should be tested separately, so I just wanted to suppress them in the test and only test the desired behavior on the attributes.

@sbillinge sbillinge merged commit dfc8bbd into diffpy:main Nov 11, 2024
5 checks passed
@sbillinge sbillinge deleted the gt2 branch November 11, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Qmin Qmax to be within mask range
2 participants