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

Add attribute to run specific tests in an isolated process #508

Closed
1 of 3 tasks
ghost opened this issue Apr 12, 2022 · 6 comments
Closed
1 of 3 tasks

Add attribute to run specific tests in an isolated process #508

ghost opened this issue Apr 12, 2022 · 6 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@ghost
Copy link

ghost commented Apr 12, 2022

Proposal

Assuming that#[deprecated_safe] is accepted and applied to env::set_var/env::remove_var, there will need to be some way for unit tests to be made sound if they call the now unsafe env::set_var/env::remove_var. This can be done by guaranteeing that specific tests are started with only a single thread active, and then each affected test can ensure it meets the safety precondition of env::set_var/env::remove_var that no other threads could be reading or writing the environment.

To guarantee specific tests are started with only a single thread active, a new attribute is added that can be applied to #[test] items. This will cause each marked test to be run in its own isolated process, with a single thread guaranteed to be active when the #[test] item is entered. A new process is needed for each test, rather than running marked tests one after another in a single thread, as a prior test may have spun up background threads.

Proof of concept implementation that piggybacks off the functionality already present for -Zpanic-abort-tests.

Some additional motivations from the zulip stream, unrelated to env::set_var/env::remove_var:

Concerns from zulip:

Alternatives from zulip:

Mentors or Reviewers

If you have a reviewer or mentor in mind for this work, mention then
here. You can put your own name here if you are planning to mentor the
work.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@ghost ghost added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Apr 12, 2022
@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 12, 2022
@nikomatsakis
Copy link
Contributor

@rustbot label +I-lang-nominated

Nominating for @rust-lang/lang discussion--this too feels like a language change to me.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 21, 2022
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting, and we were generally fine with adding this. We'd like to have a lang FCP before this is stabilized. We don't see any reason to block experimentation in this area.

@pnkfelix
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label May 10, 2022
@pnkfelix
Copy link
Member

pnkfelix commented May 10, 2022

(I'm seconding to try to push for us to move forward on adding this as an unstable feature. I assume that both T-compiler and T-lang will be included in an FCP as part of eventual stabilization on rust-lang/rust itself.)

@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels May 23, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 27, 2022
MaeIsBad added a commit to primait/veil that referenced this issue Sep 22, 2022
This is neccessary because rust runs all tests in the same process,
leading to tests failing randomly, because of other tests modifying the
global state. There is some proposal on the rust compiler-team github
repo but until that gets stabilized we have to use this hack instead
rust-lang/compiler-team#508
WilliamVenner added a commit to primait/veil that referenced this issue Sep 26, 2022
* Revert "Add redaction control based on environment variables (#7)"

Due to the way cargo works .veil.toml can't impact other crates
consistently.

This reverts commit 69dd762.

* Remove all mentions of the environment aware feature

* Add a set_debug_format function

This is a simpler replacement for the environmental awareness feature.

* Mention being able to skip redacting data

* Move tests that disable redaction into a separate crate

This is neccessary because rust runs all tests in the same process,
leading to tests failing randomly, because of other tests modifying the
global state. There is some proposal on the rust compiler-team github
repo but until that gets stabilized we have to use this hack instead
rust-lang/compiler-team#508

* Loosen the version requirement of once_cell to 1.0.0

Co-authored-by: William <[email protected]>

* Hide the disable feature behind a feature flag

Ensure that the use of this feature requires a very explicit opt-in from
the user.

* Add support for disabling veil with VEIL_DISABLE_REDACTION

* Test the toggle feature with drone

* Minor fixes to disable-redaction-test

* Rename it to veil-tests-disable-redaction
* Set publish = false
* Change the outer line doc comment describing the test into a inner
  line doc comment

* Improve the disable veil redaction docs

This avoids repeating that the disable function needs the toggle feature
flag which makes it clearer that the VEIL_DISABLE_REDACTION envar also
needs the feature flag

* Clarify RedactionBehavior doc comment

Co-authored-by: William <[email protected]>

* Add missing space in README.md

* Update README.md

Make it clear that to skip redacting data the user needs to either call
the disable function *or* set the VEIL_DISABLE_REDACTION variable, not
both.

* Update outdated comment in the disable_redaction example

Co-authored-by: mae.kasza <[email protected]>
Co-authored-by: William <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants