Skip to content

Serialize tests that need environment variables#1685

Merged
madsmtm merged 2 commits intomainfrom
global-test-env
Mar 8, 2026
Merged

Serialize tests that need environment variables#1685
madsmtm merged 2 commits intomainfrom
global-test-env

Conversation

@madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Mar 7, 2026

One of the things that's been bugging me about our test suite for a while is that we modify the global environment with std::env::set_var and std::env::remove_var in a very ad-hoc fashion, including doing things like , or calling reset_env() at the top of every test.

Instead, this PR introduces a GlobalEnv struct that takes a global lock, allows setting / removing variables only when that lock is held, and reverts the environment back to what it was once the test is done.

Unfortunately, this does force us to serialize every test, but I don't see any other solution. If we wanted, we could use cargo nextest in CI, see #1402, but let's handle that separately.

This is important for #1682, where I change Build::env, and thus we'll need some other way of consistently setting environment variables in tests, but I also imagine it'll be nice to have for testing things like CC's auto-TARGET sourcing.

@madsmtm madsmtm requested a review from NobodyXu March 7, 2026 17:50
Copy link
Contributor

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, just two nits

Regarding cargo-nextest, yes I think we should definitely open another PR to use it, I might have time to do it

Copy link
Contributor

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

The two nits should not be blocking

@NobodyXu
Copy link
Contributor

NobodyXu commented Mar 8, 2026

Unfortunately, this does force us to serialize every test, but I don't see any other solution. If we wanted, we could use cargo nextest in CI, see #1402, but let's handle that separately.

Opened #1686 for this

@madsmtm madsmtm enabled auto-merge (squash) March 8, 2026 15:12
Co-authored-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>
@madsmtm madsmtm merged commit d41559b into main Mar 8, 2026
79 checks passed
@madsmtm madsmtm deleted the global-test-env branch March 8, 2026 15:19
@ChrisDenton
Copy link
Member

Rather than serialisation, could you cfg(test) an implementation of GlobalEnv that uses a per test key/value store instead of the real environment?

@madsmtm
Copy link
Contributor Author

madsmtm commented Mar 8, 2026

Rather than serialisation, could you cfg(test) an implementation of GlobalEnv that uses a per test key/value store instead of the real environment?

#[cfg(test)] only works for tests inside lib.rs (or submodules), it doesn't work for integration tests. I guess we could move the tests to not be integration tests though.

Generally though, I'd rather have high complexity in tests and low complexity in the implementation (instead of having to support mocking).

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