dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Ask where a TRACES tag sits, so the tool stops tagging its own fixtures

Browse Source 
The traceability tool scans `tools/`, which is its own source, and a line was
taken for a tag whenever `TRACES:` appeared anywhere on it. Its unit-test
fixtures are therefore tags. R1 — cross-platform output within a bounded
tolerance, the requirement with no acceptance criterion at all — was reported
implemented on the strength of two string literals in
`context_looks_forward_then_backward`.

R1 was only the visible case because it had no other coverage. The same
fixtures also contributed sites to FR-CAT-1, FR-CAT-2 and NFR-P1, `gestures.rs`
contributed one to FR-UI-4 from a `push_str`, and `dr-pipeline/build.rs`
contributed FR-DEV-3a and FR-DEV-3c from the tag it *emits* into generated
code. Those four requirements keep real tags elsewhere, so nothing but noise is
lost by dropping them.

The rule is about position, not about string literals. It cannot be about
string literals: `schema.rs` writes six genuine tags inside Rust string
literals, because the SQL it embeds is commented with `--`, and an extractor
that refused those would lose more than it saved. What separates the two is
where on the line the tag is. A tag written to be read is the first word of its
comment; a tag quoted inside an expression never is. So `tag_body` asks for a
comment opener at the start of the line and `TRACES:` immediately after it.

That closes every shape but one: a multi-line literal whose lines really do
begin with `///`, which no line-oriented reader can tell from source. There is
one such fixture and its ids are now UT and IT, which `is_requirement` already
excludes from coverage — the mechanism existed and was simply never used on
the tool itself. `this_crates_own_fixtures_cannot_reach_the_register` enforces
that: any requirement id below `mod tests` in this crate fails the test and
says to use a UT- or IT- id instead. Tagging the tool's real code is still
allowed.

