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

restore some config tests #4064

Merged

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Feb 26, 2020

Resolves #2183. I saw these again the other day when I was in the config module and decided to take a closer look at them.

CFG_RELEASE_CHANNEL is now checked at compile time, so the original issue with sequencing of tests that were mutating the shared env vars during test exection is no longer relevant.

macro_rules! is_nightly_channel {
() => {
option_env!("CFG_RELEASE_CHANNEL").map_or(true, |c| c == "nightly" || c == "dev")
};
}

Instead, I split out the tests for nightly vs. non-nightly with their respective assertions accordingly, and the latter set of tests will run as part of one of the CI jobs (ex: https://travis-ci.com/rust-lang/rustfmt/jobs/291200498).

All that being said.. 😄

@topecongiro in #3387 (comment) you mentioned removing unstable_features altogether as part of 2.0, so would you rather I go ahead and just remove the unstable_features config option (and related content like the original commented out tests) altogether?

Happy to do so if that's the plan, though based on the thread in #3387 it seems like users would be interested in having the ability to opt-in to being able to use non-stablized rustfmt config options even on stable (not available today). FWIW I can understand that request, and I believe libtest supports
something similar today:

This will fail (json format is unstable)

$ cargo +stable test -- --format json
    Finished test [unoptimized + debuginfo] target(s) in 0.31s
     Running target/debug/deps/rusty_hook-18bae14627c600cc
error: The "json" format is only accepted on the nightly compiler
error: test failed, to rerun pass '--lib'

but this works

$ cargo +stable test -- -Z unstable-options --format json
    Finished test [unoptimized + debuginfo] target(s) in 0.28s
     Running target/debug/deps/rusty_hook-af9ceb85a6bdee3c
{ "type": "suite", "event": "started", "test_count": 52 }
....
....

let mut config = Config::default();
assert_eq!(config.unstable_features(), false);
config.set().unstable_features(true);
assert_eq!(config.unstable_features(), true);
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't expect this to be the case, but apparently we don't check the stability attribute of config options for set and override_value. That seems to imply that folks can technically already use unstable rustfmt options on stable anyway, though only via command line overrides to rustfmt

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's unfortunate. I will reconsider what we should do about unstable features on stable.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@topecongiro topecongiro merged commit 45ac003 into rust-lang:master Apr 16, 2020
@calebcartwright calebcartwright deleted the unstable-features-tests branch April 17, 2020 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests relying on env vars are unreliable
3 participants