Skip to content

Commit e79fcac

Browse files
karthiknadigCopilot
andcommitted
fix: classify conditional test coverage correctly (Refs #534)
Exclude all/any predicates that require test without excluding optional test branches. Keep unmapped integration-test and benchmark entries in the test denominator while preserving conservative bounds and raw coverage gates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 543e80f commit e79fcac

3 files changed

Lines changed: 70 additions & 8 deletions

File tree

‎docs/QUALITY_SNAPSHOTS.md‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,15 +123,18 @@ without instrumentation are listed explicitly, never assumed covered. These diag
123123
introduce a fabricated baseline, change the raw denominator, or replace regression protection.
124124

125125
Classification excludes integration-test/benchmark directories and Rust items explicitly marked
126-
`#[cfg(test)]` or `#[test]`, including inline modules and test-only helper functions. It masks
126+
`#[cfg(test)]` or `#[test]`, including inline modules and test-only helper functions. Nested
127+
`all`/`any` predicates are also excluded when they require `test`: `all(test, unix)` is test-only,
128+
but `any(test, unix)` is not. It masks
127129
strings, raw/byte strings, characters, and nested comments before matching item boundaries.
128-
Helpers outside those boundaries and complex conditional attributes remain conservatively in the
130+
Helpers outside those boundaries and unsupported conditional predicates remain conservatively in the
129131
production category; this is a source-focused diagnostic, not full Rust conditional-compilation
130132
analysis. Invalid/missing LCOV, missing source, inconsistent hit summaries, and source-line
131133
mismatches fail the reporting step. LLVM summaries can include more entries in `LF`/`LH`
132134
than the unique `DA` source lines (observed in real Windows exports). That deficit is reported
133-
per file (including unmatched summary hits) and conservatively retained as uncovered production,
134-
never dropped from the denominator or silently assigned coverage.
135+
per file (including unmatched summary hits) and conservatively retained as uncovered production
136+
in mixed source files, or uncovered tests in integration-test/benchmark files. It is never dropped
137+
from the denominator or silently assigned coverage.
135138
Changed lines without `DA` records are listed separately in JSON, including non-executable syntax;
136139
they are not silently considered covered.
137140
Native macOS also demonstrates `LH` below the number of positive unique `DA` entries. Reports

‎scripts/coverage_detail.py‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,37 @@ def code_mask(source: str) -> str:
149149
return ''.join(chars)
150150

151151

152+
def cfg_requires_test(expression: str) -> bool:
153+
expression = expression.strip()
154+
if expression == 'test':
155+
return True
156+
group = re.fullmatch(r'(all|any)\s*\((.*)\)', expression, re.DOTALL)
157+
if not group:
158+
return False
159+
arguments = []
160+
depth = start = 0
161+
for index, char in enumerate(group[2]):
162+
depth += (char == '(') - (char == ')')
163+
if depth < 0:
164+
raise SnapshotError('Unbalanced Rust cfg predicate')
165+
if char == ',' and depth == 0:
166+
arguments.append(group[2][start:index])
167+
start = index + 1
168+
if depth:
169+
raise SnapshotError('Unbalanced Rust cfg predicate')
170+
final = group[2][start:]
171+
if final.strip():
172+
arguments.append(final)
173+
required = [cfg_requires_test(argument) for argument in arguments]
174+
return any(required) if group[1] == 'all' else bool(required) and all(required)
175+
176+
152177
def test_lines(source: str) -> set[int]:
153178
masked = code_mask(source)
154179
excluded: set[int] = set()
155-
for match in re.finditer(r'#\s*\[\s*(?:cfg\s*\(\s*test\s*\)|test)\s*\]', masked):
180+
for match in re.finditer(r'#\s*\[\s*(?:cfg\s*\((?P<cfg>[^]]*)\)|test)\s*\]', masked):
181+
if match['cfg'] is not None and not cfg_requires_test(match['cfg']):
182+
continue
156183
start = match.start()
157184
end = match.end()
158185
# Other attributes belong to this same item, not its body.
@@ -226,20 +253,21 @@ def summarize(root: Path, records: dict[str, SourceCoverage], changed: dict[str,
226253
if any(n > len(source.splitlines()) for n in lines):
227254
raise SnapshotError(f'LCOV source revision mismatch: {name}')
228255
parts = Path(name).parts
229-
excluded = set(lines) if 'tests' in parts or 'benches' in parts else test_lines(source)
256+
test_only = 'tests' in parts or 'benches' in parts
257+
excluded = set(lines) if test_only else test_lines(source)
230258
production = {n: count for n, count in lines.items() if n not in excluded}
231259
tests = {n: count for n, count in lines.items() if n in excluded}
232260
edits = production.keys() & changed.get(name, set())
233261
uncertainty = record.unmapped + record.summary_hit_shortfall - record.unmapped_hits
234262
files.append({
235-
'path': name, 'production_found': len(production) + record.unmapped,
263+
'path': name, 'production_found': len(production) + (0 if test_only else record.unmapped),
236264
'unmapped_summary_lines': record.unmapped,
237265
'unmapped_summary_hits': record.unmapped_hits,
238266
'summary_hit_shortfall': record.summary_hit_shortfall,
239267
'mapped_hit_uncertainty': uncertainty,
240268
'changed_lines_without_line_records': sorted(changed.get(name, set()) - lines.keys() - excluded),
241269
'production_hit': max(0, sum(n > 0 for n in production.values()) - uncertainty),
242-
'test_found': len(tests),
270+
'test_found': len(tests) + (record.unmapped if test_only else 0),
243271
'test_hit': max(0, sum(n > 0 for n in tests.values()) - uncertainty),
244272
'uncovered_production': sorted(n for n, hits in production.items() if not hits),
245273
'changed_production_found': len(edits),

‎scripts/tests/test_coverage_detail.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,37 @@ def test_unmapped_hits_cannot_inflate_any_subset_lower_bound(self):
123123
self.assertEqual(row['test_hit'], 0)
124124
self.assertEqual(row['changed_production_hit'], 0)
125125

126+
def test_conditional_attributes_exclude_only_items_requiring_test(self):
127+
required = [
128+
'all(test, unix)', 'all(windows, test,)', 'all(test, feature = "flag")',
129+
'all(any(unix, windows), all(feature = "flag", test))',
130+
'any(all(test, unix), all(windows, test))',
131+
]
132+
optional = [
133+
'any(test, unix)', 'any(windows, test)', 'not(test)', 'all(unix, feature = "test")',
134+
'any(all(test, unix), windows)', 'all(any(test, unix), windows)', 'any()', 'all()',
135+
]
136+
for condition in required + optional:
137+
source = f'#[cfg({condition})]\nmod scoped {{\n fn helper() {{}}\n}}\nfn real() {{}}\n'
138+
with self.subTest(condition=condition):
139+
self.assertEqual(detail.test_lines(source), {1, 2, 3, 4} if condition in required else set())
140+
141+
def test_unmapped_test_file_entries_stay_in_the_test_denominator(self):
142+
for folder in ['tests', 'benches']:
143+
name = f'crates/pet/{folder}/fixture.rs'
144+
path = self.root / name
145+
path.parent.mkdir(parents=True)
146+
path.write_text('fn helper() {}\nfn check() {}\n')
147+
records = detail.line_records(self.lcov(self.record(name).replace('LF:2', 'LF:3')))
148+
row = detail.summarize(self.root, records, {name: {1, 2}})['files'][0]
149+
with self.subTest(folder=folder):
150+
self.assertEqual(row['production_found'], 0)
151+
self.assertEqual(row['production_hit'], 0)
152+
self.assertEqual(row['changed_production_found'], 0)
153+
self.assertEqual(row['test_found'], 3)
154+
self.assertEqual(row['test_hit'], 0)
155+
self.assertEqual(row['unmapped_summary_lines'], 1)
156+
126157
def test_inline_tests_do_not_hide_later_production_items(self):
127158
source = 'fn before() {}\n#[cfg(test)]\nmod tests {\n fn check() {}\n}\nfn after() {}\n'
128159
self.assertEqual(detail.test_lines(source), {2, 3, 4, 5})

0 commit comments

Comments
 (0)