-
Notifications
You must be signed in to change notification settings - Fork 230
Fix #5464 - Properly handle "Separator" argument #5471
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
Changes from all commits
6d826b6
a53f31c
b22163c
9adf6d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| #include <utilities/idd/IddEnums.hxx> | ||
|
|
||
| #include "../../utilities/core/Finder.hpp" | ||
| #include "../../utilities/core/StringStreamLogSink.hpp" | ||
| #include "../../utilities/filetypes/WorkflowJSON.hpp" | ||
| #include "../../utilities/filetypes/WorkflowStep.hpp" | ||
|
|
||
|
|
@@ -457,3 +458,90 @@ TEST_F(MeasureFixture, UserScript_TestModelUserScriptDomain) { | |
| EXPECT_EQ("Integer User argument 'int_arg' has a value '-3' that is not in the domain [0, 2147483647].", result.stepErrors()[0]); | ||
| EXPECT_EQ(0u, result.stepWarnings().size()); | ||
| } | ||
|
|
||
| // Test for #5464 - Shouldn't throw an error when a separator is used in the arguments | ||
| class ModelMeasureWithSeparator : public ModelMeasure | ||
| { | ||
| public: | ||
| virtual std::string name() const override { | ||
| return "ModelMeasureWithSeparator"; | ||
| } | ||
|
|
||
| virtual std::vector<OSArgument> arguments(const Model& /*model*/) const override { | ||
| std::vector<OSArgument> result; | ||
|
|
||
| OSArgument arg = OSArgument::makeDoubleArgument("double_arg", true); | ||
| arg.setMaxValue(10.0); | ||
| result.push_back(arg); | ||
|
|
||
| arg = OSArgument::makeSeparatorArgument("separator"); | ||
| result.push_back(arg); | ||
|
Comment on lines
+477
to
+478
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New Test, failing before |
||
|
|
||
| arg = OSArgument::makeIntegerArgument("int_arg", true); | ||
| arg.setMinValue(0); | ||
| result.push_back(arg); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| // remove all spaces and add a new one | ||
| virtual bool run(Model& model, OSRunner& runner, const std::map<std::string, OSArgument>& user_arguments) const override { | ||
| ModelMeasure::run(model, runner, user_arguments); // initializes runner | ||
|
|
||
| return runner.validateUserArguments(arguments(model), user_arguments); | ||
| } | ||
| }; | ||
|
|
||
| TEST_F(MeasureFixture, ModelMeasureWithSeparator) { | ||
| ModelMeasureWithSeparator measure; | ||
| EXPECT_EQ("ModelMeasureWithSeparator", measure.name()); | ||
|
|
||
| Model model; | ||
|
|
||
| std::vector<WorkflowStep> steps; | ||
| steps.push_back(MeasureStep("dummy")); | ||
|
|
||
| WorkflowJSON workflow; | ||
| workflow.setWorkflowSteps(steps); | ||
|
|
||
| TestOSRunner runner(workflow); | ||
| OSArgumentVector arguments = measure.arguments(model); | ||
| std::map<std::string, OSArgument> argumentMap = convertOSArgumentVectorToMap(arguments); | ||
| ASSERT_EQ(3, argumentMap.size()); | ||
|
|
||
| OSArgument& double_arg = argumentMap["double_arg"]; | ||
| OSArgument& int_arg = argumentMap["int_arg"]; | ||
|
|
||
| StringStreamLogSink sink; | ||
| sink.setLogLevel(Fatal); | ||
|
|
||
| // call with a good value | ||
| double_arg.setValue(-1.0); | ||
| int_arg.setValue(1.0); | ||
| EXPECT_TRUE(measure.run(model, runner, argumentMap)); | ||
|
|
||
| EXPECT_EQ(0, sink.logMessages().size()) << sink.string(); | ||
| sink.resetStringStream(); | ||
|
Comment on lines
+515
to
+524
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fails before |
||
|
|
||
| WorkflowStepResult result = runner.result(); | ||
| ASSERT_TRUE(result.stepResult()); | ||
| EXPECT_EQ(StepResult::Success, result.stepResult()->value()); | ||
| EXPECT_EQ(0u, result.stepErrors().size()); | ||
| EXPECT_EQ(0u, result.stepWarnings().size()); | ||
|
|
||
| // Out of bound value for int_arg | ||
| runner.reset(); | ||
| double_arg.setValue(1.0); | ||
| int_arg.setValue(-3); | ||
| EXPECT_FALSE(measure.run(model, runner, argumentMap)); | ||
|
|
||
| EXPECT_EQ(0, sink.logMessages().size()) << sink.string(); | ||
| sink.resetStringStream(); | ||
|
|
||
| result = runner.result(); | ||
| ASSERT_TRUE(result.stepResult()); | ||
| EXPECT_EQ(StepResult::Fail, result.stepResult()->value()); | ||
| ASSERT_EQ(1u, result.stepErrors().size()); | ||
| EXPECT_EQ("Integer User argument 'int_arg' has a value '-3' that is not in the domain [0, 2147483647].", result.stepErrors()[0]); | ||
| EXPECT_EQ(0u, result.stepWarnings().size()); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,7 @@ | |
| <xs:enumeration value="String"/> | ||
| <xs:enumeration value="Choice"/> | ||
| <xs:enumeration value="Path"/> | ||
| <xs:enumeration value="Separator"/> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was missing cf #5464 (comment)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, I see there was a second error before: |
||
| </xs:restriction> | ||
| </xs:simpleType> | ||
| </xs:element> | ||
|
|
||
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.
No point validating it.