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

A flag to unset all pre-existing variables #24

Open
tabokie opened this issue Aug 14, 2024 · 6 comments · May be fixed by #29
Open

A flag to unset all pre-existing variables #24

tabokie opened this issue Aug 14, 2024 · 6 comments · May be fixed by #29

Comments

@tabokie
Copy link

tabokie commented Aug 14, 2024

It would make tests more reproducible.

@vmx
Copy link
Owner

vmx commented Aug 14, 2024

That's a good idea. Though I don't think we need a new flag, this can be done already today with something along the lines of:

let vars_to_unset = std::env::vars().map(|(key, _value)| (key, None)).collect();
temp_env::with_vars(
    [
        vars_to_unset,
        vec![("MY_VAR".to_string(), Some("my_value"))],
    ]
    .concat(),
    || {
        println!("vmx: env without pre-existing vars: {:?}", std::env::vars());
    },
);

Does that solve your problem?

@tabokie
Copy link
Author

tabokie commented Aug 24, 2024

Calling std::env::vars() directly will again be interfered by other test cases. E.g. test A unsets some variables, test B collects vars_to_unset missing those variables, then test A finishes and test B enters temp_env.

But I agree it'll work if everyone is purely adding additional variables instead of unsetting some.

@vmx
Copy link
Owner

vmx commented Aug 28, 2024

Calling std::env::vars() directly will again be interfered by other test cases. E.g. test A unsets some variables, test B collects vars_to_unset missing those variables, then test A finishes and test B enters temp_env.

If I understand it correctly, you would set the vars_to_unset once (with a OnceCell` or so) and the other tests would re-use them.

@tabokie
Copy link
Author

tabokie commented Aug 29, 2024

But that would force every tests to run the setup even if they don't necessarily need a clean environment. Any one test missing the setup && unset some variables in temp_env will cause the problem I described earlier.

@fabian-braun
Copy link
Collaborator

I have a draft here. Is that what you had in mind?

I'm a bit hesitant about it, because this will also unset all of these variables and that may cause surprising effects in tests, for example backtraces not working properly within the tests because RUST_BACKTRACE is unset or macros that rely on CARGO_PKG_NAME will suddenly not produce the expected result.

@fabian-braun fabian-braun linked a pull request Dec 7, 2024 that will close this issue
@tabokie
Copy link
Author

tabokie commented Dec 12, 2024

Is that what you had in mind?

Yes it is, appreciate the work!

may cause surprising effects in tests

True, I didn't think about this before. It isn't just the behavior inside temp_env is altered, but all other unrelated tests running at the same time.

I think a better description of what temp_env should provide, is the isolation against all other tests running temp_env. Currently it means we can read variables unmodified by them. But we can't write variables (after the construction of the env).

Say we provide a method called temp_env::set_in_scope(key, value), the desired functionality can be implemented like this:

temp_env::new(
    || {
        for (key, var) in std::env::vars() {
          temp_env::set_in_scope(key, None);
        }
    },
);

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 a pull request may close this issue.

3 participants