Tracking Issue for ui test suite improvements

[jieyouxu]

This is a tracking issue for an initiative of improving ui test suite organization and ui test usability. This issue is not meant for general discussions, but is instead intended for tracking logistics of PRs. For specific matters, please discuss in the zulip thread https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Discussion.20for.20ui.20test.20suite.20improvements.

Tip

I'm happy to mentor and review work that contributes towards this initiative.

For plain renames/moves to more meaningful subdirectories, it's fine to do a bigger patch since that is easier to review.

For not just plain renames/moves, you could select a few non-descriptive ui tests (see below for criteria) to improve (max ~5-7 tests in a batch) and r? jieyouxu on the PR.

Please don't claim this issue; it's intended to be available to be worked by multiple contributors.

Context

The ui test suite (tests/ui/) has a lot of tests. Often, many ui tests suffer from:

• Uninformative test names¹ when it's not clear that a test is a specific regression test for some edge case of a particular feature, for example.
• Lack of backlinks: links to the issue for regression tests, relevant discussions, relevant context, further resources are very helpful.
  • Yes, you might be able to find via git archaeology, but tests get moved and tests get changed, and it's also at least an extra layer of indirection.
• Lack of test intention documentation: many ui tests don't describe what they intend to check. This makes future work that modify or fail the test very hard and tedious because you have to first figure out what the test is intending to check via git archaeology or asking the authors/reviewers. Furthermore, the test might fail to actually check what it intends to check!
• Lack of docs on how the test plans to check what it intends to check, when this is not trivial.
• Confusing organization: some ui tests are placed randomly, like directly under tests/ui/. Some ui tests fall into multiple categories, which is fine, but it may make sense to rehome an ui test if it's better organized under a different directory. See Tracking issue for moving ui tests to subdirectories #73494.
• Duplicate tests. There are certainly ui tests that are duplicated, but it's a pain to figure out which ones are full "true" duplicates as opposed to only overlapping.

Possible improvements

The guiding rationale for improving ui test usability is to:

1. Make it easier to figure out test intention:
   1. What the test intends to check.
   2. What are the relevant context (issues, PRs, discussions, RFCs) or areas.
2. Make it easier to find tests. E.g. keywords, better directory organizations, better test names.
3. Don't regress existing ui test coverage: for example, if a parser test relies on specific formatting, do not rustfmt the test as it will regress test coverage.

See Best practices for writing tests in rustc-dev-guide for advice on how to make ui tests more useful. However, don't take the advice at face value -- they should be evaluated on a case-by-case basis. For instance, some subdirectory might contain a collection of specific regression tests related to issues, and in that case having tests be named just issue-xxxxx.rs isn't bad. On the contrary, a top-level issue-xxxxx.rs under tests/ui/ is not very informative.

Example of things that might be done, but only if it makes sense on a case-by-case basis:

• Improve test documentation:
  • Briefly describe test intention.
  • Backlink to issue, e.g. //! Issue: <https://github.com/rust-lang/rust/issues/374>. or whatever useful relevant context.
  • If the test checks something that's not obvious in how the check is achieved, elaborate on how the test achieves its purpose.
• Rename the test file to something more informative, e.g. macro-empty-suggestion-span-123456.rs.
• Rehome the test under a more fitting subdirectory, e.g. tests/ui/macro-empty-suggestion-span-123456.rs -> tests/ui/hir-typeck/suggestions/macro-empty-suggestion-span-123456.rs (hypothetical, or some other better organization).
• Reformat the test, but only if the formatting is sufficiently weird, and that the test does not rely on the exact formatting.
• Remove distractions that are not important to the test's purpose: for example, don't use lowercase type names if the test is actually exercising codegen that would be unaffected by lowercase type name, and that only serves as distraction. Be very careful to not change things that would invalidate the test!

Because these require discretion (changes are not always improvements!), this issue is labeled E-medium and not just E-easy. Having "insider" compiler implementation knowledge helps a lot here.

Example test doc comment

No fixed format, adapt as suitable for the test at hand. But an example:

//! Check that `-A warnings` cli flag applies to *all* warnings, including feature gate warnings.
//!
//! This test tries to exercise that by checking that the "empty trait list for derive" warning for
//! `#[derive()]` is permitted by `-A warnings`, which is a non-lint warning.
//!
//! # Relevant context
//!
//! - Original impl PR: <https://github.com/rust-lang/rust/pull/21248>.
//! - RFC 507 "Release channels":
//!   <https://github.com/rust-lang/rfcs/blob/c017755b9bfa0421570d92ba38082302e0f3ad4f/text/0507-release-channels.md>.

Long-term plan

1. Reorganize all the stray tests immediately under tests/ui and place them into suitable subdirectories, improving the tests themselves along the way.
2. Review and audit the immediate subdirectories under tests/ui/, and see if they need to be fusioned/fissioned/renamed or otherwise adjusted. Where suitable, we can also introduce some subdirectory-level README.md to document subdirectory intention/area.
3. Kill off generic tests/ui/issues/ directory and rehome the tests properly.

Implementation history

Note: these are some of the earlier PRs (so not necessarily exhaustive), I may stop updating this listing if we have more PRs. Writing "Part of #133895" will cause the PR to still show-up via backlinks.

• Move most tests for -l and #[link(..)] into tests/ui/link-native-libs #133996
• Advent of tests/ui (misc cleanups and improvements) [1/N] #133900
• Advent of tests/ui (misc cleanups and improvements) [2/N] #134024
• Advent of tests/ui (misc cleanups and improvements) [3/N] #134418
• Advent of tests/ui (misc cleanups and improvements) [4/N] #140036
• Clean up some tests in tests/ui #138471
• Clean UI tests 2 of n #138639
• Clean UI tests 3 of n #139889
• Clean UI tests 4 of n #139995
• Tidying up UI tests [1/N] #140029

Footnotes

1. not always problematic! Requires discretion. ↩