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 - take 2 #2832

Merged
merged 11 commits into from
Jan 15, 2025
Merged

Test sharding - take 2 #2832

merged 11 commits into from
Jan 15, 2025

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Jan 14, 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), which is not effected by sharding.

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

@iwahbe iwahbe force-pushed the iwahbe/test_sharding_wip branch 4 times, most recently from 35b8d6f to dfeb591 Compare January 14, 2025 14:36
While github.com/pulumi/sharder should be optimal, github.com/pulumi/shard should approach
optimality as the number of tests grows. I want to see the degradation in test performance
we see when using a simpler solution.
@iwahbe iwahbe force-pushed the iwahbe/test_sharding_wip branch from dfeb591 to 3b8c80d Compare January 14, 2025 14:41
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.48%. Comparing base (f68bff0) to head (a913512).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2832      +/-   ##
==========================================
- Coverage   68.69%   67.48%   -1.21%     
==========================================
  Files         325      325              
  Lines       41621    41621              
==========================================
- Hits        28593    28090     -503     
- Misses      11422    11953     +531     
+ Partials     1606     1578      -28     

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

@iwahbe iwahbe self-assigned this Jan 14, 2025
@iwahbe iwahbe requested a review from a team January 14, 2025 15:52
@iwahbe iwahbe marked this pull request as ready for review January 14, 2025 15:53
@iwahbe iwahbe changed the title WIP Test sharding Test sharding - take 2 Jan 14, 2025
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for checking this again. I'd wrongly assumed the results would be similar to before the recent refactoring of tests.
Simpler is definately better

This test flakes in the new sharded CI. It wasn't run before (`x/` was ignored in
testing), so I'm not worrying about it. It only tests old and unmaintained code at this
point.
@iwahbe iwahbe force-pushed the iwahbe/test_sharding_wip branch from 0fc8d8a to bfec06a Compare January 14, 2025 17:37
This function has caused repeated test failures with concurrent map mutations, the most
recent of which is
https://github.com/pulumi/pulumi-terraform-bridge/actions/runs/12772071950/job/35600764628?pr=2832. This
commit converts the function to return a copy instead of mutating.
@iwahbe iwahbe force-pushed the iwahbe/test_sharding_wip branch from bfec06a to a913512 Compare January 15, 2025 10:48
@iwahbe iwahbe merged commit cb11d40 into master Jan 15, 2025
71 checks passed
@iwahbe iwahbe deleted the iwahbe/test_sharding_wip branch January 15, 2025 11:10
@iwahbe iwahbe mentioned this pull request 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.

2 participants