From 2b54877a7d8425698d83e8842b1be326f38b5004 Mon Sep 17 00:00:00 2001 From: Ben Dunkin Date: Sun, 11 Jul 2021 12:46:05 -0700 Subject: [PATCH 1/4] Allow test sharding for e.g. Bazel test sharding feature This greatly simplifies running Catch2 tests in single binary in parallel from external test runners. Instead of having to shard the tests by tags/test names, an external test runner can now just ask for test shard 2 (out of X), and execute that in single process, without having to know what tests are actually in the shard. Note that sharding also applies to test listing, and happens after tests were ordered according to the `--order` feature. --- docs/command-line.md | 17 ++++ src/catch2/catch_all.hpp | 1 + src/catch2/catch_config.cpp | 2 + src/catch2/catch_config.hpp | 5 ++ src/catch2/catch_session.cpp | 10 +++ .../interfaces/catch_interfaces_config.hpp | 2 + src/catch2/internal/catch_commandline.cpp | 15 ++++ src/catch2/internal/catch_sharding.hpp | 41 +++++++++ .../catch_test_case_registry_impl.cpp | 3 +- tests/CMakeLists.txt | 2 + tests/ExtraTests/CMakeLists.txt | 16 ++++ .../Baselines/compact.sw.approved.txt | 6 ++ .../Baselines/console.std.approved.txt | 2 +- .../Baselines/console.sw.approved.txt | 57 +++++++++++- .../SelfTest/Baselines/junit.sw.approved.txt | 5 +- .../Baselines/sonarqube.sw.approved.txt | 3 + tests/SelfTest/Baselines/tap.sw.approved.txt | 14 ++- tests/SelfTest/Baselines/xml.sw.approved.txt | 68 +++++++++++++- .../IntrospectiveTests/CmdLine.tests.cpp | 21 +++++ .../IntrospectiveTests/Sharding.tests.cpp | 41 +++++++++ tests/TestScripts/testSharding.py | 90 +++++++++++++++++++ 21 files changed, 415 insertions(+), 6 deletions(-) create mode 100644 src/catch2/internal/catch_sharding.hpp create mode 100644 tests/SelfTest/IntrospectiveTests/Sharding.tests.cpp create mode 100644 tests/TestScripts/testSharding.py diff --git a/docs/command-line.md b/docs/command-line.md index f8e8027009..0154f07593 100644 --- a/docs/command-line.md +++ b/docs/command-line.md @@ -29,6 +29,7 @@ [Specify the section to run](#specify-the-section-to-run)
[Filenames as tags](#filenames-as-tags)
[Override output colouring](#override-output-colouring)
+[Test Sharding](#test-sharding)
Catch works quite nicely without any command line options at all - but for those times when you want greater control the following options are available. Click one of the following links to take you straight to that option - or scroll on to browse the available options. @@ -67,6 +68,8 @@ Click one of the following links to take you straight to that option - or scroll ` --benchmark-no-analysis`
` --benchmark-warmup-time`
` --use-colour`
+ ` --shard-count`
+ ` --shard-index`

@@ -425,6 +428,20 @@ processing of output. `--use-colour yes` forces coloured output, `--use-colour no` disables coloured output. The default behaviour is `--use-colour auto`. + +## Test Sharding +
--shard-count <#number of shards>, --shard-index <#shard index to run>
+ +> [Introduced](https://github.com/catchorg/Catch2/pull/2257) in Catch2 X.Y.Z. + +When `--shard-count <#number of shards>` is used, the tests to execute will be split evenly in to the given number of sets, +identified by indicies starting at 0. The tests in the set given by `--shard-index <#shard index to run>` will be executed. +The default shard count is `1`, and the default index to run is `0`. It is an error to specify a shard index greater than +the number of shards. + +This is useful when you want to split test execution across multiple processes, as is done with [Bazel test sharding](https://docs.bazel.build/versions/main/test-encyclopedia.html#test-sharding). + + --- [Home](Readme.md#top) diff --git a/src/catch2/catch_all.hpp b/src/catch2/catch_all.hpp index 27282de556..742d89a92d 100644 --- a/src/catch2/catch_all.hpp +++ b/src/catch2/catch_all.hpp @@ -85,6 +85,7 @@ #include #include #include +#include #include #include #include diff --git a/src/catch2/catch_config.cpp b/src/catch2/catch_config.cpp index b79059ecb8..c9fc8eef51 100644 --- a/src/catch2/catch_config.cpp +++ b/src/catch2/catch_config.cpp @@ -73,6 +73,8 @@ namespace Catch { double Config::minDuration() const { return m_data.minDuration; } TestRunOrder Config::runOrder() const { return m_data.runOrder; } uint32_t Config::rngSeed() const { return m_data.rngSeed; } + unsigned int Config::shardCount() const { return m_data.shardCount; } + unsigned int Config::shardIndex() const { return m_data.shardIndex; } UseColour Config::useColour() const { return m_data.useColour; } bool Config::shouldDebugBreak() const { return m_data.shouldDebugBreak; } int Config::abortAfter() const { return m_data.abortAfter; } diff --git a/src/catch2/catch_config.hpp b/src/catch2/catch_config.hpp index 7b67ba7688..6aa6c4d69c 100644 --- a/src/catch2/catch_config.hpp +++ b/src/catch2/catch_config.hpp @@ -37,6 +37,9 @@ namespace Catch { int abortAfter = -1; uint32_t rngSeed = generateRandomSeed(GenerateFrom::Default); + unsigned int shardCount = 1; + unsigned int shardIndex = 0; + bool benchmarkNoAnalysis = false; unsigned int benchmarkSamples = 100; double benchmarkConfidenceInterval = 0.95; @@ -99,6 +102,8 @@ namespace Catch { double minDuration() const override; TestRunOrder runOrder() const override; uint32_t rngSeed() const override; + unsigned int shardCount() const override; + unsigned int shardIndex() const override; UseColour useColour() const override; bool shouldDebugBreak() const override; int abortAfter() const override; diff --git a/src/catch2/catch_session.cpp b/src/catch2/catch_session.cpp index e7878117bf..3b58adbd07 100644 --- a/src/catch2/catch_session.cpp +++ b/src/catch2/catch_session.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -72,6 +73,8 @@ namespace Catch { for (auto const& match : m_matches) m_tests.insert(match.tests.begin(), match.tests.end()); } + + m_tests = createShard(m_tests, m_config->shardCount(), m_config->shardIndex()); } Totals execute() { @@ -171,6 +174,7 @@ namespace Catch { return 1; auto result = m_cli.parse( Clara::Args( argc, argv ) ); + if( !result ) { config(); getCurrentMutableContext().setConfig(m_config.get()); @@ -253,6 +257,12 @@ namespace Catch { if( m_startupExceptions ) return 1; + + if( m_configData.shardIndex >= m_configData.shardCount ) { + Catch::cerr() << "The shard count (" << m_configData.shardCount << ") must be greater than the shard index (" << m_configData.shardIndex << ")\n" << std::flush; + return 1; + } + if (m_configData.showHelp || m_configData.libIdentify) { return 0; } diff --git a/src/catch2/interfaces/catch_interfaces_config.hpp b/src/catch2/interfaces/catch_interfaces_config.hpp index e40cf7023a..bd0c8a408a 100644 --- a/src/catch2/interfaces/catch_interfaces_config.hpp +++ b/src/catch2/interfaces/catch_interfaces_config.hpp @@ -74,6 +74,8 @@ namespace Catch { virtual std::vector const& getTestsOrTags() const = 0; virtual TestRunOrder runOrder() const = 0; virtual uint32_t rngSeed() const = 0; + virtual unsigned int shardCount() const = 0; + virtual unsigned int shardIndex() const = 0; virtual UseColour useColour() const = 0; virtual std::vector const& getSectionsToRun() const = 0; virtual Verbosity verbosity() const = 0; diff --git a/src/catch2/internal/catch_commandline.cpp b/src/catch2/internal/catch_commandline.cpp index ad571cc5ed..5f8a44d566 100644 --- a/src/catch2/internal/catch_commandline.cpp +++ b/src/catch2/internal/catch_commandline.cpp @@ -149,6 +149,15 @@ namespace Catch { return ParserResult::runtimeError( "Unrecognized reporter, '" + reporter + "'. Check available with --list-reporters" ); return ParserResult::ok( ParseResultType::Matched ); }; + auto const setShardCount = [&]( std::string const& shardCount ) { + auto result = Clara::Detail::convertInto( shardCount, config.shardCount ); + + if (config.shardCount == 0) { + return ParserResult::runtimeError( "The shard count must be greater than 0" ); + } else { + return result; + } + }; auto cli = ExeName( config.processName ) @@ -240,6 +249,12 @@ namespace Catch { | Opt( config.benchmarkWarmupTime, "benchmarkWarmupTime" ) ["--benchmark-warmup-time"] ( "amount of time in milliseconds spent on warming up each test (default: 100)" ) + | Opt( setShardCount, "shard count" ) + ["--shard-count"] + ( "split the tests to execute into this many groups" ) + | Opt( config.shardIndex, "shard index" ) + ["--shard-index"] + ( "index of the group of tests to execute (see --shard-count)" ) | Arg( config.testsOrTags, "test name|pattern|tags" ) ( "which test or tests to use" ); diff --git a/src/catch2/internal/catch_sharding.hpp b/src/catch2/internal/catch_sharding.hpp new file mode 100644 index 0000000000..0e8822ee91 --- /dev/null +++ b/src/catch2/internal/catch_sharding.hpp @@ -0,0 +1,41 @@ + +// Copyright Catch2 Authors +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// https://www.boost.org/LICENSE_1_0.txt) + +// SPDX-License-Identifier: BSL-1.0 +#ifndef CATCH_SHARDING_HPP_INCLUDED +#define CATCH_SHARDING_HPP_INCLUDED + +#include + +#include + +namespace Catch { + + template + Container createShard(Container const& container, std::size_t const shardCount, std::size_t const shardIndex) { + assert(shardCount > shardIndex); + + if (shardCount == 1) { + return container; + } + + const std::size_t totalTestCount = container.size(); + + const std::size_t shardSize = totalTestCount / shardCount; + const std::size_t leftoverTests = totalTestCount % shardCount; + + const std::size_t startIndex = shardIndex * shardSize + (std::min)(shardIndex, leftoverTests); + const std::size_t endIndex = (shardIndex + 1) * shardSize + (std::min)(shardIndex + 1, leftoverTests); + + auto startIterator = std::next(container.begin(), startIndex); + auto endIterator = std::next(container.begin(), endIndex); + + return Container(startIterator, endIterator); + } + +} + +#endif // CATCH_SHARDING_HPP_INCLUDED diff --git a/src/catch2/internal/catch_test_case_registry_impl.cpp b/src/catch2/internal/catch_test_case_registry_impl.cpp index dd1744a707..3c1ab54b2a 100644 --- a/src/catch2/internal/catch_test_case_registry_impl.cpp +++ b/src/catch2/internal/catch_test_case_registry_impl.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -135,7 +136,7 @@ namespace { filtered.push_back(testCase); } } - return filtered; + return createShard(filtered, config.shardCount(), config.shardIndex()); } std::vector const& getAllTestCasesSorted( IConfig const& config ) { return getRegistryHub().getTestCaseRegistry().getAllTestsSorted( config ); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7f7c32fe7e..9a46f6e249 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -87,6 +87,7 @@ set(TEST_SOURCES ${SELF_TEST_DIR}/IntrospectiveTests/RandomNumberGeneration.tests.cpp ${SELF_TEST_DIR}/IntrospectiveTests/Reporters.tests.cpp ${SELF_TEST_DIR}/IntrospectiveTests/Tag.tests.cpp + ${SELF_TEST_DIR}/IntrospectiveTests/Sharding.tests.cpp ${SELF_TEST_DIR}/IntrospectiveTests/String.tests.cpp ${SELF_TEST_DIR}/IntrospectiveTests/StringManip.tests.cpp ${SELF_TEST_DIR}/IntrospectiveTests/Xml.tests.cpp @@ -310,6 +311,7 @@ set_tests_properties(TagAlias PROPERTIES add_test(NAME RandomTestOrdering COMMAND ${PYTHON_EXECUTABLE} ${CATCH_DIR}/tests/TestScripts/testRandomOrder.py $) + add_test(NAME CheckConvenienceHeaders COMMAND ${PYTHON_EXECUTABLE} ${CATCH_DIR}/tools/scripts/checkConvenienceHeaders.py diff --git a/tests/ExtraTests/CMakeLists.txt b/tests/ExtraTests/CMakeLists.txt index bce10df5b2..f2add6d6a0 100644 --- a/tests/ExtraTests/CMakeLists.txt +++ b/tests/ExtraTests/CMakeLists.txt @@ -8,6 +8,22 @@ project( Catch2ExtraTests LANGUAGES CXX ) message( STATUS "Extra tests included" ) + +add_test( + NAME TestShardingIntegration + COMMAND ${PYTHON_EXECUTABLE} ${CATCH_DIR}/tests/TestScripts/testSharding.py $ +) + +add_test( + NAME TestSharding::OverlyLargeShardIndex + COMMAND $ --shard-index 5 --shard-count 5 +) +set_tests_properties( + TestSharding::OverlyLargeShardIndex + PROPERTIES + PASS_REGULAR_EXPRESSION "The shard count \\(5\\) must be greater than the shard index \\(5\\)" +) + # The MinDuration reporting tests do not need separate compilation, but # they have non-trivial execution time, so they are categorized as # extra tests, so that they are run less. diff --git a/tests/SelfTest/Baselines/compact.sw.approved.txt b/tests/SelfTest/Baselines/compact.sw.approved.txt index dfb08230f6..3f39422ebc 100644 --- a/tests/SelfTest/Baselines/compact.sw.approved.txt +++ b/tests/SelfTest/Baselines/compact.sw.approved.txt @@ -1273,6 +1273,12 @@ CmdLine.tests.cpp:: passed: cli.parse({ "test", "--benchmark-no-ana CmdLine.tests.cpp:: passed: config.benchmarkNoAnalysis for: true CmdLine.tests.cpp:: passed: cli.parse({ "test", "--benchmark-warmup-time=10" }) for: {?} CmdLine.tests.cpp:: passed: config.benchmarkWarmupTime == 10 for: 10 == 10 +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: cli.parse({ "test", "--shard-index=2"}) for: {?} +CmdLine.tests.cpp:: passed: config.shardIndex == 2 for: 2 == 2 +CmdLine.tests.cpp:: passed: !result for: true +CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) for: "The shard count must be greater than 0" contains: "The shard count must be greater than 0" Misc.tests.cpp:: passed: std::tuple_size::value >= 1 for: 3 >= 1 Misc.tests.cpp:: passed: std::tuple_size::value >= 1 for: 2 >= 1 Misc.tests.cpp:: passed: std::tuple_size::value >= 1 for: 1 >= 1 diff --git a/tests/SelfTest/Baselines/console.std.approved.txt b/tests/SelfTest/Baselines/console.std.approved.txt index f7320bd70e..af7fb09cea 100644 --- a/tests/SelfTest/Baselines/console.std.approved.txt +++ b/tests/SelfTest/Baselines/console.std.approved.txt @@ -1427,5 +1427,5 @@ due to unexpected exception with message: =============================================================================== test cases: 373 | 296 passed | 70 failed | 7 failed as expected -assertions: 2115 | 1959 passed | 129 failed | 27 failed as expected +assertions: 2121 | 1965 passed | 129 failed | 27 failed as expected diff --git a/tests/SelfTest/Baselines/console.sw.approved.txt b/tests/SelfTest/Baselines/console.sw.approved.txt index 695a30efe9..189944cb0e 100644 --- a/tests/SelfTest/Baselines/console.sw.approved.txt +++ b/tests/SelfTest/Baselines/console.sw.approved.txt @@ -9390,6 +9390,61 @@ CmdLine.tests.cpp:: PASSED: with expansion: 10 == 10 +------------------------------------------------------------------------------- +Process can be configured on command line + Sharding options + shard-count +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK( cli.parse({ "test", "--shard-count=8"}) ) +with expansion: + {?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE( config.shardCount == 8 ) +with expansion: + 8 == 8 + +------------------------------------------------------------------------------- +Process can be configured on command line + Sharding options + shard-index +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK( cli.parse({ "test", "--shard-index=2"}) ) +with expansion: + {?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE( config.shardIndex == 2 ) +with expansion: + 2 == 2 + +------------------------------------------------------------------------------- +Process can be configured on command line + Sharding options + Zero shard-count +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK( !result ) +with expansion: + true + +CmdLine.tests.cpp:: PASSED: + CHECK_THAT( result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) ) +with expansion: + "The shard count must be greater than 0" contains: "The shard count must be + greater than 0" + ------------------------------------------------------------------------------- Product with differing arities - std::tuple ------------------------------------------------------------------------------- @@ -17040,5 +17095,5 @@ Misc.tests.cpp:: PASSED: =============================================================================== test cases: 373 | 280 passed | 86 failed | 7 failed as expected -assertions: 2132 | 1959 passed | 146 failed | 27 failed as expected +assertions: 2138 | 1965 passed | 146 failed | 27 failed as expected diff --git a/tests/SelfTest/Baselines/junit.sw.approved.txt b/tests/SelfTest/Baselines/junit.sw.approved.txt index c2486391ed..ab55d93c1e 100644 --- a/tests/SelfTest/Baselines/junit.sw.approved.txt +++ b/tests/SelfTest/Baselines/junit.sw.approved.txt @@ -1,7 +1,7 @@ - + @@ -1096,6 +1096,9 @@ Message.tests.cpp: + + + diff --git a/tests/SelfTest/Baselines/sonarqube.sw.approved.txt b/tests/SelfTest/Baselines/sonarqube.sw.approved.txt index 18bca50717..4f5e0b12d0 100644 --- a/tests/SelfTest/Baselines/sonarqube.sw.approved.txt +++ b/tests/SelfTest/Baselines/sonarqube.sw.approved.txt @@ -75,6 +75,9 @@ + + + diff --git a/tests/SelfTest/Baselines/tap.sw.approved.txt b/tests/SelfTest/Baselines/tap.sw.approved.txt index aad246f575..e5247b5f7c 100644 --- a/tests/SelfTest/Baselines/tap.sw.approved.txt +++ b/tests/SelfTest/Baselines/tap.sw.approved.txt @@ -2468,6 +2468,18 @@ ok {test-number} - config.benchmarkNoAnalysis for: true ok {test-number} - cli.parse({ "test", "--benchmark-warmup-time=10" }) for: {?} # Process can be configured on command line ok {test-number} - config.benchmarkWarmupTime == 10 for: 10 == 10 +# Process can be configured on command line +ok {test-number} - cli.parse({ "test", "--shard-count=8"}) for: {?} +# Process can be configured on command line +ok {test-number} - config.shardCount == 8 for: 8 == 8 +# Process can be configured on command line +ok {test-number} - cli.parse({ "test", "--shard-index=2"}) for: {?} +# Process can be configured on command line +ok {test-number} - config.shardIndex == 2 for: 2 == 2 +# Process can be configured on command line +ok {test-number} - !result for: true +# Process can be configured on command line +ok {test-number} - result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) for: "The shard count must be greater than 0" contains: "The shard count must be greater than 0" # Product with differing arities - std::tuple ok {test-number} - std::tuple_size::value >= 1 for: 3 >= 1 # Product with differing arities - std::tuple @@ -4266,5 +4278,5 @@ ok {test-number} - q3 == 23. for: 23.0 == 23.0 ok {test-number} - # xmlentitycheck ok {test-number} - -1..2132 +1..2138 diff --git a/tests/SelfTest/Baselines/xml.sw.approved.txt b/tests/SelfTest/Baselines/xml.sw.approved.txt index 419941dee6..256e43fd99 100644 --- a/tests/SelfTest/Baselines/xml.sw.approved.txt +++ b/tests/SelfTest/Baselines/xml.sw.approved.txt @@ -11469,6 +11469,72 @@ Nor would this +
+
+ + + cli.parse({ "test", "--shard-count=8"}) + + + {?} + + + + + config.shardCount == 8 + + + 8 == 8 + + + +
+ +
+
+
+ + + cli.parse({ "test", "--shard-index=2"}) + + + {?} + + + + + config.shardIndex == 2 + + + 2 == 2 + + + +
+ +
+
+
+ + + !result + + + true + + + + + result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) + + + "The shard count must be greater than 0" contains: "The shard count must be greater than 0" + + + +
+ +
@@ -20030,6 +20096,6 @@ loose text artifact - + diff --git a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp index c1fd652729..5b58bdec14 100644 --- a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp +++ b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp @@ -569,6 +569,27 @@ 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"})); + + REQUIRE(config.shardCount == 8); + } + + SECTION("shard-index") { + CHECK(cli.parse({ "test", "--shard-index=2"})); + + REQUIRE(config.shardIndex == 2); + } + + SECTION("Zero shard-count") { + auto result = cli.parse({ "test", "--shard-count=0"}); + + CHECK( !result ); + CHECK_THAT( result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) ); + } + } } TEST_CASE("Test with special, characters \"in name", "[cli][regression]") { diff --git a/tests/SelfTest/IntrospectiveTests/Sharding.tests.cpp b/tests/SelfTest/IntrospectiveTests/Sharding.tests.cpp new file mode 100644 index 0000000000..9ef80d2311 --- /dev/null +++ b/tests/SelfTest/IntrospectiveTests/Sharding.tests.cpp @@ -0,0 +1,41 @@ +/* + * Distributed under the Boost Software License, Version 1.0. (See accompanying + * file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) + */ +#include +#include + +#include + +#include +#include + +TEST_CASE("Sharding Function", "[approvals]") { + std::vector testContainer = { 0, 1, 2, 3, 4, 5, 6 }; + std::unordered_map> expectedShardSizes = { + {1, {7}}, + {2, {4, 3}}, + {3, {3, 2, 2}}, + {4, {2, 2, 2, 1}}, + {5, {2, 2, 1, 1, 1}}, + {6, {2, 1, 1, 1, 1, 1}}, + {7, {1, 1, 1, 1, 1, 1, 1}}, + }; + + auto shardCount = GENERATE(range(1, 7)); + auto shardIndex = GENERATE_COPY(filter([=](int i) { return i < shardCount; }, range(0, 6))); + + std::vector result = Catch::createShard(testContainer, shardCount, shardIndex); + + auto& sizes = expectedShardSizes[shardCount]; + REQUIRE(result.size() == sizes[shardIndex]); + + std::size_t startIndex = 0; + for(int i = 0; i < shardIndex; i++) { + startIndex += sizes[i]; + } + + for(std::size_t i = 0; i < sizes[shardIndex]; i++) { + CHECK(result[i] == testContainer[i + startIndex]); + } +} \ No newline at end of file diff --git a/tests/TestScripts/testSharding.py b/tests/TestScripts/testSharding.py new file mode 100644 index 0000000000..fcea03e2e1 --- /dev/null +++ b/tests/TestScripts/testSharding.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python3 + +""" +This test script verifies that sharding tests does change which tests are run. +This is done by running the binary multiple times, once to list all the tests, +once per shard to list the tests for that shard, and once again per shard to +execute the tests. The sharded lists are compared to the full list to ensure +none are skipped, duplicated, and that the order remains the same. This process +is repeated for multiple command line argument combinations to ensure sharding +works with different filters and test orderings. +""" + +import subprocess +import sys +import xml.etree.ElementTree as ET + +from collections import namedtuple + +def make_base_commandline(self_test_exe): + return [ + self_test_exe, + '--reporter', 'xml', + "--shard-count", "5", + "--shard-index", "2", + "[generators]~[benchmarks]~[.]" + ] + +def list_tests(self_test_exe): + cmd = make_base_commandline(self_test_exe) + ['--list-tests'] + + process = subprocess.Popen( + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + stdout, stderr = process.communicate() + if stderr: + raise RuntimeError("Unexpected error output:\n" + process.stderr) + + root = ET.fromstring(stdout) + result = [elem.text for elem in root.findall('./TestCase/Name')] + + if len(result) < 2: + raise RuntimeError("Unexpectedly few tests listed (got {})".format( + len(result))) + + return result + + +def execute_tests(self_test_exe): + cmd = make_base_commandline(self_test_exe) + + process = subprocess.Popen( + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + stdout, stderr = process.communicate() + if stderr: + raise RuntimeError("Unexpected error output:\n" + process.stderr) + + root = ET.fromstring(stdout) + result = [elem.attrib["name"] for elem in root.findall('./TestCase')] + + if len(result) < 2: + raise RuntimeError("Unexpectedly few tests listed (got {})".format( + len(result))) + return result + + +def check_listed_and_executed_tests_match(listed_tests, executed_tests): + listed_names = set(listed_tests) + executed_names = set(executed_tests) + + listed_string = "\n".join(listed_names) + exeucted_string = "\n".join(executed_names) + + assert listed_names == executed_names, ( + "Executed tests do not match the listed tests:\nExecuted:\n{}\n\nListed:\n{}".format(exeucted_string, listed_string) + ) + + +def test_sharding(self_test_exe): + listed_tests = list_tests(self_test_exe) + executed_tests = execute_tests(self_test_exe) + + check_listed_and_executed_tests_match(listed_tests, executed_tests) + + +def main(): + self_test_exe, = sys.argv[1:] + + test_sharding(self_test_exe) + +if __name__ == '__main__': + sys.exit(main()) From 24a1fed6ff1bb2c60f17f7f776e85a32010a56dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Tue, 26 Oct 2021 23:26:07 +0200 Subject: [PATCH 2/4] Improve shardIndex/Count cli argument parsing --- src/catch2/internal/catch_commandline.cpp | 39 +++- .../Baselines/automake.sw.approved.txt | 1 + .../Baselines/compact.sw.approved.txt | 18 +- .../Baselines/console.std.approved.txt | 4 +- .../Baselines/console.sw.approved.txt | 164 +++++++++------ .../SelfTest/Baselines/junit.sw.approved.txt | 11 +- .../Baselines/sonarqube.sw.approved.txt | 9 +- tests/SelfTest/Baselines/tap.sw.approved.txt | 38 ++-- .../Baselines/teamcity.sw.approved.txt | 2 + tests/SelfTest/Baselines/xml.sw.approved.txt | 187 +++++++++++------- .../IntrospectiveTests/CmdLine.tests.cpp | 54 +++-- 11 files changed, 354 insertions(+), 173 deletions(-) diff --git a/src/catch2/internal/catch_commandline.cpp b/src/catch2/internal/catch_commandline.cpp index 5f8a44d566..6cf71b77a0 100644 --- 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(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(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/Baselines/automake.sw.approved.txt b/tests/SelfTest/Baselines/automake.sw.approved.txt index 0860a79ebb..46945798d0 100644 --- a/tests/SelfTest/Baselines/automake.sw.approved.txt +++ b/tests/SelfTest/Baselines/automake.sw.approved.txt @@ -177,6 +177,7 @@ Nor would this :test-result: FAIL Output from all sections is reported :test-result: PASS Overloaded comma or address-of operators are not used :test-result: PASS Parse test names and tags +:test-result: PASS Parsing sharding-related cli flags :test-result: PASS Pointers can be compared to null :test-result: PASS Precision of floating point stringification can be set :test-result: PASS Predicate matcher can accept const char* diff --git a/tests/SelfTest/Baselines/compact.sw.approved.txt b/tests/SelfTest/Baselines/compact.sw.approved.txt index 3f39422ebc..b28f9ac1e9 100644 --- a/tests/SelfTest/Baselines/compact.sw.approved.txt +++ b/tests/SelfTest/Baselines/compact.sw.approved.txt @@ -1178,6 +1178,18 @@ CmdLine.tests.cpp:: passed: !(spec.matches(*fakeTestCase("hidden an CmdLine.tests.cpp:: passed: !(spec.matches(*fakeTestCase("only foo", "[foo]"))) for: !false CmdLine.tests.cpp:: passed: !(spec.matches(*fakeTestCase("only hidden", "[.]"))) for: !false CmdLine.tests.cpp:: passed: spec.matches(*fakeTestCase("neither foo nor hidden", "[bar]")) for: true +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) 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: 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: cli.parse({ "test", "--shard-index=0" }) for: {?} +CmdLine.tests.cpp:: passed: config.shardIndex == 0 for: 0 == 0 Condition.tests.cpp:: passed: p == 0 for: 0 == 0 Condition.tests.cpp:: passed: p == pNULL for: 0 == 0 Condition.tests.cpp:: passed: p != 0 for: 0x != 0 @@ -1273,12 +1285,6 @@ CmdLine.tests.cpp:: passed: cli.parse({ "test", "--benchmark-no-ana CmdLine.tests.cpp:: passed: config.benchmarkNoAnalysis for: true CmdLine.tests.cpp:: passed: cli.parse({ "test", "--benchmark-warmup-time=10" }) for: {?} CmdLine.tests.cpp:: passed: config.benchmarkWarmupTime == 10 for: 10 == 10 -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: cli.parse({ "test", "--shard-index=2"}) for: {?} -CmdLine.tests.cpp:: passed: config.shardIndex == 2 for: 2 == 2 -CmdLine.tests.cpp:: passed: !result for: true -CmdLine.tests.cpp:: passed: result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) for: "The shard count must be greater than 0" contains: "The shard count must be greater than 0" Misc.tests.cpp:: passed: std::tuple_size::value >= 1 for: 3 >= 1 Misc.tests.cpp:: passed: std::tuple_size::value >= 1 for: 2 >= 1 Misc.tests.cpp:: passed: std::tuple_size::value >= 1 for: 1 >= 1 diff --git a/tests/SelfTest/Baselines/console.std.approved.txt b/tests/SelfTest/Baselines/console.std.approved.txt index af7fb09cea..f513b37e94 100644 --- a/tests/SelfTest/Baselines/console.std.approved.txt +++ b/tests/SelfTest/Baselines/console.std.approved.txt @@ -1426,6 +1426,6 @@ due to unexpected exception with message: Why would you throw a std::string? =============================================================================== -test cases: 373 | 296 passed | 70 failed | 7 failed as expected -assertions: 2121 | 1965 passed | 129 failed | 27 failed as expected +test cases: 374 | 297 passed | 70 failed | 7 failed as expected +assertions: 2127 | 1971 passed | 129 failed | 27 failed as expected diff --git a/tests/SelfTest/Baselines/console.sw.approved.txt b/tests/SelfTest/Baselines/console.sw.approved.txt index 189944cb0e..e9d9d9a726 100644 --- a/tests/SelfTest/Baselines/console.sw.approved.txt +++ b/tests/SelfTest/Baselines/console.sw.approved.txt @@ -8602,6 +8602,111 @@ CmdLine.tests.cpp:: PASSED: with expansion: true +------------------------------------------------------------------------------- +Parsing sharding-related cli flags + shard-count +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK( cli.parse({ "test", "--shard-count=8" }) ) +with expansion: + {?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE( config.shardCount == 8 ) +with expansion: + 8 == 8 + +------------------------------------------------------------------------------- +Parsing sharding-related cli flags + Negative shard count reports error +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK_FALSE( result ) +with expansion: + !{?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard count must be a positive number") ) +with expansion: + "Shard count must be a positive number" contains: "Shard count must be a + positive number" + +------------------------------------------------------------------------------- +Parsing sharding-related cli flags + Zero shard count reports error +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK_FALSE( result ) +with expansion: + !{?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard count must be a positive number") ) +with expansion: + "Shard count must be a positive number" contains: "Shard count must be a + positive number" + +------------------------------------------------------------------------------- +Parsing sharding-related cli flags + shard-index +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK( cli.parse({ "test", "--shard-index=2" }) ) +with expansion: + {?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE( config.shardIndex == 2 ) +with expansion: + 2 == 2 + +------------------------------------------------------------------------------- +Parsing sharding-related cli flags + Negative shard index reports error +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK_FALSE( result ) +with expansion: + !{?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE_THAT( result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") ) +with expansion: + "Shard index must be a non-negative number" contains: "Shard index must be a + non-negative number" + +------------------------------------------------------------------------------- +Parsing sharding-related cli flags + Shard index 0 is accepted +------------------------------------------------------------------------------- +CmdLine.tests.cpp: +............................................................................... + +CmdLine.tests.cpp:: PASSED: + CHECK( cli.parse({ "test", "--shard-index=0" }) ) +with expansion: + {?} + +CmdLine.tests.cpp:: PASSED: + REQUIRE( config.shardIndex == 0 ) +with expansion: + 0 == 0 + ------------------------------------------------------------------------------- Pointers can be compared to null ------------------------------------------------------------------------------- @@ -9390,61 +9495,6 @@ CmdLine.tests.cpp:: PASSED: with expansion: 10 == 10 -------------------------------------------------------------------------------- -Process can be configured on command line - Sharding options - shard-count -------------------------------------------------------------------------------- -CmdLine.tests.cpp: -............................................................................... - -CmdLine.tests.cpp:: PASSED: - CHECK( cli.parse({ "test", "--shard-count=8"}) ) -with expansion: - {?} - -CmdLine.tests.cpp:: PASSED: - REQUIRE( config.shardCount == 8 ) -with expansion: - 8 == 8 - -------------------------------------------------------------------------------- -Process can be configured on command line - Sharding options - shard-index -------------------------------------------------------------------------------- -CmdLine.tests.cpp: -............................................................................... - -CmdLine.tests.cpp:: PASSED: - CHECK( cli.parse({ "test", "--shard-index=2"}) ) -with expansion: - {?} - -CmdLine.tests.cpp:: PASSED: - REQUIRE( config.shardIndex == 2 ) -with expansion: - 2 == 2 - -------------------------------------------------------------------------------- -Process can be configured on command line - Sharding options - Zero shard-count -------------------------------------------------------------------------------- -CmdLine.tests.cpp: -............................................................................... - -CmdLine.tests.cpp:: PASSED: - CHECK( !result ) -with expansion: - true - -CmdLine.tests.cpp:: PASSED: - CHECK_THAT( result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) ) -with expansion: - "The shard count must be greater than 0" contains: "The shard count must be - greater than 0" - ------------------------------------------------------------------------------- Product with differing arities - std::tuple ------------------------------------------------------------------------------- @@ -17094,6 +17144,6 @@ Misc.tests.cpp: Misc.tests.cpp:: PASSED: =============================================================================== -test cases: 373 | 280 passed | 86 failed | 7 failed as expected -assertions: 2138 | 1965 passed | 146 failed | 27 failed as expected +test cases: 374 | 281 passed | 86 failed | 7 failed as expected +assertions: 2144 | 1971 passed | 146 failed | 27 failed as expected diff --git a/tests/SelfTest/Baselines/junit.sw.approved.txt b/tests/SelfTest/Baselines/junit.sw.approved.txt index ab55d93c1e..21e3dbd205 100644 --- a/tests/SelfTest/Baselines/junit.sw.approved.txt +++ b/tests/SelfTest/Baselines/junit.sw.approved.txt @@ -1,7 +1,7 @@ - + @@ -1060,6 +1060,12 @@ Message.tests.cpp: + + + + + + @@ -1096,9 +1102,6 @@ Message.tests.cpp: - - - diff --git a/tests/SelfTest/Baselines/sonarqube.sw.approved.txt b/tests/SelfTest/Baselines/sonarqube.sw.approved.txt index 4f5e0b12d0..e83a3532ae 100644 --- a/tests/SelfTest/Baselines/sonarqube.sw.approved.txt +++ b/tests/SelfTest/Baselines/sonarqube.sw.approved.txt @@ -43,6 +43,12 @@ + + + + + + @@ -75,9 +81,6 @@ - - - diff --git a/tests/SelfTest/Baselines/tap.sw.approved.txt b/tests/SelfTest/Baselines/tap.sw.approved.txt index e5247b5f7c..b306f5cb65 100644 --- a/tests/SelfTest/Baselines/tap.sw.approved.txt +++ b/tests/SelfTest/Baselines/tap.sw.approved.txt @@ -2278,6 +2278,30 @@ ok {test-number} - !(spec.matches(*fakeTestCase("only foo", "[foo]"))) for: !fal ok {test-number} - !(spec.matches(*fakeTestCase("only hidden", "[.]"))) for: !false # Parse test names and tags ok {test-number} - spec.matches(*fakeTestCase("neither foo nor hidden", "[bar]")) for: true +# Parsing sharding-related cli flags +ok {test-number} - cli.parse({ "test", "--shard-count=8" }) for: {?} +# Parsing sharding-related cli flags +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" +# 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" +# Parsing sharding-related cli flags +ok {test-number} - cli.parse({ "test", "--shard-index=2" }) for: {?} +# Parsing sharding-related cli flags +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" +# Parsing sharding-related cli flags +ok {test-number} - cli.parse({ "test", "--shard-index=0" }) for: {?} +# Parsing sharding-related cli flags +ok {test-number} - config.shardIndex == 0 for: 0 == 0 # Pointers can be compared to null ok {test-number} - p == 0 for: 0 == 0 # Pointers can be compared to null @@ -2468,18 +2492,6 @@ ok {test-number} - config.benchmarkNoAnalysis for: true ok {test-number} - cli.parse({ "test", "--benchmark-warmup-time=10" }) for: {?} # Process can be configured on command line ok {test-number} - config.benchmarkWarmupTime == 10 for: 10 == 10 -# Process can be configured on command line -ok {test-number} - cli.parse({ "test", "--shard-count=8"}) for: {?} -# Process can be configured on command line -ok {test-number} - config.shardCount == 8 for: 8 == 8 -# Process can be configured on command line -ok {test-number} - cli.parse({ "test", "--shard-index=2"}) for: {?} -# Process can be configured on command line -ok {test-number} - config.shardIndex == 2 for: 2 == 2 -# Process can be configured on command line -ok {test-number} - !result for: true -# Process can be configured on command line -ok {test-number} - result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) for: "The shard count must be greater than 0" contains: "The shard count must be greater than 0" # Product with differing arities - std::tuple ok {test-number} - std::tuple_size::value >= 1 for: 3 >= 1 # Product with differing arities - std::tuple @@ -4278,5 +4290,5 @@ ok {test-number} - q3 == 23. for: 23.0 == 23.0 ok {test-number} - # xmlentitycheck ok {test-number} - -1..2138 +1..2144 diff --git a/tests/SelfTest/Baselines/teamcity.sw.approved.txt b/tests/SelfTest/Baselines/teamcity.sw.approved.txt index ef70e14c18..6b3336d663 100644 --- a/tests/SelfTest/Baselines/teamcity.sw.approved.txt +++ b/tests/SelfTest/Baselines/teamcity.sw.approved.txt @@ -459,6 +459,8 @@ Message.tests.cpp:|nexplicit failure with message:|n "Message from ##teamcity[testFinished name='Overloaded comma or address-of operators are not used' duration="{duration}"] ##teamcity[testStarted name='Parse test names and tags'] ##teamcity[testFinished name='Parse test names and tags' duration="{duration}"] +##teamcity[testStarted name='Parsing sharding-related cli flags'] +##teamcity[testFinished name='Parsing sharding-related cli flags' duration="{duration}"] ##teamcity[testStarted name='Pointers can be compared to null'] ##teamcity[testFinished name='Pointers can be compared to null' duration="{duration}"] ##teamcity[testStarted name='Precision of floating point stringification can be set'] diff --git a/tests/SelfTest/Baselines/xml.sw.approved.txt b/tests/SelfTest/Baselines/xml.sw.approved.txt index 256e43fd99..071b3649b8 100644 --- a/tests/SelfTest/Baselines/xml.sw.approved.txt +++ b/tests/SelfTest/Baselines/xml.sw.approved.txt @@ -10474,6 +10474,123 @@ Nor would this + +
+ + + cli.parse({ "test", "--shard-count=8" }) + + + {?} + + + + + config.shardCount == 8 + + + 8 == 8 + + + +
+
+ + + !(result) + + + !{?} + + + + + result.errorMessage(), ContainsSubstring("Shard count must be a positive number") + + + "Shard count must be a positive number" contains: "Shard count must be a positive number" + + + +
+
+ + + !(result) + + + !{?} + + + + + result.errorMessage(), ContainsSubstring("Shard count must be a positive number") + + + "Shard count must be a positive number" contains: "Shard count must be a positive number" + + + +
+
+ + + cli.parse({ "test", "--shard-index=2" }) + + + {?} + + + + + config.shardIndex == 2 + + + 2 == 2 + + + +
+
+ + + !(result) + + + !{?} + + + + + result.errorMessage(), ContainsSubstring("Shard index must be a non-negative number") + + + "Shard index must be a non-negative number" contains: "Shard index must be a non-negative number" + + + +
+
+ + + cli.parse({ "test", "--shard-index=0" }) + + + {?} + + + + + config.shardIndex == 0 + + + 0 == 0 + + + +
+ + @@ -11469,72 +11586,6 @@ Nor would this -
-
- - - cli.parse({ "test", "--shard-count=8"}) - - - {?} - - - - - config.shardCount == 8 - - - 8 == 8 - - - -
- -
-
-
- - - cli.parse({ "test", "--shard-index=2"}) - - - {?} - - - - - config.shardIndex == 2 - - - 2 == 2 - - - -
- -
-
-
- - - !result - - - true - - - - - result.errorMessage(), ContainsSubstring( "The shard count must be greater than 0" ) - - - "The shard count must be greater than 0" contains: "The shard count must be greater than 0" - - - -
- -
@@ -20096,6 +20147,6 @@ loose text artifact - - + + diff --git a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp index 5b58bdec14..332cbe5048 100644 --- a/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp +++ b/tests/SelfTest/IntrospectiveTests/CmdLine.tests.cpp @@ -569,27 +569,53 @@ 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")); + } + + SECTION("Shard index 0 is accepted") { + CHECK(cli.parse({ "test", "--shard-index=0" })); + + REQUIRE(config.shardIndex == 0); + } + } TEST_CASE("Test with special, characters \"in name", "[cli][regression]") { From bec1a724c7a126777f2c518df5f935cbdd4131e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Wed, 27 Oct 2021 14:26:07 +0200 Subject: [PATCH 3/4] Cleanup the shard integration test script --- tests/TestScripts/testSharding.py | 136 ++++++++++++++++++++++-------- 1 file changed, 102 insertions(+), 34 deletions(-) diff --git a/tests/TestScripts/testSharding.py b/tests/TestScripts/testSharding.py index fcea03e2e1..eb3fad9935 100644 --- a/tests/TestScripts/testSharding.py +++ b/tests/TestScripts/testSharding.py @@ -5,86 +5,154 @@ This is done by running the binary multiple times, once to list all the tests, once per shard to list the tests for that shard, and once again per shard to execute the tests. The sharded lists are compared to the full list to ensure -none are skipped, duplicated, and that the order remains the same. This process -is repeated for multiple command line argument combinations to ensure sharding -works with different filters and test orderings. +none are skipped, duplicated, and that the order remains the same. """ +import random import subprocess import sys import xml.etree.ElementTree as ET from collections import namedtuple +from typing import List, Dict + +seed = random.randint(0, 2 ** 32 - 1) +number_of_shards = 5 + def make_base_commandline(self_test_exe): return [ self_test_exe, '--reporter', 'xml', - "--shard-count", "5", - "--shard-index", "2", + '--order', 'rand', + '--rng-seed', str(seed), "[generators]~[benchmarks]~[.]" ] -def list_tests(self_test_exe): - cmd = make_base_commandline(self_test_exe) + ['--list-tests'] - - process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - stdout, stderr = process.communicate() - if stderr: - raise RuntimeError("Unexpected error output:\n" + process.stderr) - root = ET.fromstring(stdout) +def list_tests(self_test_exe: str, extra_args: List[str] = None): + cmd = make_base_commandline(self_test_exe) + ['--list-tests'] + if extra_args: + cmd.extend(extra_args) + + try: + ret = subprocess.run(cmd, + stdout = subprocess.PIPE, + stderr = subprocess.PIPE, + timeout = 10, + check = True, + universal_newlines = True) + except subprocess.CalledProcessError as ex: + print('Could not list tests:\n{}'.format(ex.stderr)) + + if ret.stderr: + raise RuntimeError("Unexpected error output:\n" + ret.stderr) + + root = ET.fromstring(ret.stdout) result = [elem.text for elem in root.findall('./TestCase/Name')] if len(result) < 2: raise RuntimeError("Unexpectedly few tests listed (got {})".format( len(result))) + return result -def execute_tests(self_test_exe): +def execute_tests(self_test_exe: str, extra_args: List[str] = None): cmd = make_base_commandline(self_test_exe) - - process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - stdout, stderr = process.communicate() - if stderr: + if extra_args: + cmd.extend(extra_args) + + try: + ret = subprocess.run(cmd, + stdout = subprocess.PIPE, + stderr = subprocess.PIPE, + timeout = 10, + check = True, + universal_newlines = True) + except subprocess.CalledProcessError as ex: + print('Could not list tests:\n{}'.format(ex.stderr)) + + if ret.stderr: raise RuntimeError("Unexpected error output:\n" + process.stderr) - root = ET.fromstring(stdout) + root = ET.fromstring(ret.stdout) result = [elem.attrib["name"] for elem in root.findall('./TestCase')] if len(result) < 2: raise RuntimeError("Unexpectedly few tests listed (got {})".format( len(result))) + return result -def check_listed_and_executed_tests_match(listed_tests, executed_tests): - listed_names = set(listed_tests) - executed_names = set(executed_tests) +def test_sharded_listing(self_test_exe: str) -> Dict[int, List[str]]: + """ + Asks the test binary for list of all tests, and also for lists of + tests from shards. + + The combination of shards is then checked whether it corresponds to + the full list of all tests. - listed_string = "\n".join(listed_names) - exeucted_string = "\n".join(executed_names) + Returns the dictionary of shard-index => listed tests for later use. + """ + all_tests = list_tests(self_test_exe) + big_shard_tests = list_tests(self_test_exe, ['--shard-count', '1', '--shard-index', '0']) - assert listed_names == executed_names, ( - "Executed tests do not match the listed tests:\nExecuted:\n{}\n\nListed:\n{}".format(exeucted_string, listed_string) + assert all_tests == big_shard_tests, ( + "No-sharding test list does not match the listing of big shard:\nNo shard:\n{}\n\nWith shard:\n{}\n".format( + '\n'.join(all_tests), + '\n'.join(big_shard_tests) ) + ) + shard_listings = dict() + for shard_idx in range(number_of_shards): + shard_listings[shard_idx] = list_tests(self_test_exe, ['--shard-count', str(number_of_shards), '--shard-index', str(shard_idx)]) -def test_sharding(self_test_exe): - listed_tests = list_tests(self_test_exe) - executed_tests = execute_tests(self_test_exe) + shard_sizes = [len(v) for v in shard_listings.values()] + assert len(all_tests) == sum(shard_sizes) - check_listed_and_executed_tests_match(listed_tests, executed_tests) + # Check that the shards have roughly the right sizes (e.g. we don't + # have all tests in single shard and the others are empty) + differences = [abs(x1 - x2) for x1, x2 in zip(shard_sizes, shard_sizes[1:])] + assert all(diff <= 1 for diff in differences), "A shard has weird size: {}".format(shard_sizes) + + combined_shards = [inner for outer in shard_listings.values() for inner in outer] + assert all_tests == combined_shards, ( + "All tests and combined shards disagree.\nNo shard:\n{}\n\nCombined:\n{}\n\n".format( + '\n'.join(all_tests), + '\n'.join(combined_shards) + ) + ) + shard_listings[-1] = all_tests + + return shard_listings + + +def test_sharded_execution(self_test_exe: str, listings: Dict[int, List[str]]): + """ + Runs the test binary and checks that the executed tests match the + previously listed tests. + + Also does this for various shard indices, and that the combination + of all shards matches the full run/listing. + """ + all_tests = execute_tests(self_test_exe) + big_shard_tests = execute_tests(self_test_exe, ['--shard-count', '1', '--shard-index', '0']) + assert all_tests == big_shard_tests + + assert listings[-1] == all_tests + + for shard_idx in range(number_of_shards): + assert listings[shard_idx] == execute_tests(self_test_exe, ['--shard-count', str(number_of_shards), '--shard-index', str(shard_idx)]) def main(): self_test_exe, = sys.argv[1:] - - test_sharding(self_test_exe) + listings = test_sharded_listing(self_test_exe) + test_sharded_execution(self_test_exe, listings) if __name__ == '__main__': sys.exit(main()) From 6ac20a06574b6ede6169fe38a726268dc851dfab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Wed, 27 Oct 2021 14:51:45 +0200 Subject: [PATCH 4/4] Make testSharding.py test script executable --- tests/TestScripts/testSharding.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tests/TestScripts/testSharding.py diff --git a/tests/TestScripts/testSharding.py b/tests/TestScripts/testSharding.py old mode 100644 new mode 100755