Improve shardIndex/Count cli argument parsing

catchorg · Oct 26, 2021 · a3169e4 · a3169e4
1 parent fd39f65
commit a3169e4
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 20 deletions.
diff --git a/src/catch2/internal/catch_commandline.cpp b/src/catch2/internal/catch_commandline.cpp
@@ -150,15 +150,42 @@ namespace Catch {
             return ParserResult::ok( ParseResultType::Matched );
         };
         auto const setShardCount = [&]( std::string const& shardCount ) {
-            auto result = Clara::Detail::convertInto( shardCount, config.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");
+                }
 
-            if (config.shardCount == 0) {
-                return ParserResult::runtimeError( "The shard count must be greater than 0" );
-            } else {
-                return result;
+                config.shardCount = static_cast<unsigned int>(parsedCount);
+                return ParserResult::ok(ParseResultType::Matched);
+            } CATCH_CATCH_ANON(std::exception const&) {
+                return ParserResult::runtimeError("Could not parse '" + shardCount + "' as shard count");
             }
         };
 
+        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<unsigned int>(parsedIndex);
+                return ParserResult::ok(ParseResultType::Matched);
+            } CATCH_CATCH_ANON(std::exception const&) {
+                return ParserResult::runtimeError("Could not parse '" + shardIndex + "' as shard index");
+            }
+        };
+
+
         auto cli
             = ExeName( config.processName )
             | Help( config.showHelp )
@@ -252,7 +279,7 @@ namespace Catch {
             | Opt( setShardCount, "shard count" )
                 ["--shard-count"]
                 ( "split the tests to execute into this many groups" )
-            | Opt( config.shardIndex, "shard index" )
+            | Opt( setShardIndex, "shard index" )
                 ["--shard-index"]
                 ( "index of the group of tests to execute (see --shard-count)" )
             | Arg( config.testsOrTags, "test name|pattern|tags" )

diff --git a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp
@@ -569,27 +569,47 @@ TEST_CASE( "Process can be configured on command line", "[config][command-line]"
             REQUIRE(config.benchmarkWarmupTime == 10);
         }
     }
+}
 
-    SECTION("Sharding options") {
-        SECTION("shard-count") {
-            CHECK(cli.parse({ "test", "--shard-count=8"}));
+TEST_CASE("Parsing sharding-related cli flags", "[sharding]") {
+    using namespace Catch::Matchers;
 
-            REQUIRE(config.shardCount == 8);
-        }
+    Catch::ConfigData config;
+    auto cli = Catch::makeCommandLineParser(config);
 
-        SECTION("shard-index") {
-            CHECK(cli.parse({ "test", "--shard-index=2"}));
+    SECTION("shard-count") {
+        CHECK(cli.parse({ "test", "--shard-count=8" }));
 
-            REQUIRE(config.shardIndex == 2);
-        }
+        REQUIRE(config.shardCount == 8);
+    }
 
-        SECTION("Zero shard-count") {
-            auto result = cli.parse({ "test", "--shard-count=0"});
+    SECTION("Negative shard count reports error") {
+        auto result = cli.parse({ "test", "--shard-count=-1" });
 
-            CHECK( !result );
-            CHECK_THAT( result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) );
-        }
+        CHECK_FALSE(result);
+        REQUIRE_THAT(result.errorMessage(), ContainsSubstring("Shard count must be a positive number"));
     }
+
+    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"));
+    }
+
+    SECTION("shard-index") {
+        CHECK(cli.parse({ "test", "--shard-index=2" }));
+
+        REQUIRE(config.shardIndex == 2);
+    }
+
+    SECTION("Negative shard index reports error") {
+        auto result = cli.parse({ "test", "--shard-index=-12" });
+
+        CHECK_FALSE(result);
+        REQUIRE_THAT(result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number"));
+    }
+
 }
 
 TEST_CASE("Test with special, characters \"in name", "[cli][regression]") {