Update Unit Tests in PolarizationEffiency Cor #38458
Conversation
- wrote unit tests to check that wildes method perform well when spinstates is added - wrote unit tests to check that fredrik method throws exception when tested with wildes-only properties spinstates and flippers - wrote unit tests to check that fredrikze method can take AddSpinStateToLog property - refactored unit tests for maintainability
yusufjimoh
added
ISIS Team: LSS
| alg->setPropertyValue("SpinStatesOutWildes", "+, -"); | ||
|
|
||
| try { | ||
| alg->execute(); |
There was a problem hiding this comment.
It's a really good idea to check the specific error message here. Looking at some examples in other tests, this syntax might be a nice way of simplifying this a bit:
TS_ASSERT_THROWS_EQUALS(alg->execute(), const std::invalid_argument &e,
std::string(e.what()), "Property SpinStatesOutWildes cannot be used with the Fredrikze method.");
| try { | ||
| alg->execute(); | ||
| } catch (const std::invalid_argument &e) { | ||
| TSM_ASSERT_EQUALS("Incorrect exception message.", std::string(e.what()), |
There was a problem hiding this comment.
Could we make use of TS_ASSERT_THROWS_EQUALS here?
| try { | ||
| alg->execute(); | ||
| } catch (const std::invalid_argument &e) { | ||
| TSM_ASSERT_EQUALS("Incorrect exception message.", std::string(e.what()), |
There was a problem hiding this comment.
Could we make use of TS_ASSERT_THROWS_EQUALS here?
| alg.execute(); | ||
| setAlgorithmProperties(alg, "points", "Wildes"); | ||
| alg->setProperty("InputWorkspaces", inputs); | ||
| alg->execute(); | ||
| WorkspaceGroup_sptr out = AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>("out"); |
There was a problem hiding this comment.
Could we use the new checkWorkspaceGroupSize method here too?
| alg.execute(); | ||
| setAlgorithmProperties(alg, "points-short", "Wildes"); | ||
| alg->setProperty("InputWorkspaces", inputs); | ||
| alg->execute(); | ||
| WorkspaceGroup_sptr out = AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>("out"); |
There was a problem hiding this comment.
Could we use the new checkWorkspaceGroupSize method here too?
| void setAlgorithmProperties(const std::shared_ptr<PolarizationEfficiencyCor> &alg, | ||
| const std::string &efficiencyMethod, const std::string &method = "", | ||
| const std::string &analysisMethod = "") { | ||
| alg->setProperty("OutputWorkspace", "out"); |
There was a problem hiding this comment.
It looks like we set the output workspace and efficiencies in every test, so instead of having this separate setAlgorithmProperties method, could we just move all the logic that is here into createAlgorithm?
| } | ||
| } | ||
|
|
||
| void checkWorkspaceGroupSize(size_t expectedSize) { |
There was a problem hiding this comment.
This method is checking the size of a specific workspace group (i.e. the one called "out"). Would it be worth renaming it so that it's called checkOutputWorkspaceGroupSize to make that clearer?
| } | ||
|
|
||
| void checkWorkspaceGroupSize(size_t expectedSize) { | ||
| WorkspaceGroup_sptr out = AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>("out"); |
There was a problem hiding this comment.
Could we define "out" as a constant i.e. const std::string OUTPUT_GRP_NAME{"out"};. We use the value in a couple of places, so it would be nice to have it defined in just one place if we can.
| MatrixWorkspace_sptr createEfficiencies(std::string const &kind) { | ||
| static std::map<std::string, std::vector<std::string>> const labels = {{"Wildes", {"P1", "P2", "F1", "F2"}}, | ||
| {"Fredrikze", {"Pp", "Ap", "Rho", "Alpha"}}}; | ||
| if (kind == "Wildes" || kind == "Fredrikze") { | ||
| if (kind == WILDES_METHOD || kind == FREDRIKZE_METHOD) { |
There was a problem hiding this comment.
It's really great for maintainability that we're using the constants here. There are quite a few places in these tests where we're using "Wildes" and "Fredrikze", so could we go through and replace all of those that we can with the constants?
Description of work
Fixes #38287
To test:
This does not require release notes because it involves updating the unit tests