-
Notifications
You must be signed in to change notification settings - Fork 533
Add test sharding support for rust_test #3774
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
base: main
Are you sure you want to change the base?
Conversation
illicitonion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me (though I don't have enough batch skills to really verify the .bat file). One thought on provider organisation :)
rust/private/rust.bzl
Outdated
|
|
||
| # Get the test binary from CrateInfo - it's the output of the compiled test | ||
| crate_info_provider = None | ||
| for p in providers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do something like edit rustc_compile_action to return a dict of provider types to providers, and then do a key lookup, rather than sniffing an ordered list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better - done.
f6ec6ee to
a7f30a5
Compare
47f7781 to
c23d0dc
Compare
c23d0dc to
adb3e34
Compare
|
Remaining test breakages appear to be pre-existing issues unrelated to this change. The windows .bat file was a pain to test, but was able to get it to work eventually. I also have a branch where I use a small rust util to launch the tests - lemme know if we should explore that either here or as a followup. |
|
I would love to have the util be a rust wrapper. Seems like that would avoid multiple implementations and a bash dependency |
UebelAndre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once upon a time we had a test wrapper and it ended up being problematic for debuggers. I'm very nervous about introducing this in this way. Is there not a test crate rules_rust could provide that would enable sharding instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for cross-platform consistency, could this be a rust binary?
| E.g. `bazel test //src:rust_test --test_arg=foo::test::test_fn`. | ||
| """), | ||
| ), | ||
| "experimental_enable_sharding": attr.bool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be a build flag similar to these?
rules_rust/rust/settings/BUILD.bazel
Lines 85 to 93 in cdaf15f
| experimental_link_std_dylib() | |
| experimental_per_crate_rustc_flag() | |
| experimental_use_cc_common_link() | |
| experimental_use_coverage_metadata_files() | |
| experimental_use_global_allocator() |
Problem
rust_testtargets don't support Bazel's native test sharding. Whenshard_countis set on arust_test:--incompatible_check_sharding_support: all tests run N times (once per shard)--incompatible_check_sharding_support: the test fails because Rust's libtest harness doesn't readTEST_SHARD_INDEX/TEST_TOTAL_SHARDSenvironment variablesSolution
This PR adds opt-in sharding support via a new
experimental_enable_shardingattribute. When enabled, tests are executed via a wrapper script that:TEST_TOTAL_SHARDSenvironment variableTEST_SHARD_STATUS_FILEto advertise sharding support to Bazel--list --format terseflagindex % TEST_TOTAL_SHARDS == TEST_SHARD_INDEX--exactUsage