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

Test sharding #2810

Closed
wants to merge 7 commits into from
Closed

Test sharding #2810

wants to merge 7 commits into from

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Jan 6, 2025

This PR implements a test sharding strategy to speed up the bridge tests from 1 hour 30 minutes to under 15 minutes.

It uses https://github.com/VenelinMartinov/sharder which is a package for generating regexes to plug into go test based on the test profile data. The script is based on the previous work by @blampe in https://github.com/pulumi/shard.

This PR also adds two new make commands:

  • make test_profile this regenerates the test profile data, so that shard partitioning can be more fair
  • make test_shard. This is only run in CI. This command picks up the SHARD_CMD env var to determine which tests to run. SHARD_CMD is populated in CI by the sharder package and contains a regex which should match the correct tests to run.

Sharding is only done for ubuntu tests as they take the longest.

Example run: https://github.com/pulumi/pulumi-terraform-bridge/actions/runs/12656374664

@VenelinMartinov VenelinMartinov marked this pull request as draft January 6, 2025 13:19
@VenelinMartinov VenelinMartinov force-pushed the vvm/test_sharding branch 5 times, most recently from bce5fde to fef033d Compare January 6, 2025 13:54
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.69%. Comparing base (f68bff0) to head (33505e7).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2810      +/-   ##
==========================================
- Coverage   68.69%   68.69%   -0.01%     
==========================================
  Files         325      325              
  Lines       41621    41621              
==========================================
- Hits        28593    28590       -3     
- Misses      11422    11424       +2     
- Partials     1606     1607       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VenelinMartinov VenelinMartinov force-pushed the vvm/test_sharding branch 12 times, most recently from a542264 to 6a4085f Compare January 6, 2025 15:00
@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/refactor_pf_detailed_diff_tests January 7, 2025 14:41
@VenelinMartinov VenelinMartinov force-pushed the vvm/test_sharding branch 3 times, most recently from 80ec996 to 77e23d4 Compare January 13, 2025 13:44
@VenelinMartinov VenelinMartinov force-pushed the vvm/refactor_sdkv2_detailed_diff_tests branch from 4da1a4c to bc0f627 Compare January 13, 2025 15:31
@VenelinMartinov VenelinMartinov force-pushed the vvm/refactor_sdkv2_detailed_diff_tests branch from bc0f627 to a738145 Compare January 14, 2025 09:50
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

If I add new tests, do I need to re-run make test_profile? If so, that means that I need to re-run the entire test suite on my laptop to add a single test (which is a major usability regression). If not, then I don't understand how the test profile gets updated.

github.com/pulumi/sharder's REAMDE is very sparse, which doesn't give me confidence. I'd like to be able to read the README and understand how the tool works, not just a short explanation of what it does. The README should also explain the irreconcilable differences between github.com/pulumi/sharder and github.com/pulumi/shard.

testoutput.txt Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
@VenelinMartinov VenelinMartinov force-pushed the vvm/refactor_sdkv2_detailed_diff_tests branch from a738145 to 98c3684 Compare January 14, 2025 11:23
Base automatically changed from vvm/refactor_sdkv2_detailed_diff_tests to master January 14, 2025 12:37
@VenelinMartinov VenelinMartinov force-pushed the vvm/test_sharding branch 2 times, most recently from 32631d4 to a541316 Compare January 14, 2025 12:58
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jan 14, 2025

@iwahbe I've updated the README on sharder. Let me know if that is now clearer.

If I add new tests, do I need to re-run make test_profile?

No, that is not necessary. I've added a description to the sharder README about it. The TLDR is that the last shard will run all tests not run by any previous ones. This means that if the test timings are not updated the shards might become unbalanced but no tests will be skipped.

In practice, I think this will mean that we'll re-record them once we add a good chunk of tests but don't need to touch that otherwise.

@iwahbe
Copy link
Member

iwahbe commented Jan 14, 2025

I'm testing how much performance we gain with this method vs just using pulumi/shard. If the gain is significant, then I'll approve.

@iwahbe iwahbe mentioned this pull request Jan 14, 2025
iwahbe added a commit that referenced this pull request Jan 15, 2025
Follow-up work on #2810.

This PR aimed to validate the more complex test optimization strategy of
#2810 (pre-calculated perfect allocation; implemented by
github.com/pulumi/sharder) by demonstrating the effects of a simpler
strategy (random allocation; implemented by github.com/pulumi/shard).

Initial testing showed that both strategies were bounded by the 15
minute job [`Test and Lint / test (1.22.x, windows-latest, DEFAULT,
0)`](https://github.com/pulumi/pulumi-terraform-bridge/actions/runs/12770018688/job/35593903461?pr=2832#logs),
which is not effected by sharding.

Since both approaches appear identical, we will merge the simpler
strategy.

---------

Co-authored-by: Venelin <venelin@pulumi.com>
@iwahbe
Copy link
Member

iwahbe commented Jan 15, 2025

Closing in favor of #2832, which was stacked on top of this PR.

@iwahbe iwahbe closed this Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants