refactor(rules): a test file leaves the ceiling only if a #[cfg(test)] declares it - #89
Merged
Merged
Conversation
…] declares it The tests are about to move into sibling files, and a sibling is a file: without this, `crates/cp-store/src/store_test.rs` would arrive at 1.617 lines against a 1.500 ceiling and the first file moved would break CI. Excluding them by name would be worse than the problem — anything could dodge the ceiling by calling itself `_test.rs`. So the exemption is the one Tisty uses: a file is skipped when some `#[cfg(test)]` in the tree names it in a `#[path = "..."]`. The declaration earns it, not the name, and the list comes out of the source rather than a pattern. It measures nothing differently today: `--print` gives byte-identical output before and after, because nothing declares a path yet. What it does is make the next commits possible, and it is checked both ways — a 1.601-line sibling behind a declaration passes, the same file with nobody declaring it is refused at both the file and the function ceiling.
…ore, cp-store
35 test modules out of 21 files, 10.117 lines that move without one of them
changing. `store.rs` carried 3.360 lines of test over 1.449 of code; reading it
meant scrolling past a small book.
The pattern is Tisty's, from the task that put a ceiling on its files:
#[cfg(test)]
#[path = "store_test.rs"]
mod tests;
The sibling lives in the same `src/`, without the `mod tests { }` wrapper and
with the body dedented one level. It is still a child module, so `use super::*`
and the private access work exactly as before. A module called `tests` becomes
`<file>_test.rs`; one with a name of its own keeps it — `store_listing.rs`,
`kind_borders.rs`, `kind_inherited_from_2x.rs`.
**That nothing changed is not a claim, it is a comparison.** Every one of the 94
siblings was re-indented and diffed against the body it came from in
`origin/main`: 94 compared, 0 different. Then `cargo fmt` reflows six of them,
because four columns of freed indentation let lines that rustfmt had split fit
on one again — which is why the comparison is run before the formatter, not
after.
Per crate, the count of tests is the arbiter: cp-config 16 → 16, cp-core
301 → 301, cp-store 268 → 268.
**And it answers the question the task asked.** The ceiling was supposed to be
measuring code only; if the numbers moved, it had been measuring wrong. They do
not: `store.rs` reads 1.409 before and 1.409 after, and so does every other file
over 500 lines. The 19-line difference across the whole tree is the six files
rustfmt reflowed.
… and macOS crates 41 test modules out of 41 files, 3.125 lines, same pattern and same siblings as the core's. cp-win 90 → 90 tests and cp-win-sys 84 → 84, run here. The macOS crates cannot be built on this machine, so they get the two checks that do not need a compiler: the 63 `#[test]` functions in cp-mac and cp-mac-sys are still 63, and their 14 siblings were re-indented and diffed against `origin/main` like every other one, with no difference. What is left for CI on macOS is that they compile, which is what the job called "the macOS crates still build" is for. One case needed a name the others did not. `app/src-tauri/src/waking.rs` holds two modules both called `tests`, one behind `#[cfg(all(test, windows))]` and one behind `#[cfg(all(test, target_os = "macos"))]`, which Rust allows because they are mutually exclusive. Two files cannot both be `waking_test.rs`, so when a name repeats in one file the platform goes into both of them — `waking_test_windows.rs` and `waking_test_macos.rs`, rather than leaving the first bare and suffixing only the second.
…e tray app 18 test modules out of 17 files, 1.484 lines, and the last of the 94. cp-panel 68 → 68 tests, cp-gui 26 → 26. With this, no `#[cfg(test)] mod` with a body is left anywhere in `crates` or `app/src-tauri/src`: 94 declarations, 94 siblings, 0 blocks. The whole tree still runs 853 tests, clippy is clean across the workspace with `-D warnings`, rustfmt is conforming, and the ceiling passes with the measured code unchanged. What this buys is that a file can be read: 14.726 lines of test are no longer sitting between the code and the reader, and the counter measures what is in front of it instead of what it deduced by skipping blocks.
…te each other
CI on Windows stopped with `LNK1104: cannot open file
target\debug\examples\twice.exe` while linking cp-mac's example. The file was
open: cp-win was linking its own `twice` into it at that moment.
cargo puts every example at `target/<profile>/examples/<name>`, with no room for
the package it came from, so `cp-mac/examples/probe.rs` and
`cp-win/examples/probe.rs` are one binary, and so are the two `twice.rs`. Proven
here: building cp-win's `twice` and then cp-mac's leaves one
`target/debug/examples/twice.exe`, and the second silently replaces the first.
Under `cargo test --no-run --workspace` the two link steps run at once, and on
Windows the loser cannot open a file the winner holds.
This has been true since both crates existed and nothing had caught it: whether
it fails depends on whether the two links overlap in time, and until now they
had not. Nothing about this branch caused it — moving 94 test modules into
siblings changed how the compilation units interleave, which is enough to lose a
race that was always there.
So the examples get names of their own — `mac_probe`, `mac_twice`, `win_probe`,
`win_twice` — which also says which platform each one is for at a glance. The
`probe/` directory beside them does not move, so `include!("probe/battery.rs")`
and the two lines in .github/oversized.txt still point where they did. The four
`--example probe` invocations in ci.yml follow the rename.
Renaming one side would have been enough to break the collision, and both are
renamed because one bare `probe` next to a `mac_probe` invites the next person to
guess which is which.
…not read Rust
Two lenses over this branch found the same two holes, and both are now cases
that fail on purpose.
**The exemption I added was a back door.** `only_tests()` looked for the
`#[path]` anywhere in a window of three lines after a `#[cfg(test)]`, without
checking they belonged to the same item. 1552 lines of production code went
invisible in eight different ways: a stray `#[cfg(test)] const` three lines
above a production `mod`, a `#[path]` inside a comment, a `#[path]` inside a raw
string, a file exempting itself, a `..` crossing into another crate, and — worst
— a `#[cfg(test)]` in a `.rs` that nothing compiles, outside `src/`, where
neither clippy nor rustfmt nor the no-comments rule could ever refute it. The
shape that opens it already exists in the tree at `crates/cp-panel/src/engine.rs`.
It now takes the exact three lines, in order, with nothing between them: the
`cfg`, then `#[path = "<name>.rs"]` with a bare name and no separators, then
`mod <ident>;`. A file cannot exempt itself, and only files the measurer already
reads can exempt anything, so an exemption always sits somewhere a compiler and
a formatter can see. Twelve cases: the eight attacks are refused and the four
legitimate shapes still work — including `all(windows, test)` and
`any(test, feature = "z")`, which the old window rejected by accident of order.
**And the brace counter never understood Rust.** `bare_of` neutralised `"…"` and
`'x'` but not `r#"…"#`, so a raw string with an odd number of quotes inside
desynchronised the rest of the line. This is not hypothetical: measuring
`origin/main` with this branch's script, `crates/cp-core/src/kind.rs` reads 323
lines where it should read 304, and the measurer hands a test function
(`one_marker_alone_is_not_code`) to production. The line is
`assert!(balanced(r#"{"\""}"#), …)` — it counts −1 braces, so `mod borders`
closes 19 lines early and the rest is counted as code. The mirror image is the
dangerous one, and the lens proved it: a file of 1612 lines, 1604 of them
production, measures **0** and passes the ceiling.
Rather than teach the counter about raw strings, the block-skipping is gone. It
existed to subtract inline test modules, and after this branch there are none:
`code_of` is now the line count of the file, the way Tisty measures. What made
that safe is a new rule — a test module written inside a file of code fails CI,
so the case the skipping handled cannot come back. The whole class of
brace-counting bugs goes with it.
The counted numbers move, and upwards, because the `#[cfg(test)]` helpers that
stayed in their files now count as what they are — lines in a file of code:
`store.rs` 1409 → 1457, `paste_as.rs` 1363 → 1366. Nothing crosses the ceiling
and the three lines in .github/oversized.txt still hold.
A second rule closes the class that `b086c54` only closed one case of: two
crates may not name an example the same, because cargo links every example to
`target/<profile>/examples/<name>` and the two link steps race.
… test from code Sixteen of the 94 siblings carried the name of a production module — `store_listing.rs`, `kind_borders.rs`, `dib_properties.rs`, `watch_insisting.rs` — because that is what the module inside them is called. It reads well and it cost two things. **The next split of `store.rs` was aimed straight at them.** That file measures 1457 against a ceiling of 1500: 43 lines of room. Whoever has to split it will reach for `store_listing.rs`, `store_housekeeping.rs` and `store_identity.rs`, and all three already exist and are exempt from the ceiling. Either they pick a worse name and live with `store_listing.rs` (tests) beside `store_lists.rs` (code), or they put production in an exempt file and it stays unmeasured for good, because `store.rs` still declares it. **And no tool could tell them apart.** The only thing that knows which files are tests is the measurer reading the `#[path]` declarations; every other consumer — `--ignore-filename-regex`, `sonar.test.inclusions`, a future `exclude_globs` — can only match names. With these sixteen, matching `*_test.rs` would have classified fourteen test files as production, silently. So the name says which module it is *and* ends in `_test.rs`: `store_listing_test.rs`, `kind_inherited_from_2x_test.rs`, `waking_windows_test.rs`. Tisty names them descriptively and has the same latent trap; this diverges from it on purpose. Nothing moved but the names and the sixteen `#[path]` lines that point at them. 853 tests. Two `#[cfg(test)]` helpers also go where they belong. `ansi_drop_of` in `crates/cp-win/src/drop.rs` was 13 lines of test code sitting in a file of production, `pub(crate)` for a caller that is now its own child module, and its only user is `drop_test.rs`. `A_DAY` in `crates/cp-panel/src/engine.rs` is used only by `engine_test.rs`, and its twin in cp-store already lives inside the sibling. The other four stray helpers stay where they are: each is used by three or four different siblings, so the parent is the right place.
…here a test goes
`crates/cp-panel/src/app.rs` and `measure.rs` are excluded from coverage on
purpose, in four `--ignore-filename-regex` arguments and in
`sonar.coverage.exclusions`. The pattern names `(app|main|measure)\.rs`, which
cannot match `app_test.rs`. So 58 lines that the policy says not to measure
arrived at ~100% into the denominator of `--fail-under-lines 90` and `95`,
loosening the gate, and Sonar would have shown `app_test.rs` fully covered in a
crate whose `app.rs` is deliberately uncovered. The regex takes `(_test)?` now,
and the two siblings join the Sonar list.
Measured, so the size of it is not a guess: across cp-config the lcov total does
not move at all — `lib.rs` 244 lines at 98.36% before, `lib.rs` 87 plus
`lib_test.rs` 157 at the same 98.36% after — because those lines were already
counted inside the file they came from. Only the two excluded files changed
anything.
`sonar.test.inclusions` now claims the siblings as tests rather than sources.
They were analysed as production before too, inline, so this is not a
regression being fixed but an opportunity the split opened: 14.726 lines stop
being weighed as product code.
And CONTRIBUTING.md said "Tests when appropriate", which left a contributor
writing `#[cfg(test)] mod tests { }` inside the file — the thing this branch
just undid 94 times, and which now fails CI. It carries the pattern, the naming
rule and why the name matters. While there: it asked for "comments in English",
which has contradicted the "the code carries no comments" rule in rules.yml for
as long as both existed.
…did nothing
A code review over the last round found ten holes worth closing. Three were mine
from that round.
**The Sonar change was inert.** `sonar.test.inclusions` only filters inside the
roots that `sonar.tests` names, and that is still `app/src`, so the two Rust globs
added last commit could never match a file and the 94 siblings stayed main
source. They are out again rather than half-done: widening `sonar.tests` without
also moving those paths out of `sonar.sources` aborts the scan with «can't be
indexed twice», and with `sonar.qualitygate.wait=true` a broken scan blocks every
PR. Classifying them as tests is worth doing and needs a run to verify, which is
not this branch.
**CONTRIBUTING.md licensed a comment CI refuses.** It said a hidden constraint
earns one line; `rules.yml` fails on any `//` or `/*` in `crates` and
`app/src-tauri/src`, with no exemption. And it claimed the `_test.rs` name is how
the ceiling tells a test from code, which was false — the measurer keyed on the
`#[path]` declaration and never looked at the name. That is true now, because the
declaration must point at a `_test.rs`, and the doc says what actually holds.
**Moving the tests out switched off `clippy::items_after_test_module`.** With the
module inline, clippy refused code after it; behind `#[path]` it sees nothing, so
the declaration could sit mid-file with two hundred lines below and everything
passed. `app/src-tauri/src/backup.rs` was the standing proof, carrying 94 lines
after its declaration — green on main only because a `#[derive]` on the next item
suppresses the lint. Its declaration goes last now, like the other 93, and
`--inline` checks it for all of them.
**The cfg matcher was wrong in both directions.** `[^)]*\btest\b` matched
`#[cfg(not(test))]`, `#[cfg(feature = "test")]` and `#[cfg(target_os = "test")]`
— so three lines of an ordinary platform split exempted a 1552-line production
file — and missed `#[cfg(all(not(miri), test))]`, because the character class
cannot cross a nested paren. One matcher now serves both the ceiling and the ban:
it neutralises strings, drops `not(test)`, and then looks for `test` as a word.
**The exemption never required the suffix.** `#[path = "giant.rs"]` on a
production file hid it from both ceilings, bypassing `.github/oversized.txt`,
which is the opt-out that demands a count, a path and a written reason. The
declaration must name a `*_test.rs`.
**And the ban had four open doors:** an attribute between the `cfg` and the
`mod`, both on one line, `mod tests{` without the space, and `mod Tests {` with a
capital. It is no longer a grep — `scripts/oversized.py --inline` shares the
matcher, so the ban and the exemption cannot disagree about what a test gate is —
and it also refuses a `*_test.rs` that nothing declares, which was the missing
twin: lose a declaration and the tests stop running, rustfmt stops reaching the
file, and the only error that appears blames the test file for being 1615 lines.
Seventeen cases, each failing on purpose: four shapes that must not exempt and
one nested-paren shape that must, six spellings of an inline module plus one
`#[cfg(not(test))] mod` that is legitimate, code after a declaration, and an
undeclared sibling.
Two more, smaller. The example-name check could not see cargo's directory form,
`examples/<name>/main.rs`, so two crates could still collide through it; it walks
both forms now, in every crate, and both new steps carry `set -euo pipefail` —
without it, a renamed directory made them print success and exit 0 on a `grep`
that found nothing to read. And `examples/` and `build.rs` are out of the ban's
reach, because there the prescribed sibling becomes its own target and fails to
build for want of a `main`: a contributor had no legal option there.
**One finding is measured and deliberately not acted on.** The coverage
exclusion still names `(app|main|measure)(_test)?\.rs` rather than the general
`_test\.rs$`, which is the honest rule. Measured on this tree: excluding every
sibling takes line coverage from 92.80% to 83.21%, and the gates are
`--fail-under-lines 90` and `95`. The general rule needs the thresholds
recalibrated, which is a decision, not a cleanup. The two twins that mattered
stay excluded, and that costs 0.02 points.
The `cross` job gains `--all-targets` for cp-mac-sys, which compiles nine of the
fourteen moved macOS test files on a non-macOS host. cp-mac cannot join it: its
dev-dependencies pull blake3 and libsqlite3-sys, whose build scripts want a C
toolchain for darwin. Verified both ways.
853 tests, clippy clean with `-D warnings`, rustfmt conforming, both ceilings
green, markdownlint clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The tests are about to move into sibling files, and a sibling is a file: without
this,
crates/cp-store/src/store_test.rswould arrive at 1.617 lines against a1.500 ceiling and the first file moved would break CI.
Excluding them by name would be worse than the problem — anything could dodge
the ceiling by calling itself
_test.rs. So the exemption is the one Tistyuses: a file is skipped when some
#[cfg(test)]in the tree names it in a#[path = "..."]. The declaration earns it, not the name, and the list comesout of the source rather than a pattern.
It measures nothing differently today:
--printgives byte-identical outputbefore and after, because nothing declares a path yet. What it does is make the
next commits possible, and it is checked both ways — a 1.601-line sibling behind
a declaration passes, the same file with nobody declaring it is refused at both
the file and the function ceiling.