-
Notifications
You must be signed in to change notification settings - Fork 5
docs: require unit tests in *_tests.rs files #18
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
Changes from all commits
848d52a
48d1339
4f12fb9
580f74c
935362d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,15 +48,17 @@ placeholder. | |
| Each feature area is a directory module under a crate's `src/`. A module root | ||
| explains the module, wires its pieces together, and exposes the smallest useful | ||
| API. Move substantial type definitions into `types.rs` and put module-local | ||
| unit tests in a dedicated `test.rs`, wired from the bottom of the module root | ||
| unit tests in a sibling `<module>_tests.rs`, wired from the bottom of the module root | ||
| with: | ||
|
|
||
| ```rust | ||
| #[cfg(test)] | ||
| mod test; | ||
| #[path = "mod_tests.rs"] | ||
| mod tests; | ||
| ``` | ||
|
|
||
| Do not accumulate inline `mod tests` blocks in implementation files, and do not | ||
| Do not write inline `mod tests` blocks in implementation files, do not name a test | ||
| file `test.rs`, `tests.rs` or `<module>_test.rs`, and do not | ||
| let a general-purpose `utils.rs` or `helpers.rs` grow — those are a symptom of a | ||
| missing module. Prefer many small modules that each do one thing well over few | ||
| broad ones. | ||
|
|
@@ -186,7 +188,7 @@ new module capability requires more. | |
|
|
||
| ## Testing | ||
|
|
||
| - Module-local unit tests live in `crates/<crate>/src/<feature>/test.rs` and may | ||
| - Module-local unit tests live in `crates/<crate>/src/<feature>/mod_tests.rs` and may | ||
| touch private items. | ||
| - `unwrap_used`, `expect_used`, and `panic` are denied in **test** targets too. | ||
| Return `Result` from a test and use `?` for paths that should succeed; assert | ||
|
|
@@ -218,7 +220,7 @@ Write documentation for the reader who has never seen the code. | |
|
|
||
| - Every public item gets a rustdoc comment. `missing_docs` is a warning that CI | ||
| treats as an error. | ||
| - Start every `mod.rs` and `test.rs` with a concise module-level `//!` | ||
| - Start every `mod.rs` and `*_tests.rs` with a concise module-level `//!` | ||
| description. | ||
| - `src/lib.rs` carries the crate-level overview: what the crate does, the | ||
| primary entry points, and a short runnable example. | ||
|
|
@@ -307,3 +309,28 @@ For automated contributors specifically: | |
| credentials, and never paste them into a pull request or issue. | ||
| 7. **Ask only when blocked.** Make routine judgment calls yourself; escalate | ||
| only irreversible decisions or genuine forks with no clear default. | ||
|
|
||
| ## Tests live in `*_tests.rs` files | ||
|
|
||
| - Unit tests are never inline. Do not write a `#[cfg(test)] mod tests { ... }` | ||
| block in a source file. Put the tests in a sibling `<module>_tests.rs` | ||
| (`mod_tests.rs` beside a `mod.rs`, `lib_tests.rs` beside `lib.rs`) and declare | ||
| it at the bottom of the module: | ||
|
|
||
| ```rust | ||
| #[cfg(test)] | ||
| #[path = "foo_tests.rs"] | ||
| mod tests; | ||
| ``` | ||
|
|
||
| - The test file starts with `use super::*;` and carries no `#[cfg(test)]` of its | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This requires every test file to begin with AGENTS.md reference: AGENTS.md:L221-L224 Useful? React with 👍 / 👎. |
||
| own. It is still a child module, so it reaches private items exactly as an | ||
| inline module did. | ||
| - Name test files `<module>_tests.rs`; a second group for the same module is | ||
| `<module>_<topic>_tests.rs`. Never `test.rs`, `tests.rs` or `<module>_test.rs`. | ||
| - Integration tests stay in the crate's `tests/` directory. | ||
| - OpenHuman's `scripts/externalize-inline-tests.mjs <repo-root> --write` moves | ||
| inline test modules out mechanically; without `--write` it only reports. | ||
| - Existing `test.rs` and `<module>_test.rs` files predate this rule. Rename each | ||
| to `<module>_tests.rs` (keep its `mod` name, add the `#[path]` attribute) the | ||
| next time you touch it. | ||
|
Comment on lines
+334
to
+336
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This migration rule says an existing AGENTS.md reference: AGENTS.md:L334-L336 Useful? React with 👍 / 👎. |
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -89,7 +89,7 @@ async fn a_box_can_be_created_used_and_removed() -> Result<()> { | |||||
| let removed = invoke(dir.path(), &["rm", "box-0"]).await; | ||||||
| assert_eq!(removed.code, 0); | ||||||
|
|
||||||
| assert!(invoke(dir.path(), &["ls"]).await.out.is_empty()); | ||||||
| assert_eq!(invoke(dir.path(), &["ls"]).await.out.len(), 0); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert to Replacing
Suggested change
[RULE] clippy-lint-regression · There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename This PR adds a rule to [RULE] test-file-naming · |
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -197,7 +197,7 @@ async fn run_creates_uses_and_destroys_a_box_in_one_step() -> Result<()> { | |||||
| assert_eq!(executed.code, 0); | ||||||
| assert_eq!(executed.out.trim(), "once"); | ||||||
| // Nothing is left behind. | ||||||
| assert!(invoke(dir.path(), &["ls"]).await.out.is_empty()); | ||||||
| assert_eq!(invoke(dir.path(), &["ls"]).await.out.len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -209,7 +209,7 @@ async fn run_leaves_nothing_behind_when_the_command_fails() -> Result<()> { | |||||
|
|
||||||
| assert_eq!(executed.code, 3); | ||||||
| // A failing command must not leak a box; the cleanup is unconditional. | ||||||
| assert!(invoke(dir.path(), &["ls"]).await.out.is_empty()); | ||||||
| assert_eq!(invoke(dir.path(), &["ls"]).await.out.len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -225,7 +225,7 @@ async fn run_reports_a_command_that_cannot_start_and_still_cleans_up() -> Result | |||||
|
|
||||||
| assert_eq!(executed.code, EXIT_TINYBOX_ERROR); | ||||||
| assert!(executed.err.contains("error:")); | ||||||
| assert!(invoke(dir.path(), &["ls"]).await.out.is_empty()); | ||||||
| assert_eq!(invoke(dir.path(), &["ls"]).await.out.len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -313,7 +313,7 @@ async fn a_usage_error_reports_clap_s_exit_code() -> Result<()> { | |||||
| let outcome = invoke(dir.path(), &["not-a-command"]).await; | ||||||
|
|
||||||
| assert_eq!(outcome.code, 2); | ||||||
| assert!(!outcome.err.is_empty()); | ||||||
| assert_ne!(outcome.err.len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -343,8 +343,8 @@ async fn requested_help_goes_to_stdout_and_usage_errors_to_stderr() -> Result<() | |||||
| // A mistake is a diagnostic, and stays on stderr. | ||||||
| let misuse = invoke(dir.path(), &["not-a-command"]).await; | ||||||
| assert_eq!(misuse.code, 2); | ||||||
| assert!(misuse.out.is_empty()); | ||||||
| assert!(!misuse.err.is_empty()); | ||||||
| assert_eq!(misuse.out.len(), 0); | ||||||
| assert_ne!(misuse.err.len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -848,11 +848,12 @@ async fn a_one_shot_docker_run_leaves_nothing_behind() -> Result<()> { | |||||
|
|
||||||
| assert_eq!(executed.code, 0); | ||||||
| assert_eq!(executed.out.trim(), "once"); | ||||||
| assert!( | ||||||
| assert_eq!( | ||||||
| invoke_scripted(dir.path(), host.clone(), &["ls"]) | ||||||
| .await | ||||||
| .out | ||||||
| .is_empty() | ||||||
| .len(), | ||||||
| 0 | ||||||
| ); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
@@ -932,7 +933,7 @@ async fn an_ssh_destination_that_would_be_read_as_an_option_is_refused() -> Resu | |||||
|
|
||||||
| assert_eq!(outcome.code, EXIT_TINYBOX_ERROR); | ||||||
| assert!(outcome.err.contains("ssh destination")); | ||||||
| assert!(host.commands().is_empty()); | ||||||
| assert_eq!(host.commands().len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -1151,7 +1152,7 @@ async fn templates_can_be_listed_and_forgotten() -> Result<()> { | |||||
| assert!(listed.out.contains("sha-9f2c0e1b7a4d")); | ||||||
|
|
||||||
| assert_eq!(invoke(dir.path(), &["template", "rm", "ci"]).await.code, 0); | ||||||
| assert!(invoke(dir.path(), &["template", "ls"]).await.out.is_empty()); | ||||||
| assert_eq!(invoke(dir.path(), &["template", "ls"]).await.out.len(), 0); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path states that all module-local tests live under
<feature>/mod_tests.rs, which only describes directory modules. The repository also has file modules such asjail.rswith siblingjail_tests.rs, and the new detailed section explicitly prescribes that layout, so contributors working on file modules receive conflicting instructions; describe bothmod.rsandfoo.rslayouts here.AGENTS.md reference: AGENTS.md:L315-L317
Useful? React with 👍 / 👎.