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 reasons to all ignored tests. #10929

Merged
merged 1 commit into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 48 additions & 14 deletions crates/cargo-test-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,17 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream {
// but they don't really handle the absence of files well.
let mut ignore = false;
let mut requires_reason = false;
let mut found_reason = false;
let mut explicit_reason = None;
let mut implicit_reasons = Vec::new();
macro_rules! set_ignore {
($predicate:expr, $($arg:tt)*) => {
let p = $predicate;
ignore |= p;
if p {
implicit_reasons.push(std::fmt::format(format_args!($($arg)*)));
}
};
}
let is_not_nightly = !version().1;
for rule in split_rules(attr) {
match rule.as_str() {
Expand All @@ -29,57 +39,81 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream {
// explicit opt-in (these generally only work on linux, and
// have some extra requirements, and are slow, and can pollute
// the environment since it downloads dependencies).
ignore |= is_not_nightly;
ignore |= option_env!("CARGO_RUN_BUILD_STD_TESTS").is_none();
set_ignore!(is_not_nightly, "requires nightly");
set_ignore!(
option_env!("CARGO_RUN_BUILD_STD_TESTS").is_none(),
"CARGO_RUN_BUILD_STD_TESTS must be set"
);
}
"build_std_mock" => {
// Only run the "mock" build-std tests on nightly and disable
// for windows-gnu which is missing object files (see
// https://github.com/rust-lang/wg-cargo-std-aware/issues/46).
ignore |= is_not_nightly;
ignore |= cfg!(all(target_os = "windows", target_env = "gnu"));
set_ignore!(is_not_nightly, "requires nightly");
set_ignore!(
cfg!(all(target_os = "windows", target_env = "gnu")),
"does not work on windows-gnu"
);
}
"nightly" => {
requires_reason = true;
ignore |= is_not_nightly;
set_ignore!(is_not_nightly, "requires nightly");
}
s if s.starts_with("requires_") => {
let command = &s[9..];
ignore |= !has_command(command);
set_ignore!(!has_command(command), "{command} not installed");
}
s if s.starts_with(">=1.") => {
requires_reason = true;
let min_minor = s[4..].parse().unwrap();
ignore |= version().0 < min_minor;
let minor = version().0;
set_ignore!(minor < min_minor, "requires rustc 1.{minor} or newer");
}
s if s.starts_with("reason=") => {
found_reason = true;
explicit_reason = Some(s[7..].parse().unwrap());
}
_ => panic!("unknown rule {:?}", rule),
}
}
if requires_reason && !found_reason {
if requires_reason && explicit_reason.is_none() {
panic!(
"#[cargo_test] with a rule also requires a reason, \
such as #[cargo_test(nightly, reason = \"needs -Z unstable-thing\")]"
);
}
Comment on lines +78 to 83
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add ignore to #[cargo_test] so that it would require a reason?

#[cargo_test(ignore, reason = "broken, need artifact info in index")]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but it wouldn't prevent someone from adding #[ignore] and would also make it tricky to write conditional ignores, like:

#[cfg_attr(
    any(target_env = "msvc", target_vendor = "apple"),
    ignore = "--out-dir and examples are currently broken on MSVC and apple"
)]

I wouldn't want to complicate the macro too much, or to have multiple ways of ignoring (#[cargo_test(ignore)] and #[ignore]).

For the most part, these ignore reasons are just a convenience. I don't see it critical that they get printed 100% of the time.

I think in my ideal world, tests could be ignored from within the test using code instead of using attributes, which are not well suited for complex conditions.


