Skip to content

Commit 29f8702

Browse files
jieyouxujieyouxu
committed
Add test best practices chapter
1 parent 96b3faa commit 29f8702

File tree

2 files changed

+189
-0
lines changed

2 files changed

+189
-0
lines changed

‎src/SUMMARY.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
- [Testing with Docker](./tests/docker.md)
2222
- [Testing with CI](./tests/ci.md)
2323
- [Adding new tests](./tests/adding.md)
24+
- [Best practices](./tests/best-practices.md)
2425
- [Compiletest](./tests/compiletest.md)
2526
- [UI tests](./tests/ui.md)
2627
- [Test headers](./tests/headers.md)

‎src/tests/best-practices.md‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
# Best practices for writing tests
2+
3+
This chapter describes best practices related to authoring and modifying tests.
4+
We want to make sure the tests we author are easy to understand and modify, even
5+
several years later, without needing to consult the original author and perform
6+
a bunch of git archeology.
7+
8+
It's good practice to review the test that you authored by pretending that you
9+
are a different contributor who is looking at the test that failed several years
10+
later without much context (this also helps yourself even a few days or months
11+
later!). Then ask yourself: how can I make my life and their lives easier?
12+
13+
To help put this into perspective, let's start with an aside on how to write a
14+
test that makes the life of another contributor as hard as possible.
15+
16+
> **Aside: Simple Test Sabotage Field Manual**
17+
>
18+
> To make the life of another contributor as hard as possible, one might:
19+
>
20+
> - Only name the test after an issue, e.g. `issue-123456.rs`.
21+
> - Have no comments at all on what the test is trying to exercise, no links to
22+
> relevant context.
23+
> - Include a test that is massive (that can otherwise be minimized) and
24+
> contains non-essential pieces which distracts from the core thing the test
25+
> is actually trying to test.
26+
> - Include a bunch of unrelated syntax errors and other errors which are not
27+
> critical to what the test is trying to check.
28+
> - Weirdly format the snippets.
29+
> - Include a bunch of unused and unrelated features.
30+
> - Have e.g. `ignore-windows` [compiletest directives] but don't offer any
31+
> explanation as to *why* they are needed.
32+
33+
## Test naming
34+
35+
Make it easy for the reader to immediately understand what the test is
36+
exercising, instead of having to type in the issue number and dig through github
37+
search for what the test is trying to exercise. This has an additional benefit
38+
of making the test possible to be filtered via `--test-args` as a collection of
39+
related tests.
40+
41+
- Name the test after what it's trying to exercise or prevent regressions of.
42+
- Keep it concise.
43+
- Avoid including issue numbers in test names.
44+
45+
> **Avoid issue numbers in test names**
46+
>
47+
> Prefer including them as links or `#123456` in test comments instead.
48+
>
49+
> ```text
50+
> tests/ui/typeck/issue-123456.rs // bad
51+
> tests/ui/typeck/issue-123456-asm-macro-external-span-ice.rs // bad
52+
> tests/ui/typeck/asm-macro-external-span-ice.rs // good
53+
> ```
54+
>
55+
> `issue-123456.rs` does not tell you immediately anything about what the test
56+
> is actually exercising meaning you need to do additional searching. Including
57+
> the issue number in the test name is really noisy for finding relevant tests
58+
> by what they're exercising (if you `ls` a test directory and get a bunch of
59+
> `issue-xxxxx` prefixes). We can link to the issue in a test comment.
60+
>
61+
> ```rs
62+
> //! Check that `asm!` macro including nested macros that come from external
63+
> //! crates do not lead to a codepoint boundary assertion ICE.
64+
> //!
65+
> //! Regression test for <https://github.com/rust-lang/rust/issues/123456>.
66+
> ```
67+
68+
## Test organization
69+
70+
- For most test suites, try to find a semantically meaningful subdirectory to
71+
home the test.
72+
- E.g. for an implementation of RFC 2093 specifically, we can group a
73+
collection of tests under `tests/ui/rfc-2093-infer-outlives/`. For the
74+
directory name, include what the RFC is about.
75+
- For the [`run-make`] test suite, each `rmake.rs` must be contained within an
76+
immediate subdirectory under `tests/run-make/`. Further nesting is not
77+
presently supported. Avoid including issue number in the directory name too,
78+
include that info in a comment inside `rmake.rs`.
79+
80+
## Test descriptions
81+
82+
To help other contributors understand what the test is about if their changes
83+
lead to the test failing, we should make sure a test has sufficient docs about
84+
its intent/purpose, links to relevant context (incl. issue numbers or other
85+
discussions) and possibly relevant resources (e.g. can be helpful to link to
86+
Win32 APIs for specific behavior).
87+
88+
**Synopsis of a test with good comments**
89+
90+
```rust,ignore
91+
//! Brief summary of what the test is exercising.
92+
//! Example: Regression test for #123456: make sure coverage attribute don't ICE
93+
//! when applied to non-items.
94+
//!
95+
//! Optional: Remarks on related tests/issues, external APIs/tools, crash
96+
//! mechanism, how it's fixed, FIXMEs, limitations, etc.
97+
//! Example: This test is like `tests/attrs/linkage.rs`, but this test is
98+
//! specifically checking `#[coverage]` which exercises a different code
99+
//! path. The ICE was triggered during attribute validation when we tried
100+
//! to construct a `def_path_str` but only emitted the diagnostic when the
101+
//! platform is windows, causing an ICE on unix.
102+
//!
103+
//! Links to relevant issues and discussions. Examples below:
104+
//! Regression test for <https://github.com/rust-lang/rust/issues/123456>.
105+
//! See also <https://github.com/rust-lang/rust/issues/101345>.
106+
//! See discussion at <https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/123456-example-topic>.
107+
//! See [`clone(2)`].
108+
//!
109+
//! [`clone(2)`]: https://man7.org/linux/man-pages/man2/clone.2.html
110+
111+
//@ ignore-windows
112+
// Reason: (why is this test ignored for windows? why not specifically
113+
// windows-gnu or windows-msvc?)
114+
115+
// Optional: Summary of test cases: What positive cases are checked?
116+
// What negative cases are checked? Any specific quirks?
117+
118+
fn main() {
119+
#[coverage]
120+
//~^ ERROR coverage attribute can only be applied to function items.
121+
let _ = {
122+
// Comment highlighting something that deserves reader attention.
123+
fn foo() {}
124+
};
125+
}
126+
```
127+
128+
For how much context/explanation is needed, it is up to the author and
129+
reviewer's discretion. A good rule of thumb is non-trivial things exercised in
130+
the test deserves some explanation to help other contributors to understand.
131+
This may include remarks on:
132+
133+
- How an ICE can get triggered if it's quite elaborate.
134+
- Related issues and tests (e.g. this test is like another test but is kept
135+
separate because...).
136+
- Platform-specific behaviors.
137+
- Behavior of external dependencies and APIs: syscalls, linkers, tools,
138+
environments and the likes.
139+
140+
## Test content
141+
142+
- Try to make sure the test is as minimal as possible.
143+
- Minimize non-critical code and especially minimize unnecessary syntax and type
144+
errors which can clutter stderr snapshots.
145+
- Where possible, use semantically meaningful names (e.g. `fn
146+
bare_coverage_attributes() {}`).
147+
148+
## Flaky tests
149+
150+
All tests need to strive to be reproducible and reliable. Flaky tests are the
151+
worst kind of tests, arguably even worse than not having the test in the first
152+
place.
153+
154+
- Flaky tests can fail in completely unrelated PRs which can confuse other
155+
contributors and waste their time trying to figure out if test failure is
156+
related.
157+
- Flaky tests provide no useful information from its test results other than
158+
it's flaky and not reliable: if a test passed but it's flakey, did I just get
159+
lucky? if a test is flakey but it failed, was it just spurious?
160+
- Flaky tests degrade confidence in the whole test suite. If a test suite can
161+
randomly spuriously fail due to flaky tests, did the whole test suite pass or
162+
did I just get lucky/unlucky?
163+
- Flaky tests can randomly fail in full CI, wasting previous full CI resources.
164+
165+
## Compiletest directives
166+
167+
See [compiletest directives] for a listing of directives.
168+
169+
- For `ignore-*`/`needs-*`/`only-*` directives, unless extremely obvious,
170+
provide a brief remark on why the directive is needed. E.g. `"//@ ignore-wasi
171+
(wasi codegens the main symbol differently)"`.
172+
173+
## FileCheck best practices
174+
175+
See [LLVM FileCheck guide][FileCheck] for details.
176+
177+
- Avoid matching on specific register numbers or basic block numbers unless
178+
they're special or critical for the test. Consider using patterns to match
179+
them where suitable.
180+
181+
> **TODO**
182+
>
183+
> Pending concrete advice.
184+
185+
[compiletest]: ./compiletest.md
186+
[compiletest directives]: ./headers.md
187+
[`run-make`]: ./compiletest.md#run-make-tests
188+
[FileCheck]: https://llvm.org/docs/CommandGuide/FileCheck.html

0 commit comments

Comments
 (0)