Skip to content

Conversation

yusufjimoh
Copy link
Contributor

Description of work

  • added unit tests to test splitSpinStateString
  • added unit tests to test indexOfWorkspaceForSpinState
  • added unit tests to test workSpaceForSpinState

Fixes #37749

To test:

  • review unit tests
  • tests should pass

This does not require release notes because it involves updating the unit tests

@yusufjimoh yusufjimoh linked an issue Jan 14, 2025 that may be closed by this pull request
@yusufjimoh yusufjimoh marked this pull request as ready for review January 17, 2025 09:18
@yusufjimoh yusufjimoh added Reflectometry Issues and pull requests related to reflectometry SANS Issues and pull requests related to SANS Maintenance Unassigned issues to be addressed in the next maintenance period. ISIS: LSS Issue and pull requests relating to SANS and Reflectometry (Large Scale Structures) at ISIS labels Jan 17, 2025
@yusufjimoh yusufjimoh added this to the Release 6.13 milestone Jan 17, 2025
@rbauststfc rbauststfc self-assigned this Jan 17, 2025
Copy link
Contributor

@rbauststfc rbauststfc left a 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.

…lper methods

- added unit tests to splitSpinStateString
- added unit tests to test indexOfWorkspaceForSpinState
- added unit tests to test workSpaceForSpinState
@yusufjimoh yusufjimoh force-pushed the 37749-improve-test-coverage-for-polarizationcorrectionshelpers-methods branch from fd6a6f8 to f7cd3ee Compare January 22, 2025 20:23
@yusufjimoh yusufjimoh requested a review from rbauststfc January 23, 2025 09:37
@rbauststfc
Copy link
Contributor

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?

@yusufjimoh
Copy link
Contributor Author

I have updated the PR now @rbauststfc

Copy link
Contributor

@rbauststfc rbauststfc left a 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 👍

@SilkeSchomann SilkeSchomann self-assigned this Jan 24, 2025
@SilkeSchomann SilkeSchomann merged commit 516709e into main Jan 24, 2025
10 checks passed
@SilkeSchomann SilkeSchomann deleted the 37749-improve-test-coverage-for-polarizationcorrectionshelpers-methods branch January 24, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS: LSS Issue and pull requests relating to SANS and Reflectometry (Large Scale Structures) at ISIS Maintenance Unassigned issues to be addressed in the next maintenance period. Reflectometry Issues and pull requests related to reflectometry SANS Issues and pull requests related to SANS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve test coverage for PolarizationCorrectionsHelpers methods
3 participants