// Construct the appropriate attributes.
let span = Span::call_site();
let mut ret = TokenStream::new();
let add_attr = |ret: &mut TokenStream, attr_name| {
let add_attr = |ret: &mut TokenStream, attr_name, attr_input| {
ret.extend(Some(TokenTree::from(Punct::new('#', Spacing::Alone))));
let attr = TokenTree::from(Ident::new(attr_name, span));
let mut attr_stream: TokenStream = attr.into();
if let Some(input) = attr_input {
attr_stream.extend(input);
}
ret.extend(Some(TokenTree::from(Group::new(
Delimiter::Bracket,
attr.into(),
attr_stream,
))));
};
add_attr(&mut ret, "test");
add_attr(&mut ret, "test", None);
if ignore {
add_attr(&mut ret, "ignore");
let reason = explicit_reason
.or_else(|| {
(!implicit_reasons.is_empty())
.then(|| TokenTree::from(Literal::string(&implicit_reasons.join(", "))).into())
})
.map(|reason: TokenStream| {
let mut stream = TokenStream::new();
stream.extend(Some(TokenTree::from(Punct::new('=', Spacing::Alone))));
stream.extend(Some(reason));
stream
});
add_attr(&mut ret, "ignore", reason);
}

// Find where the function body starts, and add the boilerplate at the start.
for token in item {
let group = match token {
TokenTree::Group(g) => {
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/advanced_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use cargo_test_support::{paths, project, registry::Package};
#[cargo_test]
// I don't know why, but `Command` forces all env keys to be upper case on
// Windows. Seems questionable, since I think Windows is case-preserving.
#[cfg_attr(windows, ignore)]
#[cfg_attr(windows, ignore = "broken due to not preserving case on Windows")]
fn source_config_env() {
// Try to define [source] with environment variables.
let p = project()
Expand Down
5 changes: 3 additions & 2 deletions tests/testsuite/artifact_dep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1331,7 +1331,8 @@ fn build_script_deps_adopts_target_platform_if_target_equals_target() {
}

#[cargo_test]
#[cfg_attr(target_env = "msvc", ignore)] // TODO(ST): rename bar (dependency) to something else and un-ignore this with RFC-3176
// TODO(ST): rename bar (dependency) to something else and un-ignore this with RFC-3176
#[cfg_attr(target_env = "msvc", ignore = "msvc not working")]
fn profile_override_basic() {
let p = project()
.file(
Expand Down Expand Up @@ -1450,7 +1451,7 @@ foo v0.0.0 ([CWD])
// For reference, see comments by ehuss https://github.com/rust-lang/cargo/pull/9992#discussion_r801086315 and
// joshtriplett https://github.com/rust-lang/cargo/pull/9992#issuecomment-1033394197 .
#[cargo_test]
#[ignore]
#[ignore = "broken, need artifact info in index"]
fn targets_are_picked_up_from_non_workspace_artifact_deps() {
if cross_compile::disabled() {
return;
Expand Down
6 changes: 4 additions & 2 deletions tests/testsuite/build_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4236,8 +4236,10 @@ fn rename_with_link_search_path() {
}

#[cargo_test]
// Don't have a cdylib cross target on macos.
#[cfg_attr(target_os = "macos", ignore)]
#[cfg_attr(
target_os = "macos",
ignore = "don't have a cdylib cross target on macos"
)]
fn rename_with_link_search_path_cross() {
if cross_compile::disabled() {
return;
Expand Down
6 changes: 4 additions & 2 deletions tests/testsuite/collisions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,11 @@ This may become a hard error in the future; see <https://github.com/rust-lang/ca
}

#[cargo_test]
// --out-dir and examples are currently broken on MSVC and apple.
// See https://github.com/rust-lang/cargo/issues/7493
#[cfg_attr(any(target_env = "msvc", target_vendor = "apple"), ignore)]
#[cfg_attr(
any(target_env = "msvc", target_vendor = "apple"),
ignore = "--out-dir and examples are currently broken on MSVC and apple"
)]
fn collision_export() {
// `--out-dir` combines some things which can cause conflicts.
let p = project()
Expand Down
6 changes: 4 additions & 2 deletions tests/testsuite/cross_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1206,8 +1206,10 @@ fn platform_specific_variables_reflected_in_build_scripts() {
}

#[cargo_test]
// Don't have a dylib cross target on macos.
#[cfg_attr(target_os = "macos", ignore)]
#[cfg_attr(
target_os = "macos",
ignore = "don't have a dylib cross target on macos"
)]
fn cross_test_dylib() {
if cross_compile::disabled() {
return;
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2424,7 +2424,7 @@ fn scrape_examples_avoid_build_script_cycle() {
// The example is calling a function from a proc-macro, but proc-macros don't
// export functions. It is not clear what this test is trying to exercise.
// #[cargo_test(nightly, reason = "rustdoc scrape examples flags are unstable")]
#[ignore]
#[ignore = "broken, needs fixing"]
#[cargo_test]
fn scrape_examples_complex_reverse_dependencies() {
let p = project()
Expand Down
6 changes: 3 additions & 3 deletions tests/testsuite/old_cargos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ fn default_toolchain_is_stable() -> bool {
// The optional dependency `new-baz-dep` should not be activated.
// * `bar` 1.0.2 has a dependency on `baz` that *requires* the new feature
// syntax.
#[ignore]
#[ignore = "must be run manually, requires old cargo installations"]
#[cargo_test]
fn new_features() {
let registry = registry::init();
Expand Down Expand Up @@ -534,7 +534,7 @@ fn new_features() {
}

#[cargo_test]
#[ignore]
#[ignore = "must be run manually, requires old cargo installations"]
fn index_cache_rebuild() {
// Checks that the index cache gets rebuilt.
//
Expand Down Expand Up @@ -618,7 +618,7 @@ foo v0.1.0 [..]
}

#[cargo_test]
#[ignore]
#[ignore = "must be run manually, requires old cargo installations"]
fn avoids_split_debuginfo_collision() {
// Test needs two different toolchains.
// If the default toolchain is stable, then it won't work.
Expand Down
5 changes: 4 additions & 1 deletion tests/testsuite/profile_custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ Caused by:
}

#[cargo_test]
#[ignore] // dir-name is currently disabled
// We are currently uncertain if dir-name will ever be exposed to the user.
// The code for it still roughly exists, but only for the internal profiles.
// This test was kept in case we ever want to enable support for it again.
#[ignore = "dir-name is disabled"]
fn invalid_dir_name() {
let p = project()
.file(
Expand Down
3 changes: 1 addition & 2 deletions tests/testsuite/rustdoc_extern_html.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ fn simple() {
assert!(myfun.contains(r#"href="https://docs.rs/bar/1.0.0/bar/struct.Straw.html""#));
}

// Broken, temporarily disable until https://github.com/rust-lang/rust/pull/82776 is resolved.
#[ignore]
#[ignore = "Broken, temporarily disabled until https://github.com/rust-lang/rust/pull/82776 is resolved."]
#[cargo_test]
// #[cargo_test(nightly, reason = "--extern-html-root-url is unstable")]
fn std_docs() {
Expand Down