-
Notifications
You must be signed in to change notification settings - Fork 128
Improve test coverage for PolarizationCorrectionsHelpers methods #38602
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
Improve test coverage for PolarizationCorrectionsHelpers methods #38602
Conversation
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 additional test coverage here is really comprehensive and added in a way that should be easy to maintain.
I have just a few suggestions that might be worth considering.
Framework/Algorithms/test/PolarizationCorrections/PolarizationCorrectionsHelpersTest.h
Outdated
Show resolved
Hide resolved
Framework/Algorithms/test/PolarizationCorrections/PolarizationCorrectionsHelpersTest.h
Outdated
Show resolved
Hide resolved
Framework/Algorithms/test/PolarizationCorrections/PolarizationCorrectionsHelpersTest.h
Outdated
Show resolved
Hide resolved
…lper methods - added unit tests to splitSpinStateString - added unit tests to test indexOfWorkspaceForSpinState - added unit tests to test workSpaceForSpinState
fd6a6f8
to
f7cd3ee
Compare
I'm afraid I'm not actually seeing any changes on this PR since my last review - were some commits missed out the last time you pushed to this PR? |
I have updated the PR now @rbauststfc |
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 @yusufjimoh, I can see the changes now. All PR comments have been addressed, the new tests provide very comprehensive test coverage and are well-written for maintainability. This looks good to be merged in 👍
Description of work
Fixes #37749
To test:
This does not require release notes because it involves updating the unit tests