From a43f67962e632d9596680a7e139df9e0a1423d3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sun, 23 Oct 2022 00:09:17 +0200 Subject: [PATCH] Refactor parsing of shard index/count in cmdline handling This worsens the message for negative numbers a bit, but simplifies the code enough that this is still a win. --- src/catch2/internal/catch_commandline.cpp | 46 +++++++------------ .../Baselines/compact.sw.approved.txt | 6 +-- .../Baselines/compact.sw.multi.approved.txt | 6 +-- .../Baselines/console.sw.approved.txt | 17 ++++--- .../Baselines/console.sw.multi.approved.txt | 17 ++++--- tests/SelfTest/Baselines/tap.sw.approved.txt | 6 +-- .../Baselines/tap.sw.multi.approved.txt | 6 +-- tests/SelfTest/Baselines/xml.sw.approved.txt | 12 ++--- .../Baselines/xml.sw.multi.approved.txt | 12 ++--- .../IntrospectiveTests/CmdLine.tests.cpp | 13 ++++-- 10 files changed, 65 insertions(+), 76 deletions(-) diff --git a/src/catch2/internal/catch_commandline.cpp b/src/catch2/internal/catch_commandline.cpp index 20bb07f79c..b6b812ecf5 100644 --- a/src/catch2/internal/catch_commandline.cpp +++ b/src/catch2/internal/catch_commandline.cpp @@ -7,7 +7,6 @@ // SPDX-License-Identifier: BSL-1.0 #include -#include #include #include #include @@ -177,42 +176,29 @@ namespace Catch { return ParserResult::ok( ParseResultType::Matched ); }; auto const setShardCount = [&]( std::string const& shardCount ) { - CATCH_TRY{ - std::size_t parsedTo = 0; - int64_t parsedCount = std::stoll(shardCount, &parsedTo, 0); - if (parsedTo != shardCount.size()) { - return ParserResult::runtimeError("Could not parse '" + shardCount + "' as shard count"); - } - if (parsedCount <= 0) { - return ParserResult::runtimeError("Shard count must be a positive number"); - } - - config.shardCount = static_cast(parsedCount); - return ParserResult::ok(ParseResultType::Matched); - } CATCH_CATCH_ANON(std::exception const&) { - return ParserResult::runtimeError("Could not parse '" + shardCount + "' as shard count"); + auto parsedCount = parseUInt( shardCount ); + if ( !parsedCount ) { + return ParserResult::runtimeError( + "Could not parse '" + shardCount + "' as shard count" ); } + if ( *parsedCount == 0 ) { + return ParserResult::runtimeError( + "Shard count must be positive" ); + } + config.shardCount = *parsedCount; + return ParserResult::ok( ParseResultType::Matched ); }; auto const setShardIndex = [&](std::string const& shardIndex) { - CATCH_TRY{ - std::size_t parsedTo = 0; - int64_t parsedIndex = std::stoll(shardIndex, &parsedTo, 0); - if (parsedTo != shardIndex.size()) { - return ParserResult::runtimeError("Could not parse '" + shardIndex + "' as shard index"); - } - if (parsedIndex < 0) { - return ParserResult::runtimeError("Shard index must be a non-negative number"); - } - - config.shardIndex = static_cast(parsedIndex); - return ParserResult::ok(ParseResultType::Matched); - } CATCH_CATCH_ANON(std::exception const&) { - return ParserResult::runtimeError("Could not parse '" + shardIndex + "' as shard index"); + auto parsedIndex = parseUInt( shardIndex ); + if ( !parsedIndex ) { + return ParserResult::runtimeError( + "Could not parse '" + shardIndex + "' as shard index" ); } + config.shardIndex = *parsedIndex; + return ParserResult::ok( ParseResultType::Matched ); }; - auto cli = ExeName( config.processName ) | Help( config.showHelp ) diff --git a/tests/SelfTest/Baselines/compact.sw.approved.txt b/tests/SelfTest/Baselines/compact.sw.approved.txt index 768562ad87..0a02356519 100644 --- a/tests/SelfTest/Baselines/compact.sw.approved.txt +++ b/tests/SelfTest/Baselines/compact.sw.approved.txt @@ -1287,13 +1287,13 @@ TestSpecParser.tests.cpp:: passed: spec.matches( testCase ) for: tr CmdLine.tests.cpp:: passed: cli.parse({ "test", "--shard-count=8" }) for: {?} CmdLine.tests.cpp:: passed: config.shardCount == 8 for: 8 == 8 CmdLine.tests.cpp:: passed: !(result) for: !{?} -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) for: "Could not parse '-1' as shard count" contains: "Could not parse '-1' as shard count" CmdLine.tests.cpp:: passed: !(result) for: !{?} -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) for: "Shard count must be positive" contains: "Shard count must be positive" CmdLine.tests.cpp:: passed: cli.parse({ "test", "--shard-index=2" }) for: {?} CmdLine.tests.cpp:: passed: config.shardIndex == 2 for: 2 == 2 CmdLine.tests.cpp:: passed: !(result) for: !{?} -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") for: "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) for: "Could not parse '-12' as shard index" contains: "Could not parse '-12' as shard index" CmdLine.tests.cpp:: passed: cli.parse({ "test", "--shard-index=0" }) for: {?} CmdLine.tests.cpp:: passed: config.shardIndex == 0 for: 0 == 0 TestSpecParser.tests.cpp:: passed: spec.hasFilters() for: true with 1 message: 'tagString := "[tag with spaces]"' diff --git a/tests/SelfTest/Baselines/compact.sw.multi.approved.txt b/tests/SelfTest/Baselines/compact.sw.multi.approved.txt index d8d0413026..20d14da1aa 100644 --- a/tests/SelfTest/Baselines/compact.sw.multi.approved.txt +++ b/tests/SelfTest/Baselines/compact.sw.multi.approved.txt @@ -1285,13 +1285,13 @@ TestSpecParser.tests.cpp:: passed: spec.matches( testCase ) for: tr CmdLine.tests.cpp:: passed: cli.parse({ "test", "--shard-count=8" }) for: {?} CmdLine.tests.cpp:: passed: config.shardCount == 8 for: 8 == 8 CmdLine.tests.cpp:: passed: !(result) for: !{?} -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) for: "Could not parse '-1' as shard count" contains: "Could not parse '-1' as shard count" CmdLine.tests.cpp:: passed: !(result) for: !{?} -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) for: "Shard count must be positive" contains: "Shard count must be positive" CmdLine.tests.cpp:: passed: cli.parse({ "test", "--shard-index=2" }) for: {?} CmdLine.tests.cpp:: passed: config.shardIndex == 2 for: 2 == 2 CmdLine.tests.cpp:: passed: !(result) for: !{?} -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") for: "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) for: "Could not parse '-12' as shard index" contains: "Could not parse '-12' as shard index" CmdLine.tests.cpp:: passed: cli.parse({ "test", "--shard-index=0" }) for: {?} CmdLine.tests.cpp:: passed: config.shardIndex == 0 for: 0 == 0 TestSpecParser.tests.cpp:: passed: spec.hasFilters() for: true with 1 message: 'tagString := "[tag with spaces]"' diff --git a/tests/SelfTest/Baselines/console.sw.approved.txt b/tests/SelfTest/Baselines/console.sw.approved.txt index 8bd2dc0c1b..929c746df5 100644 --- a/tests/SelfTest/Baselines/console.sw.approved.txt +++ b/tests/SelfTest/Baselines/console.sw.approved.txt @@ -9175,10 +9175,10 @@ with expansion: !{?} CmdLine.tests.cpp:: PASSED: - REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard count must be a positive number") ) + REQUIRE_THAT( result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) ) with expansion: - "Shard count must be a positive number" contains: "Shard count must be a - positive number" + "Could not parse '-1' as shard count" contains: "Could not parse '-1' as + shard count" ------------------------------------------------------------------------------- Parsing sharding-related cli flags @@ -9193,10 +9193,9 @@ with expansion: !{?} CmdLine.tests.cpp:: PASSED: - REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard count must be a positive number") ) + REQUIRE_THAT( result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) ) with expansion: - "Shard count must be a positive number" contains: "Shard count must be a - positive number" + "Shard count must be positive" contains: "Shard count must be positive" ------------------------------------------------------------------------------- Parsing sharding-related cli flags @@ -9228,10 +9227,10 @@ with expansion: !{?} CmdLine.tests.cpp:: PASSED: - REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") ) + REQUIRE_THAT( result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) ) with expansion: - "Shard index must be a non-negative number" contains: "Shard index must be a - non-negative number" + "Could not parse '-12' as shard index" contains: "Could not parse '-12' as + shard index" ------------------------------------------------------------------------------- Parsing sharding-related cli flags diff --git a/tests/SelfTest/Baselines/console.sw.multi.approved.txt b/tests/SelfTest/Baselines/console.sw.multi.approved.txt index 0f5655605f..685f57e773 100644 --- a/tests/SelfTest/Baselines/console.sw.multi.approved.txt +++ b/tests/SelfTest/Baselines/console.sw.multi.approved.txt @@ -9173,10 +9173,10 @@ with expansion: !{?} CmdLine.tests.cpp:: PASSED: - REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard count must be a positive number") ) + REQUIRE_THAT( result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) ) with expansion: - "Shard count must be a positive number" contains: "Shard count must be a - positive number" + "Could not parse '-1' as shard count" contains: "Could not parse '-1' as + shard count" ------------------------------------------------------------------------------- Parsing sharding-related cli flags @@ -9191,10 +9191,9 @@ with expansion: !{?} CmdLine.tests.cpp:: PASSED: - REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard count must be a positive number") ) + REQUIRE_THAT( result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) ) with expansion: - "Shard count must be a positive number" contains: "Shard count must be a - positive number" + "Shard count must be positive" contains: "Shard count must be positive" ------------------------------------------------------------------------------- Parsing sharding-related cli flags @@ -9226,10 +9225,10 @@ with expansion: !{?} CmdLine.tests.cpp:: PASSED: - REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") ) + REQUIRE_THAT( result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) ) with expansion: - "Shard index must be a non-negative number" contains: "Shard index must be a - non-negative number" + "Could not parse '-12' as shard index" contains: "Could not parse '-12' as + shard index" ------------------------------------------------------------------------------- Parsing sharding-related cli flags diff --git a/tests/SelfTest/Baselines/tap.sw.approved.txt b/tests/SelfTest/Baselines/tap.sw.approved.txt index cae94b24ec..53f1c437dd 100644 --- a/tests/SelfTest/Baselines/tap.sw.approved.txt +++ b/tests/SelfTest/Baselines/tap.sw.approved.txt @@ -2418,11 +2418,11 @@ ok {test-number} - config.shardCount == 8 for: 8 == 8 # Parsing sharding-related cli flags ok {test-number} - !(result) for: !{?} # Parsing sharding-related cli flags -ok {test-number} - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +ok {test-number} - result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) for: "Could not parse '-1' as shard count" contains: "Could not parse '-1' as shard count" # Parsing sharding-related cli flags ok {test-number} - !(result) for: !{?} # Parsing sharding-related cli flags -ok {test-number} - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +ok {test-number} - result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) for: "Shard count must be positive" contains: "Shard count must be positive" # Parsing sharding-related cli flags ok {test-number} - cli.parse({ "test", "--shard-index=2" }) for: {?} # Parsing sharding-related cli flags @@ -2430,7 +2430,7 @@ ok {test-number} - config.shardIndex == 2 for: 2 == 2 # Parsing sharding-related cli flags ok {test-number} - !(result) for: !{?} # Parsing sharding-related cli flags -ok {test-number} - result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") for: "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" +ok {test-number} - result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) for: "Could not parse '-12' as shard index" contains: "Could not parse '-12' as shard index" # Parsing sharding-related cli flags ok {test-number} - cli.parse({ "test", "--shard-index=0" }) for: {?} # Parsing sharding-related cli flags diff --git a/tests/SelfTest/Baselines/tap.sw.multi.approved.txt b/tests/SelfTest/Baselines/tap.sw.multi.approved.txt index 15942bb6a9..303ba842a4 100644 --- a/tests/SelfTest/Baselines/tap.sw.multi.approved.txt +++ b/tests/SelfTest/Baselines/tap.sw.multi.approved.txt @@ -2416,11 +2416,11 @@ ok {test-number} - config.shardCount == 8 for: 8 == 8 # Parsing sharding-related cli flags ok {test-number} - !(result) for: !{?} # Parsing sharding-related cli flags -ok {test-number} - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +ok {test-number} - result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) for: "Could not parse '-1' as shard count" contains: "Could not parse '-1' as shard count" # Parsing sharding-related cli flags ok {test-number} - !(result) for: !{?} # Parsing sharding-related cli flags -ok {test-number} - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") for: "Shard count must be a positive number" contains: "Shard count must be a positive number" +ok {test-number} - result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) for: "Shard count must be positive" contains: "Shard count must be positive" # Parsing sharding-related cli flags ok {test-number} - cli.parse({ "test", "--shard-index=2" }) for: {?} # Parsing sharding-related cli flags @@ -2428,7 +2428,7 @@ ok {test-number} - config.shardIndex == 2 for: 2 == 2 # Parsing sharding-related cli flags ok {test-number} - !(result) for: !{?} # Parsing sharding-related cli flags -ok {test-number} - result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") for: "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" +ok {test-number} - result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) for: "Could not parse '-12' as shard index" contains: "Could not parse '-12' as shard index" # Parsing sharding-related cli flags ok {test-number} - cli.parse({ "test", "--shard-index=0" }) for: {?} # Parsing sharding-related cli flags diff --git a/tests/SelfTest/Baselines/xml.sw.approved.txt b/tests/SelfTest/Baselines/xml.sw.approved.txt index 4358597c5a..acaee5b389 100644 --- a/tests/SelfTest/Baselines/xml.sw.approved.txt +++ b/tests/SelfTest/Baselines/xml.sw.approved.txt @@ -11160,10 +11160,10 @@ C - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") + result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) - "Shard count must be a positive number" contains: "Shard count must be a positive number" + "Could not parse '-1' as shard count" contains: "Could not parse '-1' as shard count" @@ -11179,10 +11179,10 @@ C - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") + result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) - "Shard count must be a positive number" contains: "Shard count must be a positive number" + "Shard count must be positive" contains: "Shard count must be positive" @@ -11217,10 +11217,10 @@ C - result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") + result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) - "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" + "Could not parse '-12' as shard index" contains: "Could not parse '-12' as shard index" diff --git a/tests/SelfTest/Baselines/xml.sw.multi.approved.txt b/tests/SelfTest/Baselines/xml.sw.multi.approved.txt index 9af9db43d7..75b3870647 100644 --- a/tests/SelfTest/Baselines/xml.sw.multi.approved.txt +++ b/tests/SelfTest/Baselines/xml.sw.multi.approved.txt @@ -11160,10 +11160,10 @@ C - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") + result.errorMessage(), ContainsSubstring( "Could not parse '-1' as shard count" ) - "Shard count must be a positive number" contains: "Shard count must be a positive number" + "Could not parse '-1' as shard count" contains: "Could not parse '-1' as shard count" @@ -11179,10 +11179,10 @@ C - result.errorMessage(), ContainsSubstring("Shard count must be a positive number") + result.errorMessage(), ContainsSubstring( "Shard count must be positive" ) - "Shard count must be a positive number" contains: "Shard count must be a positive number" + "Shard count must be positive" contains: "Shard count must be positive" @@ -11217,10 +11217,10 @@ C - result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") + result.errorMessage(), ContainsSubstring( "Could not parse '-12' as shard index" ) - "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" + "Could not parse '-12' as shard index" contains: "Could not parse '-12' as shard index" diff --git a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp index cbb0a98803..abb873f21f 100644 --- a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp +++ b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp @@ -649,14 +649,18 @@ TEST_CASE("Parsing sharding-related cli flags", "[sharding]") { auto result = cli.parse({ "test", "--shard-count=-1" }); CHECK_FALSE(result); - REQUIRE_THAT(result.errorMessage(), ContainsSubstring("Shard count must be a positive number")); + REQUIRE_THAT( + result.errorMessage(), + ContainsSubstring( "Could not parse '-1' as shard count" ) ); } SECTION("Zero shard count reports error") { auto result = cli.parse({ "test", "--shard-count=0" }); CHECK_FALSE(result); - REQUIRE_THAT(result.errorMessage(), ContainsSubstring("Shard count must be a positive number")); + REQUIRE_THAT( + result.errorMessage(), + ContainsSubstring( "Shard count must be positive" ) ); } SECTION("shard-index") { @@ -669,7 +673,9 @@ TEST_CASE("Parsing sharding-related cli flags", "[sharding]") { auto result = cli.parse({ "test", "--shard-index=-12" }); CHECK_FALSE(result); - REQUIRE_THAT(result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number")); + REQUIRE_THAT( + result.errorMessage(), + ContainsSubstring( "Could not parse '-12' as shard index" ) ); } SECTION("Shard index 0 is accepted") { @@ -677,7 +683,6 @@ TEST_CASE("Parsing sharding-related cli flags", "[sharding]") { REQUIRE(config.shardIndex == 0); } - } TEST_CASE( "Parsing warnings", "[cli][warnings]" ) {