test: make source-scanning tests independent of file layout - #154
Merged
Merged
Conversation
The Reason census, the guard-site scan, and the two config no-I/O and no-environment scans named their files with include_str! or matched sites by file name, so splitting a module moved code out of their sight. They now read every file of the module they hold at run time, and the guard-site scan matches a site by its code as a multiset rather than by file and text. Closes #133
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.
Summary
The four tests that hold a property of the source text named their files, so splitting a module would move code out of their sight. They now read whole modules off the directory tree at run time, and the guard-site scan matches a call site by its code rather than by its file.
src/testing.rs: adds three test-only (#[cfg(test)]) helpers.rust_sourceswas already here, private to the test module; it moves up a level and now sorts its output.src_rootis new.module_sources("a::b")returnssrc/a/b.rsand every.rsfile undersrc/a/b/, sorted. If the module has no source at all it panics, so a renamed module can't leave a scan reading nothing. Two new tests covermodule_sources: one checks a directory module, a module that has both a file and a directory, and a single-file module; the other checks the panic.src/env_guard.rs, Reason census (declared_reasons): dropsinclude_str!("env_guard.rs"). It now reads every file of theenv_guardmodule and finds the declaration by a line whose trimmed text is exactlypub enum Reason {. Exactly one such line must exist. The parsing inreasons_declared_inand its cross-check are unchanged.src/env_guard.rs,every_site_that_reaches_the_guard_is_known:src/env_guard.rsplus everything beneathsrc/env_guard/, instead of one file path.KNOWNis now a multiset of code lines with no file names. Each entry is claimed by one site. A site moved into another file still passes. A known line repeated anywhere is reported as a new caller.rust_sourceswalk instead of its own.src/config/merge.rs,no_config_module_reads_the_environment:configmodule plus thepathsmodule, instead of a fixedinclude_str!list of seven files.pathsedge lines are still allowed only insidepaths, each exactly once, now counted across the whole module.src/config/values/local.rs,the_setter_performs_no_io: scans every file ofconfig::values::localinstead ofinclude_str!("local.rs").Deliberate violation run
Six temporary violations were added to the source, and the four tests were run against this branch's tests and then against
master's tests. The violations were then removed; none is committed.mastertestsReason::ProbeVariantwith an#[error]and a terminating arm, chained from nothingthe_reasons_render_as_sentencesfails:declared, but never reached by the chain ...: ["ProbeVariant"]crate::env_guard::is_relocating("HOME")insrc/report.rsevery_site_that_reaches_the_guard_is_knownfails:the guard has a new caller: [".../src/report.rs:197"]std::env::var_os("X")in the non-test half ofsrc/config/merge.rsno_config_module_reads_the_environmentfails:merge.rs containsenv::var``config/merge.rs containsenv::var``std::fs::readin the non-test half ofsrc/config/values/local.rsthe_setter_performs_no_iofails:`std::fs` appearedconfig/values/local.rs:std::fsappearedstd::env::var_os("X")insrc/config/history.rs, which the old list never namedconfig/history.rs containsenv::var``use crate::env_guard::is_variable_name;repeated intosrc/config/history.rsconfig/history.rsis not a listed fileThe command for both runs was
cargo test --lib -- the_reasons_render_as_sentences every_site_that_reaches no_config_module_reads the_setter_performs_no_io. Every violationmastercatches is still caught here.Validation
All of these ran at
d1deb15through the repository'shkpre-commit and pre-push hooks:cargo fmt --checkpasses.scripts/line-checkpasses:no file newly over 3743 lines.cargo clippy --all-targets -- -D warningspasses.cargo testpasses: lib reports 2036 passed, 0 failed, 14 ignored; every other test binary is green.cargo llvm-cov --ignore-filename-regex 'src/main\.rs' --summary-only --fail-under-lines 80passes at 97.27% lines.convcopasses on the commit message.Risks and rollout
#[cfg(test)]or inside amod tests, so the library and binary do not change.src/shell/activation.rsholds!env_guard::is_relocating(&name)twice: once in the plain name search and once in the search for names split by quotes. Under the multiset the second copy needed its ownKNOWNentry, soKNOWNgrows from 15 to 16. The test is now stricter, not looser.config/{mod,env,external,history,origin,path,secrets,shell_options,tool,tree,when}.rs. None of them holds a forbidden token in its non-test half today.#[cfg(test)] mod tests;would have that file scanned whole, because the non-test half is still found by splitting at the first#[cfg(test)]. No such file exists today. If one is added, the scan fails loudly rather than letting anything through.Decisions taken
How the scans find their files
Taken: directory walk at test time (accepted unrebutted)
Rejected: per-file
include_str!lists - every split would have to edit them, and a forgotten edit blinds the testReverses: restore the lists
Where the shared walk lives
Taken:
#[cfg(test)]helpers insrc/testing.rs, which already heldrust_sourcesRejected: a copy of the walk in each test, which means four walks to keep in step; a non-
cfg(test)helper, which would compileenv!("CARGO_MANIFEST_DIR")into the libraryReverses: inline the walk in each test and delete
module_sourcesandsrc_rootWhat a scan reads
Taken: the whole crate module, meaning
src/a/b.rsplus everything undersrc/a/b/, chosen by module path. The config scan reads all ofconfigandpaths, not the seven files the list happened to name.Rejected: all of
src/, which would pull in modules that do read the environment (the binary edge,detect) and would need a new exception listReverses: narrow
[("config", ..), ("paths", ..)]back to named filesHow a guard call site is identified
Taken: by its code as a multiset, so a known line is claimed once per entry
Rejected: a set of code lines, which would let a known line be copied into any file unnoticed; file plus text, which the issue rules out
Reverses: key
KNOWNon(file, line)againThe violation-run count (review item C1)
Taken: the count reads six, matching the six rows the table records; no seventh violation, its command, or its outcome exists anywhere in the record, so a seventh row could only be invented. At
d1deb15, with no violation applied, the four tests pass:cargo test --lib -- the_reasons_render_as_sentences every_site_that_reaches no_config_module_reads the_setter_performs_no_ioreports 4 passed, 0 failed.Rejected: adding a seventh row, which would record a run nobody can show happened
Reverses: run a seventh violation against both test sets and add its row with the count back at seven
Issue
Closes #133