From 5270e5b0323e2fa4abb6fa2188dbc367b9e5cc5a Mon Sep 17 00:00:00 2001 From: Marcos Bento Date: Wed, 13 Mar 2024 17:40:24 +0000 Subject: [PATCH 1/3] Fix --meter "name" "-1" Re ECFLOW-1948 --- Base/src/ecflow/base/ClientOptionsParser.cpp | 29 ++++- Base/src/ecflow/base/cts/task/MeterCmd.cpp | 4 + Client/test/TestClientOptions.cpp | 130 ++++++++++++++----- 3 files changed, 131 insertions(+), 32 deletions(-) diff --git a/Base/src/ecflow/base/ClientOptionsParser.cpp b/Base/src/ecflow/base/ClientOptionsParser.cpp index e8043feb8..c46030052 100644 --- a/Base/src/ecflow/base/ClientOptionsParser.cpp +++ b/Base/src/ecflow/base/ClientOptionsParser.cpp @@ -109,7 +109,7 @@ void parse_alter(ClientOptionsParser::option_set_t& processed_options, ClientOpt void parse_label(ClientOptionsParser::option_set_t& processed_options, ClientOptionsParser::arguments_set_t& args) { // *** Important! *** - // This custom handler is needed to ensure that the "--alter" option + // This custom handler is needed to ensure that the "--label" option // special value parameters are handled correctly. For example, // values starting with -, such as "-j64". // @@ -129,6 +129,30 @@ void parse_label(ClientOptionsParser::option_set_t& processed_options, ClientOpt processed_options.push_back(label); } +void parse_meter(ClientOptionsParser::option_set_t& processed_options, ClientOptionsParser::arguments_set_t& args) { + + // *** Important! *** + // This custom handler is needed to ensure that the "--meter" option + // special value parameters are handled correctly. For example, + // values starting with -, such as "-1". + // + // The custom handling will consider that 2 positional values (not + // to be confused with positional arguments) are provided with the + // --label option, as per one of the following forms: + // 1) --label arg1 arg2 + // 2) --label=arg1 arg2 + + ClientOptionsParser::option_t meter{std::string{"meter"}, {}}; + + parse_option(meter, processed_options, args); + + // Collect 1 positional argument (i.e. the label value) + parse_positional_arguments(meter, processed_options, args, 1); + + processed_options.push_back(meter); +} + + } // namespace ClientOptionsParser::option_set_t ClientOptionsParser::operator()(ClientOptionsParser::arguments_set_t& args) { @@ -140,6 +164,9 @@ ClientOptionsParser::option_set_t ClientOptionsParser::operator()(ClientOptionsP else if (ecf::algorithm::starts_with(args[0], "--label")) { parse_label(processed_options, args); } + else if (ecf::algorithm::starts_with(args[0], "--meter")) { + parse_meter(processed_options, args); + } return processed_options; } diff --git a/Base/src/ecflow/base/cts/task/MeterCmd.cpp b/Base/src/ecflow/base/cts/task/MeterCmd.cpp index 963e43b0f..a6a890f95 100644 --- a/Base/src/ecflow/base/cts/task/MeterCmd.cpp +++ b/Base/src/ecflow/base/cts/task/MeterCmd.cpp @@ -122,6 +122,10 @@ void MeterCmd::create(Cmd_ptr& cmd, boost::program_options::variables_map& vm, A throw std::runtime_error(ss.str()); } + if (args[0].empty()) { + throw std::runtime_error("MeterCmd: First argument must be a non-empty string, i.e. --meter=name 100\n"); + } + int value = 0; try { std::string strVal = args[1]; diff --git a/Client/test/TestClientOptions.cpp b/Client/test/TestClientOptions.cpp index 9de2586af..2f952d1da 100644 --- a/Client/test/TestClientOptions.cpp +++ b/Client/test/TestClientOptions.cpp @@ -11,6 +11,7 @@ #include #include "ecflow/base/cts/task/LabelCmd.hpp" +#include "ecflow/base/cts/task/MeterCmd.hpp" #include "ecflow/base/cts/user/AlterCmd.hpp" #include "ecflow/client/ClientEnvironment.hpp" #include "ecflow/client/ClientInvoker.hpp" @@ -22,8 +23,8 @@ /// \brief Tests the capabilities of ClientOptions /// -template -void test_label(const CommandLine& cl, REQUIRE check) { +template +void test_command(const CommandLine& cl, REQUIRE check) { std::cout << "Testing command line: " << cl.original() << std::endl; ClientOptions options; @@ -33,7 +34,7 @@ void test_label(const CommandLine& cl, REQUIRE check) { environment.set_child_password("password"); try { auto base_command = options.parse(cl, &environment); - auto derived_command = dynamic_cast(base_command.get()); + auto derived_command = dynamic_cast(base_command.get()); BOOST_REQUIRE(derived_command); check(*derived_command, environment); @@ -44,21 +45,18 @@ void test_label(const CommandLine& cl, REQUIRE check) { } template -void test_alter(const CommandLine& cl, REQUIRE check) { - std::cout << "Testing command line: " << cl.original() << std::endl; +void test_label(const CommandLine& cl, REQUIRE check) { + test_command(cl, check); +} - ClientOptions options; - ClientEnvironment environment(false); - try { - auto base_command = options.parse(cl, &environment); - auto derived_command = dynamic_cast(base_command.get()); +template +void test_meter(const CommandLine& cl, REQUIRE check) { + test_command(cl, check); +} - BOOST_REQUIRE(derived_command); - check(*derived_command, environment); - } - catch (boost::program_options::unknown_option& e) { - BOOST_FAIL(std::string("Unexpected exception caught: ") + e.what()); - } +template +void test_alter(const CommandLine& cl, REQUIRE check) { + test_command(cl, check); } BOOST_AUTO_TEST_SUITE(S_Client) @@ -748,50 +746,120 @@ BOOST_AUTO_TEST_CASE(test_is_able_handle_alter_delete) { BOOST_AUTO_TEST_CASE(test_is_able_to_handle_label) { { - auto cl = CommandLine::make_command_line("ecflow_client", - "--label=name", - "some value with spaces"); + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", "some value with spaces"); test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { BOOST_REQUIRE_EQUAL(command.name(), "name"); BOOST_REQUIRE_EQUAL(command.label(), "some value with spaces"); }); } { - auto cl = CommandLine::make_command_line("ecflow_client", - "--label=name", - R"(some "quoted" value)"); + auto cl = CommandLine::make_command_line("ecflow_client", "--label", "name", "some value with spaces"); + test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.label(), "some value with spaces"); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", R"(some "quoted" value)"); + test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.label(), R"(some "quoted" value)"); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--label", "name", R"(some "quoted" value)"); test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { BOOST_REQUIRE_EQUAL(command.name(), "name"); BOOST_REQUIRE_EQUAL(command.label(), R"(some "quoted" value)"); }); } { - auto cl = CommandLine::make_command_line("ecflow_client", - "--label=name", - "-j64"); + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", "-j64"); test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { BOOST_REQUIRE_EQUAL(command.name(), "name"); BOOST_REQUIRE_EQUAL(command.label(), "-j64"); }); } { - auto cl = CommandLine::make_command_line("ecflow_client", - "--label=name", - "--long-option"); + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", "--long-option"); test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { BOOST_REQUIRE_EQUAL(command.name(), "name"); BOOST_REQUIRE_EQUAL(command.label(), "--long-option"); }); } { - auto cl = CommandLine::make_command_line("ecflow_client", - "--label=name", - "--option=value"); + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", "--option=value"); test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { BOOST_REQUIRE_EQUAL(command.name(), "name"); BOOST_REQUIRE_EQUAL(command.label(), "--option=value"); }); } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", "--debug", "--debug"); + test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.label(), "--debug"); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--label=name", "--port", "--debug", "--port", "123"); + test_label(cl, [&](const LabelCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.label(), "--port"); + }); + } +} + +BOOST_AUTO_TEST_CASE(test_is_able_to_handle_meter) { + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter=name", "0"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), 0); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter", "name", "0"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), 0); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter=name", "-1"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), -1); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter", "name", "-1"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), -1); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter=name", "10"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), 10); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter=name", "20", "--debug"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), 20); + }); + } + { + auto cl = CommandLine::make_command_line("ecflow_client", "--meter=name", "-100", "--debug", "--port", "1234"); + test_meter(cl, [&](const MeterCmd& command, const ClientEnvironment& env) { + BOOST_REQUIRE_EQUAL(command.name(), "name"); + BOOST_REQUIRE_EQUAL(command.value(), -100); + }); + } } BOOST_AUTO_TEST_SUITE_END() From e99bbab3148d0ce3ea33f6986e2cc2b77df01a11 Mon Sep 17 00:00:00 2001 From: Marcos Bento Date: Thu, 14 Mar 2024 11:31:12 +0000 Subject: [PATCH 2/3] Add tests to cover all task commands - Fix --abort --dashed-reason Re ECFLOW-1948 --- Base/src/ecflow/base/ClientOptionsParser.cpp | 36 +- Base/src/ecflow/base/cts/task/QueueCmd.cpp | 2 +- Base/src/ecflow/base/cts/user/BeginCmd.cpp | 1 + Client/test/TestClientOptions.cpp | 672 ++++++++++++++++--- 4 files changed, 613 insertions(+), 98 deletions(-) diff --git a/Base/src/ecflow/base/ClientOptionsParser.cpp b/Base/src/ecflow/base/ClientOptionsParser.cpp index c46030052..9db3c4e89 100644 --- a/Base/src/ecflow/base/ClientOptionsParser.cpp +++ b/Base/src/ecflow/base/ClientOptionsParser.cpp @@ -23,9 +23,17 @@ bool is_valid_path(const std::string& path) { void parse_option(ClientOptionsParser::option_t& option, ClientOptionsParser::option_set_t& processed_options, ClientOptionsParser::arguments_set_t& args) { + // We must consider two forms of options: + // 1) --