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

Add unit tests for merging rows into the reflectometry GUI #26518

Merged

Conversation

gemmaguest
Copy link
Member

@gemmaguest gemmaguest commented Jul 29, 2019

This PR adds additional unit tests to do with merging rows into the ISIS Reflectometry GUI. In particular, it tests the mergeRowIntoGroup function, which updates the ReductionJobs model with the merged row (if a merge is applicable).

There is also some minor tidying to simplify the function interface and remove spurious warnings from existing tests.

To test:

Refs #26512

This does not require release notes because it concerns testing only


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

@gemmaguest gemmaguest added Reflectometry Issues and pull requests related to reflectometry Not Ready ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS labels Jul 29, 2019
@gemmaguest gemmaguest added this to the Release 4.2 S1 milestone Jul 29, 2019
@gemmaguest gemmaguest force-pushed the 26512_add_refl_gui_merge_row_tests branch from 90e1429 to 4043c6d Compare July 29, 2019 17:41
@dtasev dtasev self-assigned this Aug 5, 2019
@dtasev dtasev self-requested a review August 5, 2019 13:56
Copy link
Contributor

@dtasev dtasev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good, pass locally and on build servers. Some merge conflicts have arisen though

Remove unnecessary function pointer argument because this function is only called from one place so it is always the same.

Re mantidproject#26512
Make listener a nice mock in line with other tests

Re mantidproject#26512
@gemmaguest gemmaguest force-pushed the 26512_add_refl_gui_merge_row_tests branch from 4043c6d to 4e2dbc1 Compare August 6, 2019 09:02
@dtasev
Copy link
Contributor

dtasev commented Aug 6, 2019

I've rebuilt the failed Windows build 31771

@gemmaguest gemmaguest requested a review from dtasev August 12, 2019 07:51
Copy link
Contributor

@dtasev dtasev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@DanNixon DanNixon merged commit da917eb into mantidproject:master Aug 12, 2019
@gemmaguest gemmaguest deleted the 26512_add_refl_gui_merge_row_tests branch September 17, 2019 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS Reflectometry Issues and pull requests related to reflectometry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants