From 3f4ad44082abe7256825f6fc692c1455a1eb28bb Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 14 Jun 2021 22:55:05 +0300 Subject: internal: document that we don't #[ignore] tests --- crates/hir_def/src/nameres/collector.rs | 21 +++++++----- crates/hir_ty/src/tests/coercion.rs | 35 +++++++++++++++----- crates/hir_ty/src/tests/traits.rs | 1 - crates/ide/src/doc_links.rs | 18 ++++++---- crates/ide/src/hover.rs | 5 +-- crates/ide_assists/src/handlers/fix_visibility.rs | 32 +++++++++--------- .../ide_diagnostics/src/handlers/incorrect_case.rs | 38 +++++----------------- docs/dev/style.md | 7 ++++ 8 files changed, 86 insertions(+), 71 deletions(-) diff --git a/crates/hir_def/src/nameres/collector.rs b/crates/hir_def/src/nameres/collector.rs index 6fab58f15..4ae02e576 100644 --- a/crates/hir_def/src/nameres/collector.rs +++ b/crates/hir_def/src/nameres/collector.rs @@ -1992,8 +1992,8 @@ mod tests { collector.def_map } - fn do_resolve(code: &str) -> DefMap { - let (db, _file_id) = TestDB::with_single_file(code); + fn do_resolve(not_ra_fixture: &str) -> DefMap { + let (db, _file_id) = TestDB::with_single_file(not_ra_fixture); let krate = db.test_crate(); let edition = db.crate_graph()[krate].edition; @@ -2013,16 +2013,21 @@ mod tests { ); } - #[ignore] // this test does succeed, but takes quite a while :/ + #[ignore] #[test] fn test_macro_expand_will_stop_2() { + // FIXME: this test does succeed, but takes quite a while: 90 seconds in + // the release mode. That's why the argument is not an ra_fixture -- + // otherwise injection highlighting gets stuck. + // + // We need to find a way to fail this faster. do_resolve( r#" - macro_rules! foo { - ($($ty:ty)*) => { foo!($($ty)* $($ty)*); } - } - foo!(KABOOM); - "#, +macro_rules! foo { + ($($ty:ty)*) => { foo!($($ty)* $($ty)*); } +} +foo!(KABOOM); +"#, ); } } diff --git a/crates/hir_ty/src/tests/coercion.rs b/crates/hir_ty/src/tests/coercion.rs index 71047703d..4f859fc85 100644 --- a/crates/hir_ty/src/tests/coercion.rs +++ b/crates/hir_ty/src/tests/coercion.rs @@ -741,10 +741,24 @@ fn coerce_unsize_trait_object_simple() { } #[test] -// The rust reference says this should be possible, but rustc doesn't implement -// it. We used to support it, but Chalk doesn't. -#[ignore] fn coerce_unsize_trait_object_to_trait_object() { + // FIXME: The rust reference says this should be possible, but rustc doesn't + // implement it. We used to support it, but Chalk doesn't. Here's the + // correct expect: + // + // 424..609 '{ ...bj2; }': () + // 434..437 'obj': &dyn Baz + // 459..461 '&S': &S + // 460..461 'S': S + // 471..474 'obj': &dyn Bar + // 496..499 'obj': &dyn Baz + // 509..512 'obj': &dyn Foo + // 531..534 'obj': &dyn Bar + // 544..548 'obj2': &dyn Baz + // 570..572 '&S': &S + // 571..572 'S': S + // 582..583 '_': &dyn Foo + // 602..606 'obj2': &dyn Baz check_infer_with_mismatches( r#" #[lang = "sized"] @@ -773,21 +787,24 @@ fn coerce_unsize_trait_object_to_trait_object() { let _: &dyn Foo<_, _> = obj2; } "#, - expect![[r" + expect![[r#" 424..609 '{ ...bj2; }': () 434..437 'obj': &dyn Baz 459..461 '&S': &S 460..461 'S': S - 471..474 'obj': &dyn Bar + 471..474 'obj': &dyn Bar<{unknown}, {unknown}, {unknown}> 496..499 'obj': &dyn Baz - 509..512 'obj': &dyn Foo - 531..534 'obj': &dyn Bar + 509..512 'obj': &dyn Foo<{unknown}, {unknown}> + 531..534 'obj': &dyn Bar<{unknown}, {unknown}, {unknown}> 544..548 'obj2': &dyn Baz 570..572 '&S': &S 571..572 'S': S - 582..583 '_': &dyn Foo + 582..583 '_': &dyn Foo<{unknown}, {unknown}> 602..606 'obj2': &dyn Baz - "]], + 496..499: expected &dyn Bar<{unknown}, {unknown}, {unknown}>, got &dyn Baz + 531..534: expected &dyn Foo<{unknown}, {unknown}>, got &dyn Bar<{unknown}, {unknown}, {unknown}> + 602..606: expected &dyn Foo<{unknown}, {unknown}>, got &dyn Baz + "#]], ); } diff --git a/crates/hir_ty/src/tests/traits.rs b/crates/hir_ty/src/tests/traits.rs index 6bcede4c4..c830e576e 100644 --- a/crates/hir_ty/src/tests/traits.rs +++ b/crates/hir_ty/src/tests/traits.rs @@ -1475,7 +1475,6 @@ fn test( } #[test] -#[ignore] fn error_bound_chalk() { check_types( r#" diff --git a/crates/ide/src/doc_links.rs b/crates/ide/src/doc_links.rs index 57ae9455b..7ac0118fe 100644 --- a/crates/ide/src/doc_links.rs +++ b/crates/ide/src/doc_links.rs @@ -241,6 +241,10 @@ fn get_doc_link(db: &RootDatabase, definition: Definition) -> Option { Definition::ModuleDef(ModuleDef::Module(module)) => module.krate(), _ => definition.module(db)?.krate(), }; + // FIXME: using import map doesn't make sense here. What we want here is + // canonical path. What import map returns is the shortest path suitable for + // import. See this test: + cov_mark::hit!(test_reexport_order); let import_map = db.import_map(krate.into()); let mut base = krate.display_name(db)?.to_string(); @@ -642,13 +646,15 @@ pub mod foo { ) } - // FIXME: ImportMap will return re-export paths instead of public module - // paths. The correct path to documentation will never be a re-export. - // This problem stops us from resolving stdlib items included in the prelude - // such as `Option::Some` correctly. - #[ignore = "ImportMap may return re-exports"] #[test] fn test_reexport_order() { + cov_mark::check!(test_reexport_order); + // FIXME: This should return + // + // https://docs.rs/test/*/test/wrapper/modulestruct.Item.html + // + // That is, we should point inside the module, rather than at the + // re-export. check( r#" pub mod wrapper { @@ -663,7 +669,7 @@ fn foo() { let bar: wrapper::It$0em; } "#, - expect![[r#"https://docs.rs/test/*/test/wrapper/module/struct.Item.html"#]], + expect![[r#"https://docs.rs/test/*/test/wrapper/struct.Item.html"#]], ) } } diff --git a/crates/ide/src/hover.rs b/crates/ide/src/hover.rs index c08516805..afeded315 100644 --- a/crates/ide/src/hover.rs +++ b/crates/ide/src/hover.rs @@ -1821,9 +1821,10 @@ pub struct B$0ar ); } - #[ignore = "path based links currently only support documentation on ModuleDef items"] #[test] fn test_hover_path_link_field() { + // FIXME: Should be + // [Foo](https://docs.rs/test/*/test/struct.Foo.html) check( r#" pub struct Foo; @@ -1845,7 +1846,7 @@ pub struct Bar { --- - [Foo](https://docs.rs/test/*/test/struct.Foo.html) + [Foo](struct.Foo.html) "#]], ); } diff --git a/crates/ide_assists/src/handlers/fix_visibility.rs b/crates/ide_assists/src/handlers/fix_visibility.rs index 9b432e92f..2f2b605fc 100644 --- a/crates/ide_assists/src/handlers/fix_visibility.rs +++ b/crates/ide_assists/src/handlers/fix_visibility.rs @@ -583,25 +583,25 @@ pub struct Foo { pub(crate) bar: () } } #[test] - #[ignore] - // FIXME handle reexports properly fn fix_visibility_of_reexport() { + // FIXME: broken test, this should fix visibility of the re-export + // rather than the struct. check_assist( fix_visibility, - r" - mod foo { - use bar::Baz; - mod bar { pub(super) struct Baz; } - } - foo::Baz$0 - ", - r" - mod foo { - $0pub(crate) use bar::Baz; - mod bar { pub(super) struct Baz; } - } - foo::Baz - ", + r#" +mod foo { + use bar::Baz; + mod bar { pub(super) struct Baz; } +} +foo::Baz$0 +"#, + r#" +mod foo { + use bar::Baz; + mod bar { $0pub(crate) struct Baz; } +} +foo::Baz +"#, ) } } diff --git a/crates/ide_diagnostics/src/handlers/incorrect_case.rs b/crates/ide_diagnostics/src/handlers/incorrect_case.rs index 72f251961..68f25f284 100644 --- a/crates/ide_diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide_diagnostics/src/handlers/incorrect_case.rs @@ -341,43 +341,27 @@ mod F { } #[test] - #[ignore] - fn bug_trait_inside_fn() { - // FIXME: - // This is broken, and in fact, should not even be looked at by this - // lint in the first place. There's weird stuff going on in the - // collection phase. - // It's currently being brought in by: - // * validate_func on `a` recursing into modules - // * then it finds the trait and then the function while iterating - // through modules - // * then validate_func is called on Dirty - // * ... which then proceeds to look at some unknown module taking no - // attrs from either the impl or the fn a, and then finally to the root - // module - // - // It should find the attribute on the trait, but it *doesn't even see - // the trait* as far as I can tell. - + fn complex_ignore() { + // FIXME: this should trigger errors for the second case. check_diagnostics( r#" trait T { fn a(); } struct U {} impl T for U { fn a() { - // this comes out of bitflags, mostly #[allow(non_snake_case)] - trait __BitFlags { + trait __BitFlagsOk { const HiImAlsoBad: u8 = 2; - #[inline] - fn Dirty(&self) -> bool { - false - } + fn Dirty(&self) -> bool { false } } + trait __BitFlagsBad { + const HiImAlsoBad: u8 = 2; + fn Dirty(&self) -> bool { false } + } } } - "#, +"#, ); } @@ -414,18 +398,14 @@ extern { } #[test] - #[ignore] fn bug_traits_arent_checked() { // FIXME: Traits and functions in traits aren't currently checked by // r-a, even though rustc will complain about them. check_diagnostics( r#" trait BAD_TRAIT { - // ^^^^^^^^^ 💡 weak: Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` fn BAD_FUNCTION(); - // ^^^^^^^^^^^^ 💡 weak: Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function` fn BadFunction(); - // ^^^^^^^^^^^^ 💡 weak: Function `BadFunction` should have snake_case name, e.g. `bad_function` } "#, ); diff --git a/docs/dev/style.md b/docs/dev/style.md index 96dd684b3..84485ea28 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md @@ -174,6 +174,13 @@ Instead, explicitly check for `None`, `Err`, etc. `rust-analyzer` is not a library, we don't need to test for API misuse, and we have to handle any user input without panics. Panic messages in the logs from the `#[should_panic]` tests are confusing. +## `#[ignore]` + +Do not `#[ignore]` tests. +If the test currently does not work, assert the wrong behavior and add a fixme explaining why it is wrong. + +**Rationale:** noticing when the behavior is fixed, making sure that even the wrong behavior is acceptable (ie, not a panic). + ## Function Preconditions Express function preconditions in types and force the caller to provide them (rather than checking in callee): -- cgit v1.2.3