Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow sharded execution for bazel test sharding support #2257

Merged
merged 4 commits into from
Oct 27, 2021

Conversation

ChewyGumball
Copy link
Contributor

@ChewyGumball ChewyGumball commented Jul 11, 2021

Description

This PR adds support for splitting test execution across multiple invocations of the test binary (otherwise known as test sharding). The tests to run are split evenly into the number of groups given by the new Config::shardCount(), and the group at the index given by the new Config::shardIndex() is executed. The intended use is that multiple processes will be run, each with the same shard count, but different shard indices. Sharding works for executing as well as listing tests, and is applied after the list of tests to execute has been created (after filters).

The main motivation is to allow Bazel test sharding to work:

#define CATCH_CONFIG_RUNNER
#include "catch.hpp"

#include <cstdlib>
#include <filesystem>

int main( int argc, char* argv[] )
{
  Catch::Session session;

  int returnCode = session.applyCommandLine( argc, argv );
  if( returnCode != 0 )
        return returnCode;
 
  // writing to session.configData() or session.Config() here 
  // overrides command line args
  // only do this if you know you need to

  char* envShardCount = std::getenv("TEST_TOTAL_SHARDS");
  if (envShardCount != nullptr && session.configData().shardCount != 1) {
        session.configData().shardCount = std::stoul(envShardCount);
  }

  char* envShardIndex = std::getenv("TEST_SHARD_INDEX");
  if (envShardIndex != nullptr && session.configData().shardIndex != 0) {
        session.configData().shardIndex = std::stoul(envShardIndex);
  }

  char* envShardStatusFile = std::getenv("TEST_SHARD_STATUS_FILE");
  if (envShardStatusFile != nullptr) {
      std::filesystem::last_write_time(envShardStatusFile, std::chrono::file_clock::now());
  }

  int numFailed = session.run();
  
  // numFailed is clamped to 255 as some unices only use the lower 8 bits.
  // This clamping has already been applied, so just return it here
  // You can also do any post run clean-up here
  return numFailed;
}

@ChewyGumball ChewyGumball changed the title allow sharded execution for bazel test sharding support Allow sharded execution for bazel test sharding support Jul 12, 2021
@ChewyGumball ChewyGumball marked this pull request as ready for review July 12, 2021 04:53
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #2257 (6ac20a0) into devel (905bf43) will decrease coverage by 0.08%.
The diff coverage is 79.17%.

@@            Coverage Diff             @@
##            devel    #2257      +/-   ##
==========================================
- Coverage   91.06%   90.98%   -0.08%     
==========================================
  Files         150      151       +1     
  Lines        7139     7187      +48     
==========================================
+ Hits         6501     6539      +38     
- Misses        638      648      +10     

Copy link
Member

@horenmar horenmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, didn't have time to go through this sooner.

I like the general idea, but there is a bunch of changes to be done before merging.

docs/command-line.md Show resolved Hide resolved
docs/command-line.md Outdated Show resolved Hide resolved
src/catch2/internal/catch_sharding.hpp Outdated Show resolved Hide resolved
src/catch2/internal/catch_sharding.hpp Outdated Show resolved Hide resolved
src/catch2/internal/catch_sharding.hpp Outdated Show resolved Hide resolved
tests/TestScripts/testSharding.py Outdated Show resolved Hide resolved
tests/TestScripts/testSharding.py Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
src/catch2/internal/catch_sharding.hpp Outdated Show resolved Hide resolved
@ChewyGumball
Copy link
Contributor Author

All great comments. I've tried my best to address them in the latest commit. Would you mind taking another look?

docs/command-line.md Outdated Show resolved Hide resolved
@horenmar
Copy link
Member

horenmar commented Oct 5, 2021

I probably won't get to the tests till tomorrow.

@horenmar
Copy link
Member

horenmar commented Oct 6, 2021

Thanks, it looks pretty good now, but I think you went overboard on reducing the test script.

What I would want to see is:

  • Pick a positive tag (generators is fine)
  • List all tests for that tag without sharding
  • Shard the tests into say, 5 shards, list each shard individually, check that they add up into the whole list, and without overlap.
  • Shard the tests into the same numbers of shards, check that executing each shard executes the tests listed in previous step.

Since the expensive part is calling into the binary, rather than doing extra processing of the data on the Python side, it is possible to also add extra checks along the way, e.g. checking that the binary obeys --order selection when sharding and so on, because that only adds some extra processing in Python.

@horenmar horenmar force-pushed the sharding_support branch 2 times, most recently from a3169e4 to 60442bb Compare October 27, 2021 08:56
ChewyGumball and others added 4 commits October 27, 2021 14:44
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.
@horenmar horenmar merged commit ec2d501 into catchorg:devel Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants