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

Fix flaky tests that use std::env::set_var #253

Merged
merged 1 commit into from
Oct 14, 2023

Conversation

a-kenji
Copy link
Contributor

@a-kenji a-kenji commented Sep 28, 2023

Fix flaky tests that use std::env::set_var.

std::env::set_var is not safe to be used in parallel.

Add #[ignore] statements to these tests.
Add an extra test step to ci.sh, as I don't think there is a better way mark certain tests as single threaded only.

Reproducer:

for _ in {0..100}; do
  cargo t current_dir || exit 1
done

Will fail very fast.

Doesn't fail:

for _ in {0..100}; do
  cargo t current_dir_prefers_pwd_env_var || exit 1
  cargo t current_dir_uses_dereferenced_path_when_pwd_env_var_not_set || exit 1
done

Reference: rust-lang/rust#27970

Fix flaky tests that use `std::env::set_var`.

`std::env::set_var` is not safe to be used in parallel.

Reproducer:
``` bash
for _ in {0..100}; do
  cargo t current_dir || exit 1
done
```
Will fail very fast.

Doesn't fail:
```bash
for _ in {0..100}; do
  cargo t current_dir_prefers_pwd_env_var || exit 1
  cargo t current_dir_uses_dereferenced_path_when_pwd_env_var_not_set || exit 1
done
```

Reference: rust-lang/rust#27970
@a-kenji a-kenji changed the title Fix flaky tests that use std::env::set_var. Fix flaky tests that use std::env::set_var Sep 28, 2023
@srounce
Copy link
Member

srounce commented Oct 2, 2023

I'm wondering how either the tests or the implementation could be improved to make it more amenable to being tested normally? The test parallelism problem was a bit of oversight on my part, though it'd be nicer to not resort to workarounds for a less than optimal initial solution and instead have a better solution.

@zimbatm
Copy link
Member

zimbatm commented Oct 7, 2023

Do you have a better idea? Otherwise, I would propose to merge this PR until we find a better solution.

@Mic92
Copy link
Member

Mic92 commented Oct 14, 2023

@bors merge

@Mic92 Mic92 merged commit 554b004 into numtide:main Oct 14, 2023
11 checks passed
@a-kenji a-kenji deleted the fix/flaky/tests branch October 14, 2023 12:33
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.

None yet

4 participants