Repository navigation
Measure test coverage in CI and cover the app and tools - #19
Merged
Merged
Conversation
The tests ran about 54% of the crate's lines: nothing exercised the app, the bench and train tools, or the neural network directly. New tests bring line coverage, every file counted, above 80%: * tests/gui.rs drives the app without a window (egui_kittest) through its accessibility tree, the keyboard and the pointer: every layout, control mode and controller, the comparison, corridors and the green wave, every setting, overlay and coloring, placing and selecting vehicles, pan and zoom, both benchmark windows and About. It also checks that every control has a name a screen reader can say. * tests/scripted.rs opens each TS_* scene in a process of its own, as the app reads the variables when it starts, and checks that screenshots are valid PNG files. * tests/tools.rs runs bench and train briefly, in scratch directories with their own home directories. * tests/networks.rs checks backpropagation against the numerical gradient, Adam, the shape checks and Double DQN. A new Coverage job runs cargo-llvm-cov and fails below 80% of lines. Code built for coverage counts what it runs in counters that every thread updates, so threads running the same code at once slow one another down many times over: tests/safety.rs took 51 s instead of 7 s, with 21 times the CPU time. The job runs the tests one at a time, and under cfg(coverage) the tests that run many simulations at once run them in turn (tests/common/mod.rs). Promoting counters to registers (-do-counter-promotion) didn't help. egui_kittest's toml needs toml_datetime 1.1.3 rather than 1.1.2. Some of the app's proc macros use that crate too, so THIRD_PARTY_LICENSES.md now lists the new version. CONTRIBUTING.md says how to measure coverage locally, and docs/architecture.md and the README describe the new tests and job. Signed-off-by: Jim Schreckengast <schreck@gmail.com>
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
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.
What
tests/gui.rs(16) runs the app without a window usingegui_kittest. It drives the app with the keyboard, the pointer, and the names its controls give screen readers. It covers every layout, control mode and controller, the side-by-side comparison, corridors and the green wave, every setting, overlay and coloring, placing and selecting vehicles, pan and zoom, both benchmark windows, and About. It also checks that every control has a name.tests/scripted.rs(8) opens eachTS_*scene in a separate process, because the app reads those variables when it starts and tests that run in parallel can't set them. It also checks that screenshots are valid PNG files.tests/tools.rs(7) runsbenchandtrainbriefly as programs. Each runs in a scratch directory that also serves as its home directory, so it never touches the user's saved weights. Code that the programs run counts toward coverage.tests/networks.rs(7) checks backpropagation against the numerical gradient. It also covers Adam, the shape checks that reject saved weights that don't fit, and Double DQN.cargo llvm-cov --locked --release --fail-under-lines 80 -- --test-threads=1, with no file excluded. It installs cargo-llvm-cov 0.9.1 withtaiki-e/install-action, pinned by SHA.egui_kittest0.37 from the egui project, which brings inkittest0.5. Itstomldependency needstoml_datetime1.1.3 rather than 1.1.2. Some of the app's proc macros also use that crate, soTHIRD_PARTY_LICENSES.mdnow lists the new version. Nothing else in the shipped binaries changes.Production code is unchanged: nothing under
src/changed. Apart from tests, CI and docs, the only change is inCargo.toml: the dev-dependency and a[lints.rust]entry that declares thecoveragecfg so the compiler recognizes it.Why
The tests ran 54% of the crate's lines. Nothing exercised the app, the
benchandtrainprograms, or the neural network on its own. This meets the test_statement_coverage80 criterion of the OpenSSF Best Practices silver badge, and the new job keeps it met.Why the job runs the tests one at a time
Code built for coverage counts what it runs in counters that every thread updates without synchronization. When several threads run the same code at once, they keep taking those cache lines from one another, and everything slows down many times over:
tests/safety.rstests/gui.rs, tests in parallelPromoting the counters to registers (
-C llvm-args=-do-counter-promotion) made no difference.So the job passes
--test-threads=1. The tests that run many simulations at once now start them withspawnfromtests/common/mod.rs. Undercfg(coverage), which cargo-llvm-cov sets,spawnruns the simulations in turn; otherwise it runs them on threads as before.runs_are_reproduciblekeeps its two real threads, because it checks that runs agree when their hash maps iterate differently.Run one at a time under coverage, the tests take about 140 s locally: cooperative 59 s, tools 37 s, gui 26 s and safety 13 s.
Results
Coverage as measured by
cargo llvm-cov --release -- --test-threads=1on macOS. Linux will differ a little, because some branches are platform-specific.src/gui/src/bin/(bench,train)src/rl/src/control/src/sim/src/lib.rssrc/main.rsBefore this change, coverage was 53.81%, with 5,914 lines missed. Three things stay uncovered:
src/main.rs, which only opens a native window.shot::request(7 lines). It needs a renderer, whichegui_kittestprovides only with itswgpufeature, so the tests checkwrite_pngdirectly instead.weightslooks for saved weights.Required check?
Coverage isn't a required check yet, and the ruleset is unchanged. It should become required once it has passed on
mainand its runtime on CI is known. That needs a ruleset change. It also needs an update to the list of required checks in GOVERNANCE.md, and to §11 of docs/architecture.md: its "10 are required" and its table.Testing
cargo fmt --all --checkandcargo fmt --manifest-path fuzz/Cargo.toml --checkare clean.cargo test --locked --releasepasses. One test is ignored on purpose:sceneintests/scripted.rs, which the other scripted tests run in a child process.cargo clippy --locked --release --all-targets -- -D warningsand its wasm32 counterpart pass.cargo deny --locked check,packaging/licenses.sh --checkandcargo package --lockedpass.async-executor1.14.0 has been yanked. That warning isn't new:mainhas the same version, which eframe's accessibility support on Linux uses.cargo llvm-cov --locked --release --fail-under-lines 80 -- --test-threads=1passes locally, at 95.57%. The Coverage job hasn't run on Linux yet.Follow-ups
tests/row.async-executor1.14.0 has been yanked and should be updated in a separate change.