On `SOURCE_SUFFIXES`, which cannot reach `AndroidManifest.xml`, the Flatpak
manifest, the Dockerfile or the CI workflows: it is deliberately left alone,
and the reasoning is recorded beside it. A tag on a manifest asserts that a
comment exists next to a line nothing checks, which is the weak form
CONTRIBUTING.md warns about. The convention already in the tree — a Rust test
that `include_str!`s the file and asserts what must be in it, with the tag on
the test — is what a tag is supposed to mean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-30 10:17:34 +02:00
co-authored by Claude Opus 5
parent ef07e6ca3e
commit 9d9abb227f
2 changed files with 145 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/outstanding.md
+6 -3
View File
@@ -320,9 +320,12 @@ fixed before spike S9", because S9 both validates R1 and calibrates what toleran
The threshold was never fixed and S9 has not run, so R1 currently has no acceptance criterion at
all — there is nothing a test could assert.
Worse, the matrix reports R1 as *covered*. Both of its tags are string literals inside the
traceability tool's own unit tests (`tools/traceability/src/lib.rs`), which the tool scans along with
everything else, because a fixture demonstrating tag extraction is indistinguishable from a tag.
The matrix used to report R1 as *covered*, and what covered it was two string literals: fixtures
inside the traceability tool's own unit tests, which the tool scans along with everything else,
because a fixture demonstrating tag extraction was indistinguishable from a tag. The extractor now
asks where the tag sits — a tag is the first word of a comment, not a string appearing anywhere on a
line — and R1 is untagged again, which is the honest reading while it has no acceptance criterion to
tag anything against.
NFR-OPS-1 is covered the same way, from a tag on `compute_coverage` — and no rotating, size-capped
on-disk log exists; logging goes to stderr and logcat. These are two of the cases
[CONTRIBUTING.md](../CONTRIBUTING.md) already warns about, now named.
tools/traceability/src/lib.rs
+139 -7
View File
@@ -19,6 +19,20 @@
//! count as the numerator is precisely what lets a ratio exceed 100%, since
//! a tag naming a deleted requirement would count as covered. Such tags are
//! reported as orphans instead.
//!
//! # And why the extractor is fussy about where a tag sits
//!
//! A third rule, learned the same way. `SOURCE_ROOTS` includes `tools`, so this
//! crate scans itself, and the fixtures below demonstrating tag extraction were
//! read as tags: R1 — cross-platform output within a bounded tolerance — was
//! reported implemented on the strength of two string literals in a unit test.
//!
//! 3. **A tag is a comment whose first word is `TRACES:`**, not a line in which
//! the string appears. This is a rule about position
//! rather than about string literals, because `schema.rs` writes real tags
//! inside string literals — the SQL it embeds is commented with `--` — and
//! an extractor that refused those would lose six genuine tags to save two
//! false ones.
use std::collections::{BTreeMap, BTreeSet};
use std::path::{Path, PathBuf};
@@ -182,16 +196,43 @@ pub fn is_requirement(id: &str) -> bool {
REQUIREMENT_TYPES.contains(&type_of(id))
}
/// Comment openers a tag may be introduced by.
///
/// `--` is here for the SQL embedded in `schema.rs`, which carries real tags
/// inside Rust string literals; `#` for YAML; `*` for the continuation lines of
/// a block comment.
const COMMENT_OPENERS: &[&str] = &["///", "//!", "//", "<!--", "/*", "*", "--", "#"];
/// The body of a tag comment, if this line is one.
///
/// **A tag is a comment whose first word is `TRACES:`** — not any line in which
/// the string appears. The tool scans `tools/`, which is its own source, so
/// without this rule its unit-test fixtures are tags: a one-line literal in
/// `context_looks_forward_then_backward` had R1 reported as implemented, and
/// the register believed it. That is not a quirk of this crate. `build.rs`
/// emits a tag into generated code from a string literal, and any test
/// anywhere that exercises the extractor would do the same.
///
/// The rule does not merely exclude string literals — it cannot tell one from a
/// comment, and `schema.rs` proves it must not try, since its `-- TRACES:` tags
/// live inside string literals and are entirely real. What it asks instead is
/// where on the line the tag sits: a tag written to be read is the first thing
/// in its comment, and a tag quoted inside an expression never is.
fn tag_body(line: &str) -> Option<&str> {
let line = line.trim_start();
let after_opener = COMMENT_OPENERS.iter().find_map(|o| line.strip_prefix(o))?;
after_opener.trim_start().strip_prefix("TRACES:")
}
/// Extract every `TRACES:` tag from a source file's text.
pub fn extract_from_text(text: &str, path: &str) -> Vec<TraceEntry> {
let lines: Vec<&str> = text.lines().collect();
let mut out = Vec::new();
for (idx, line) in lines.iter().enumerate() {
let Some(pos) = line.find("TRACES:") else {
let Some(tail) = tag_body(line) else {
continue;
};
let tail = &line[pos + "TRACES:".len()..];
let ids = parse_ids(tail);
if ids.is_empty() {
continue;
@@ -309,6 +350,25 @@ pub fn compute_coverage(traced: &BTreeSet<String>, defined: &DefinedRequirements
/// implementing it is generated into `OUT_DIR`, which is not scanned and could
/// not be linked to from the report if it were. Without this a node would have
/// nowhere to record the requirement it satisfies.
///
/// # What is deliberately absent, and why widening this is the wrong fix
///
/// `AndroidManifest.xml`, the Flatpak manifest, the Dockerfile and the CI
/// workflows all carry requirements — SAF grants, sandbox permissions, the API
/// levels NFR-COMPAT-1 names — and none of them can hold a tag. That reads like
/// a gap in this list. It is not one.
///
/// A tag proves that a tag exists, not that the file under it does the thing,
/// and a declarative manifest is the case where the difference bites hardest:
/// an intent filter can be deleted and the tag above it still says the
/// requirement is met. The convention already in the tree answers it better —
/// a Rust test `include_str!`s the file and asserts what must be in it, and
/// the tag goes on the test. That satisfies what CONTRIBUTING.md asks of any
/// tag, that it name something a test would fail without, which a comment in a
/// manifest never can.
///
/// So the list stays as it is, and a config file is tagged through the test
/// that reads it.
pub const SOURCE_SUFFIXES: &[&str] = &[".rs", ".slint", ".wgsl", ".yaml"];
/// Directories never scanned.
@@ -477,16 +537,23 @@ This is discussed in FR-CAT-1 and also FR-CAT-1 again.
#[test]
fn extracts_tags_with_both_separators() {
// **The ids here are UT and IT deliberately.** A multi-line literal is
// the one fixture shape `tag_body` cannot rule out: its lines begin
// with `///` in the file as well as in the string, so this really is a
// tag as far as any line-oriented reader can tell. Naming a
// requirement here would report it implemented by the traceability
// tool. UT and IT are excluded from coverage by `is_requirement`, so
// the fixture can be as tag-shaped as it likes and still cost nothing.
//
// The requirement id *shapes* are exercised by `darkroom_id_shapes_parse`
// below, which needs no `TRACES:` at all to do it.
let src = "\
/// TRACES: FR-CAT-1, FR-CAT-2 | NFR-P1
/// TRACES: UT-001, UT-002 | IT-003
pub fn scan() {}
";
let traces = extract_from_text(src, "x.rs");
assert_eq!(traces.len(), 1);
assert_eq!(
traces[0].requirements,
vec!["FR-CAT-1", "FR-CAT-2", "NFR-P1"]
);
assert_eq!(traces[0].requirements, vec!["UT-001", "UT-002", "IT-003"]);
assert_eq!(traces[0].line, 1);
assert_eq!(traces[0].context, "pub fn scan()");
}
@@ -502,6 +569,71 @@ pub fn scan() {}
assert_eq!(back[0].context, "pub fn render()");
}
#[test]
fn a_quoted_tag_is_not_a_tag() {
// What made the tool report `R1` as implemented: a fixture in this very
// module, scanned along with everything else, because a line mentioning
// `TRACES:` was taken for a line carrying it.
let quoted = extract_from_text(
r#"let s = "// TRACES: FR-CAT-1"; // and a real one below"#,
"x.rs",
);
assert!(quoted.is_empty(), "a tag inside an expression is not a tag");
// Emitted into generated code, which is the `build.rs` case.
let emitted = extract_from_text(r#" "/// TRACES: FR-DEV-3a\n","#, "x.rs");
assert!(emitted.is_empty(), "a tag being written out is not a tag");
// Prose about the mechanism, which is what this crate's own doc
// comments are full of.
let prose = extract_from_text("/// Extract every `TRACES:` tag. FR-CAT-1", "x.rs");
assert!(prose.is_empty(), "a tag named in prose is not a tag");
// And the forms that must keep working: SQL inside a Rust literal,
// which is how `schema.rs` tags its migrations, and YAML.
let sql = extract_from_text(" -- TRACES: FR-CULL-8\n", "x.rs");
assert_eq!(sql[0].requirements, vec!["FR-CULL-8"]);
let yaml = extract_from_text("# TRACES: FR-DEV-3a\nid: exposure\n", "x.yaml");
assert_eq!(yaml[0].requirements, vec!["FR-DEV-3a"]);
}
#[test]
fn this_crates_own_fixtures_cannot_reach_the_register() {
// `SOURCE_ROOTS` includes `tools`, so this crate is scanned by the tool
// it implements and a fixture below is indistinguishable from a tag on
// the code above. `tag_body` closes every shape but one — a multi-line
// literal whose lines begin with a comment opener — and this closes
// that one, by asking where the tag is rather than what it looks like.
//
// Tagging the tool's real code is still allowed; two such tags were
// wrong on other grounds and were removed, but the rule here is only
// that a fixture may not name a requirement. Use a `UT-` or `IT-` id.
for (name, src) in [
("lib.rs", include_str!("lib.rs")),
("gestures.rs", include_str!("gestures.rs")),
("main.rs", include_str!("main.rs")),
] {
let fixtures_begin = src
.lines()
.position(|l| l.trim_start().starts_with("mod tests"))
.map_or(usize::MAX, |i| i + 1);
for entry in extract_from_text(src, name) {
let reqs: Vec<&String> = entry
.requirements
.iter()
.filter(|id| is_requirement(id))
.collect();
assert!(
entry.line < fixtures_begin || reqs.is_empty(),
"{name}:{} is a test fixture naming {reqs:?}, which the \
register would read as implemented. Use a UT- or IT- id.",
entry.line,
);
}
}
}
#[test]
fn a_tag_with_no_ids_is_ignored() {
let traces = extract_from_text("// TRACES: see the design doc\n", "x.rs");