mantid icon indicating copy to clipboard operation
mantid copied to clipboard

Add unit test for Experiment Presenter notifyPreviewApplyRequested method

Open rbauststfc opened this issue 2 years ago • 1 comments

Describe the outcome that is desired. Add a unit test for method ExperimentPresenter::notifyPreviewApplyRequested. This method was added in PR #34261, however adding a unit test proved complex because the Experiment model is not currently mocked and it turned out not to be straightforward to set this up.

Describe any solutions you are considering We considered two different approaches to write the test:

  1. Create an Experiment object with test data. Some of the tests in ExperimentPresenterTest.h seem to use this approach, however it's done in cases where the presenter method being tested doesn't call any of the model methods. In our case we call two methods on the model, so this wouldn't be ideal.

  2. Add an IExperiment interface and create a mock Experiment model. We started down this path but encountered some complications because currently the model is being passed into the presenter and stored in a member variable by value rather than as a pointer. It's unclear why this is, so that needs to be established first. Even if we decide to change this to use a pointer, it looks like some work may be required to update relevant types.

Additional context Note that the Experiment model is located in the ISISReflectometry Reduction directory, while the presenter is in the GUI/Experiment directory.

rbauststfc avatar Aug 10 '22 09:08 rbauststfc

We've established that it wasn't a deliberate decision to have no interface + mocking for the Experiment model, so this can be changed. Further information from Gemma:

It would be better to use a unique_ptr to an interface and mock it. The tests are not set up to use mock models (which is why we have all that model creation helper stuff to create real models) but I don't think it would be too difficult to start using it in new tests. One thing to note, though, is that the model does not currently have its own tests - by using real models, it's tested implicitly in the presenter tests. So we should also add model tests for any new behaviour in the model if it is not going to be tested via the presenter.

rbauststfc avatar Aug 10 '22 14:08 rbauststfc