Repository: rust
Issue: Tracking Issue for ui test suite cleanups and maintenance
Labels: C-cleanup,A-testsuite,T-compiler,E-medium,C-tracking-issue,E-tedious,S-tracking-forever

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.

## Context

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

- Uninformative test names[^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.
- 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](https://rustc-dev-guide.rust-lang.org/tests/best-practices.html#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:

```rs
//! 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

- #133996
- #133900
- #134024
- #134418

[^test-names]: not *always* problematic! Requires discretion.