bazel/test: use RP_FIXTURE_ENV=1 for all unit tests #24989
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bashtanov
force-pushed
the
bazel-tests-rp-fixture-env
branch
from
10b596e to
86c6c42
Compare
michael-redpanda
previously approved these changes
Jan 31, 2025
There was a problem hiding this comment.
Thanks @bashtanov ! cc'ing @dotnwat and @rockwotj to make sure they're kosher with this global change.
CI test resultstest results on build#61432
test results on build#61450
test results on build#61610
|
rockwotj
previously approved these changes
Jan 31, 2025
src/v/crypto/ossl_context_service.cc
Outdated
| @@ -364,7 +364,8 @@ is_fips_mode ossl_context_service::fips_mode() const { | |||
| } | |||
|
|
|||
| bool ossl_context_service::in_rp_fixture_test() const { | |||
| return ::getenv("RP_FIXTURE_ENV") != nullptr; | |||
| char* val = ::getenv("RP_FIXTURE_ENV"); | |||
| return val != nullptr && *val != '\0'; | |||
There was a problem hiding this comment.
nit I would rather check for a known explicit value here (1 yes or anything really) instead of just empty vs non empty
bashtanov
dismissed stale reviews from rockwotj and michael-redpanda
via
ae1d90b
bashtanov
force-pushed
the
bazel-tests-rp-fixture-env
branch
from
86c6c42 to
ae1d90b
Compare
rockwotj
previously approved these changes
Jan 31, 2025
dotnwat
reviewed
Jan 31, 2025
| @@ -168,7 +168,7 @@ redpanda_cc_gtest( | |||
| cpu = 2, | |||
| data = common_data_deps, | |||
| defines = ["FIPS_MODULE_REQUIRED"], | |||
| env = common_env, | |||
| env = common_env | {"RP_FIXTURE_ENV": ""}, | |||
There was a problem hiding this comment.
just discussion: this is a bit awkward, right? like there is some aspect of fixture tests (whatever is testing in_rp_fixture_test()) that should drive the name of this environment variable, rather than it being a "fixture test"?
There was a problem hiding this comment.
Yeah, personally I am not a fan of this method to toggle behavior deep in our crypto libraries. In terms of naming yeah this was just blanket added to every fixture test in CMake, and the crypto library does different stuff based on the variable
There was a problem hiding this comment.
Yes it becomes a misnomer this way. It'll probably be easier to rename it when cmake/ctest goes.
|
test failure unrelated, https://redpandadata.atlassian.net/browse/CORE-8990 created |
|
/dt |
1 similar comment
|
/dt |
We do not distinguish betwee unit and fixture tests in bazel, but fixture tests need RP_FIXTURE_ENV=1 to make adjustments for running multiple shards on the same thread. Add it for all tests by default, but allow individual tests override it.
That's for testing harness to unset it by assigning a different value.
`ossl_context_service` only works if we have one seastar shard per thread. RP_FIXTURE_ENV env variable disables the service to make it safe to run multiple seastar shards in a single thread. A lot of tests need this, so it is set by default. However, to test the service itself we need to enable it.
bashtanov
force-pushed
the
bazel-tests-rp-fixture-env
branch
from
ae1d90b to
38e2dd9
Compare
|
/dt |
rockwotj
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We do not distinguish betwee unit and fixture tests in bazel, but fixture tests need RP_FIXTURE_ENV=1 to make adjustments for running multiple shards on the same thread. Add it for all tests.
Backports Required
Release Notes