fix: stop verify, reports and briefs from claiming more than pipe checked #1
7 changed files with 255 additions and 16 deletions
fix(verify): a finding in an excluded file is not scanned, not resolved
The analyzer skips a file that analysis_options `exclude:` names (contextFor throws, which pipe records as `skipped`). The verify gate refused a scan only when files FAILED, and partialScanWarning named only failures, so a skipped file's accepted findings were simply absent from the fresh report - and verify listed them as RESOLVED, "these fixes are confirmed", for code nobody read. Excluded files are a normal part of a project, so they do not abort verify. Instead each accepted finding whose file was not analysed - skipped, failed, filtered out by --no-include-generated, or outside every scanned --dir - is reported as NOT SCANNED: neither resolved nor still present. allResolved (and so --expect-resolved) now requires that list to be empty too. partialScanWarning counts skipped files, so a scan with excluded files is announced as PARTIAL on stderr like one with failures.
commit
9eb3d98d4d
|
|
@ -70,4 +70,5 @@ export 'src/state_flow.dart'
|
|||
export 'src/state_flow.dart'
|
||||
show RiverpodGap, RiverpodModel, RiverpodProvider, RiverpodSite;
|
||||
export 'src/suggest.dart' show FilePlan, FixAction, WorkPlan, buildWorkPlan;
|
||||
export 'src/verify.dart' show VerificationReport, verifyAgainstBaseline;
|
||||
export 'src/verify.dart'
|
||||
show VerificationReport, verifyAgainstBaseline, verifyAnalysis;
|
||||
|
|
|
|||
|
|
@ -545,8 +545,12 @@ class VerifyCommand extends Command<int> {
|
|||
);
|
||||
}
|
||||
|
||||
final report = verifyAgainstBaseline(
|
||||
report: analysis.report,
|
||||
// Files the analyzer skipped - excluded by analysis_options, or outside
|
||||
// any analysis context - are a legitimate part of a project, so they do
|
||||
// not abort the run. But a finding in one was never looked for, so it is
|
||||
// reported as not scanned rather than resolved.
|
||||
final report = verifyAnalysis(
|
||||
analysis: analysis,
|
||||
baseline: entries,
|
||||
rules: selected.isEmpty ? null : selected,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -217,10 +217,24 @@ String? limitedRunWarning({
|
|||
'$discovered file(s) were analysed, so every result below is PARTIAL.';
|
||||
}
|
||||
|
||||
/// Warning for a scan in which some files could not be analysed, or null.
|
||||
String? partialScanWarning({required int failed, required int resolved}) {
|
||||
if (failed == 0) return null;
|
||||
return 'pipe: WARNING $failed file(s) could not be analysed '
|
||||
/// Warning for a scan in which some files were not analysed, or null.
|
||||
///
|
||||
/// Skipped files count: a file excluded by analysis_options `exclude:` (or
|
||||
/// outside any analysis context) is as unexamined as one that failed, and a
|
||||
/// warning that named only failures let such a scan read as complete.
|
||||
String? partialScanWarning({
|
||||
required int failed,
|
||||
required int resolved,
|
||||
int skipped = 0,
|
||||
}) {
|
||||
if (failed == 0 && skipped == 0) return null;
|
||||
final parts = [
|
||||
if (failed > 0) '$failed file(s) could not be analysed',
|
||||
if (skipped > 0)
|
||||
'$skipped file(s) were not analysed because analysis_options excludes '
|
||||
'them or no analysis context covers them',
|
||||
];
|
||||
return 'pipe: WARNING ${parts.join(' and ')} '
|
||||
'($resolved resolved), so results are PARTIAL. '
|
||||
'Run `pipe index` to see why.';
|
||||
}
|
||||
|
|
@ -293,6 +307,7 @@ Future<ResolutionRun> resolveProject(
|
|||
final partialWarning = partialScanWarning(
|
||||
failed: failedCount,
|
||||
resolved: resolvedCount,
|
||||
skipped: skippedCount,
|
||||
);
|
||||
if (partialWarning != null) stderr.writeln(partialWarning);
|
||||
|
||||
|
|
|
|||
|
|
@ -1,3 +1,6 @@
|
|||
import 'package:path/path.dart' as p;
|
||||
|
||||
import 'pipeline.dart';
|
||||
import 'rules.dart';
|
||||
|
||||
/// What changed since a baseline: what got fixed, what remains, what is new.
|
||||
|
|
@ -12,6 +15,7 @@ class VerificationReport {
|
|||
required this.stillPresent,
|
||||
required this.newFindings,
|
||||
this.outOfScope = 0,
|
||||
this.notScanned = const [],
|
||||
});
|
||||
|
||||
/// Accepted findings of the rules that ran, i.e. the ones this report judges.
|
||||
|
|
@ -30,17 +34,27 @@ class VerificationReport {
|
|||
/// Findings that were not in the baseline at all.
|
||||
final List<Finding> newFindings;
|
||||
|
||||
bool get allResolved => stillPresent.isEmpty;
|
||||
/// Accepted findings in files this scan did not analyse - excluded by
|
||||
/// analysis_options, filtered out, or outside the scanned directories. Their
|
||||
/// absence from the report says nothing, so they are neither resolved nor
|
||||
/// still present.
|
||||
final List<BaselineEntry> notScanned;
|
||||
|
||||
/// True only when every accepted finding was looked for and is gone: an
|
||||
/// accepted finding in an unscanned file is not a confirmed fix.
|
||||
bool get allResolved => stillPresent.isEmpty && notScanned.isEmpty;
|
||||
|
||||
Map<String, Object?> toJson() => {
|
||||
'baselineTotal': baselineTotal,
|
||||
'outOfScope': outOfScope,
|
||||
'resolved': resolved.length,
|
||||
'stillPresent': stillPresent.length,
|
||||
'notScanned': notScanned.length,
|
||||
'new': newFindings.length,
|
||||
'allResolved': allResolved,
|
||||
'resolvedEntries': [for (final entry in resolved) entry.toJson()],
|
||||
'stillPresentEntries': [for (final entry in stillPresent) entry.toJson()],
|
||||
'notScannedEntries': [for (final entry in notScanned) entry.toJson()],
|
||||
'newFindings': [for (final finding in newFindings) finding.toJson()],
|
||||
};
|
||||
|
||||
|
|
@ -58,6 +72,12 @@ class VerificationReport {
|
|||
'${resolved.isEmpty ? '' : ' — these fixes are confirmed'}',
|
||||
)
|
||||
..writeln('still ${stillPresent.length} still reported')
|
||||
..write(
|
||||
notScanned.isEmpty
|
||||
? ''
|
||||
: 'unscanned ${notScanned.length} in files this scan did not '
|
||||
'analyse - not confirmed either way\n',
|
||||
)
|
||||
..writeln('new ${newFindings.length} not in the baseline');
|
||||
if (resolved.isNotEmpty) {
|
||||
buffer.writeln('RESOLVED');
|
||||
|
|
@ -77,6 +97,15 @@ class VerificationReport {
|
|||
);
|
||||
}
|
||||
}
|
||||
if (notScanned.isNotEmpty) {
|
||||
buffer.writeln('NOT SCANNED');
|
||||
for (final entry in notScanned.take(top)) {
|
||||
buffer.writeln(
|
||||
' ? ${entry.location} ${entry.label}'
|
||||
' [${entry.rule}]',
|
||||
);
|
||||
}
|
||||
}
|
||||
if (newFindings.isNotEmpty) {
|
||||
buffer.writeln('NEW');
|
||||
for (final finding in newFindings.take(top)) {
|
||||
|
|
@ -96,6 +125,15 @@ class VerificationReport {
|
|||
String baselineEntryRule(BaselineEntry entry) =>
|
||||
entry.rule.isNotEmpty ? entry.rule : entry.fingerprint.split('|').first;
|
||||
|
||||
/// The file a baseline entry belongs to, or null when it cannot be told. A
|
||||
/// fingerprints-only baseline has no `file` field; the fingerprint's second `|`
|
||||
/// segment is the file.
|
||||
String? baselineEntryFile(BaselineEntry entry) {
|
||||
if (entry.file.isNotEmpty) return entry.file;
|
||||
final parts = entry.fingerprint.split('|');
|
||||
return parts.length > 1 && parts[1].isNotEmpty ? parts[1] : null;
|
||||
}
|
||||
|
||||
/// Compare a fresh analysis against the accepted baseline.
|
||||
///
|
||||
/// Self-contained: it derives new findings by comparing fingerprints, so it does
|
||||
|
|
@ -104,31 +142,85 @@ String baselineEntryRule(BaselineEntry entry) =>
|
|||
/// When only some rules ran ([rules]), accepted findings of the other rules are
|
||||
/// set aside as out of scope. Comparing them too reported every one of them as
|
||||
/// RESOLVED, because a rule that did not run reports nothing.
|
||||
///
|
||||
/// An accepted finding in a file that was not analysed - one of
|
||||
/// [unscannedFiles], or outside every one of [scannedDirectories] - is reported
|
||||
/// as not scanned. Its absence is not a fix.
|
||||
VerificationReport verifyAgainstBaseline({
|
||||
required FindingReport report,
|
||||
required List<BaselineEntry> baseline,
|
||||
Set<String>? rules,
|
||||
Set<String> unscannedFiles = const {},
|
||||
List<String>? scannedDirectories,
|
||||
}) {
|
||||
final directories = scannedDirectories;
|
||||
bool scanned(BaselineEntry entry) {
|
||||
final file = baselineEntryFile(entry);
|
||||
if (file == null) return true;
|
||||
if (unscannedFiles.contains(file)) return false;
|
||||
if (directories == null) return true;
|
||||
return directories.any(
|
||||
(directory) =>
|
||||
p.equals(directory, '.') ||
|
||||
p.isWithin(p.normalize(directory), p.normalize(file)),
|
||||
);
|
||||
}
|
||||
|
||||
final inScope = [
|
||||
for (final entry in baseline)
|
||||
if (rules == null || rules.contains(baselineEntryRule(entry))) entry,
|
||||
];
|
||||
final present = {for (final finding in report.findings) finding.fingerprint};
|
||||
final accepted = {for (final entry in baseline) entry.fingerprint};
|
||||
final notScanned = <BaselineEntry>[];
|
||||
final resolved = <BaselineEntry>[];
|
||||
final stillPresent = <BaselineEntry>[];
|
||||
for (final entry in inScope) {
|
||||
if (present.contains(entry.fingerprint)) {
|
||||
stillPresent.add(entry);
|
||||
} else if (!scanned(entry)) {
|
||||
notScanned.add(entry);
|
||||
} else {
|
||||
resolved.add(entry);
|
||||
}
|
||||
}
|
||||
return VerificationReport(
|
||||
baselineTotal: inScope.length,
|
||||
outOfScope: baseline.length - inScope.length,
|
||||
resolved: [
|
||||
for (final entry in inScope)
|
||||
if (!present.contains(entry.fingerprint)) entry,
|
||||
],
|
||||
stillPresent: [
|
||||
for (final entry in inScope)
|
||||
if (present.contains(entry.fingerprint)) entry,
|
||||
],
|
||||
resolved: resolved,
|
||||
stillPresent: stillPresent,
|
||||
notScanned: notScanned,
|
||||
newFindings: [
|
||||
for (final finding in report.findings)
|
||||
if (!accepted.contains(finding.fingerprint)) finding,
|
||||
],
|
||||
);
|
||||
}
|
||||
|
||||
/// [verifyAgainstBaseline] over a finished analysis, deriving which files were
|
||||
/// not analysed from the run itself: files the analyzer skipped (excluded by
|
||||
/// analysis_options, or outside any analysis context), files that failed, and
|
||||
/// files discovered but not selected (for example by --no-include-generated).
|
||||
VerificationReport verifyAnalysis({
|
||||
required ProjectAnalysis analysis,
|
||||
required List<BaselineEntry> baseline,
|
||||
Set<String>? rules,
|
||||
}) {
|
||||
final project = analysis.project;
|
||||
final selected = {
|
||||
for (final path in analysis.run.selected) project.relative(path),
|
||||
};
|
||||
final unscanned = <String>{
|
||||
for (final outcome in analysis.run.outcomes)
|
||||
if (!outcome.resolved) project.relative(outcome.path),
|
||||
for (final path in project.dartFiles)
|
||||
if (!selected.contains(project.relative(path))) project.relative(path),
|
||||
};
|
||||
return verifyAgainstBaseline(
|
||||
report: analysis.report,
|
||||
baseline: baseline,
|
||||
rules: rules,
|
||||
unscannedFiles: unscanned,
|
||||
scannedDirectories: project.directories,
|
||||
);
|
||||
}
|
||||
|
|
|
|||
66
test/excluded_files_test.dart
Normal file
66
test/excluded_files_test.dart
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
import 'dart:io';
|
||||
|
||||
import 'package:pipe/pipe.dart';
|
||||
import 'package:test/test.dart';
|
||||
|
||||
void _copyTree(Directory from, Directory to) {
|
||||
to.createSync(recursive: true);
|
||||
for (final entity in from.listSync(recursive: true)) {
|
||||
final relative = entity.path.substring(from.path.length + 1);
|
||||
final target = '${to.path}/$relative';
|
||||
if (entity is Directory) {
|
||||
Directory(target).createSync(recursive: true);
|
||||
} else if (entity is File) {
|
||||
File(target).writeAsBytesSync(entity.readAsBytesSync());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
void main() {
|
||||
test('verify reports findings in an excluded file as not scanned', () async {
|
||||
// The analyzer skips a file that analysis_options `exclude:` names, and the
|
||||
// verify gate only refused FAILED files. So a finding in an excluded file
|
||||
// was absent from the fresh report and verify called it RESOLVED.
|
||||
const excluded = 'lib/item_bloc.dart';
|
||||
final original = await loadProject('test/fixtures/stuck_loading');
|
||||
final before = await analyzeProject(original, concurrency: 1);
|
||||
final encoded = encodeBaseline(original.root, before.report.findings);
|
||||
await before.dispose();
|
||||
final baseline = decodeBaselineEntries(encoded);
|
||||
final inExcluded = baseline.where((entry) => entry.file == excluded);
|
||||
expect(inExcluded, isNotEmpty, reason: 'the fixture must exercise this');
|
||||
|
||||
final temp = Directory.systemTemp.createTempSync('pipe-excluded');
|
||||
_copyTree(Directory('test/fixtures/stuck_loading'), temp);
|
||||
File('${temp.path}/analysis_options.yaml').writeAsStringSync(
|
||||
'analyzer:\n exclude:\n - $excluded\n',
|
||||
);
|
||||
|
||||
final analysis = await analyzeProject(
|
||||
await loadProject(temp.path),
|
||||
concurrency: 1,
|
||||
);
|
||||
try {
|
||||
expect(
|
||||
analysis.run.outcomes.where((outcome) => outcome.skipped),
|
||||
isNotEmpty,
|
||||
reason: 'analysis_options exclude: should skip the file',
|
||||
);
|
||||
final verification = verifyAnalysis(
|
||||
analysis: analysis,
|
||||
baseline: baseline,
|
||||
);
|
||||
expect(
|
||||
verification.resolved.where((entry) => entry.file == excluded),
|
||||
isEmpty,
|
||||
);
|
||||
expect(
|
||||
verification.notScanned.map((entry) => entry.fingerprint),
|
||||
containsAll(inExcluded.map((entry) => entry.fingerprint)),
|
||||
);
|
||||
expect(verification.allResolved, isFalse);
|
||||
} finally {
|
||||
await analysis.dispose();
|
||||
}
|
||||
});
|
||||
}
|
||||
|
|
@ -118,6 +118,15 @@ void main() {
|
|||
},
|
||||
);
|
||||
|
||||
test('files excluded by analysis_options make a scan partial too', () {
|
||||
// A skipped file was never examined, exactly like a failed one; naming
|
||||
// only failures let a scan with excluded files read as complete.
|
||||
final warning = partialScanWarning(failed: 0, resolved: 11, skipped: 2);
|
||||
expect(warning, isNotNull);
|
||||
expect(warning, contains('2 file(s) were not analysed'));
|
||||
expect(warning, contains('PARTIAL'));
|
||||
});
|
||||
|
||||
test('verify refuses a partial scan instead of claiming fixes', () async {
|
||||
final runner = PipeRunner();
|
||||
await expectLater(
|
||||
|
|
@ -195,6 +204,15 @@ void main() {
|
|||
},
|
||||
);
|
||||
|
||||
test('files excluded by analysis_options make a scan partial too', () {
|
||||
// A skipped file was never examined, exactly like a failed one; naming
|
||||
// only failures let a scan with excluded files read as complete.
|
||||
final warning = partialScanWarning(failed: 0, resolved: 11, skipped: 2);
|
||||
expect(warning, isNotNull);
|
||||
expect(warning, contains('2 file(s) were not analysed'));
|
||||
expect(warning, contains('PARTIAL'));
|
||||
});
|
||||
|
||||
test('a run where every file fails to resolve is an error', () async {
|
||||
// An unreadable file comes back from the analyzer as a unit with EMPTY
|
||||
// content rather than an error, so it used to look like a successful
|
||||
|
|
|
|||
|
|
@ -176,4 +176,47 @@ void main() {
|
|||
expect(verification.describe(), isNot(contains('confirmed')));
|
||||
expect(verification.describe(), contains('rules not run'));
|
||||
});
|
||||
|
||||
test('a baselined finding in an unscanned file is not resolved', () {
|
||||
// Absent from the report because nothing read its file - excluded by
|
||||
// analysis_options, or outside the scanned --dir - is not the same as
|
||||
// fixed, so it must not be counted as a confirmed fix.
|
||||
final outsideDirs = _finding(
|
||||
rule: 'dead-handler',
|
||||
file: 'test/helpers.dart',
|
||||
line: 3,
|
||||
data: const {'event': 'OnlyInTests'},
|
||||
);
|
||||
final report = FindingReport(
|
||||
root: '/root',
|
||||
findings: const [],
|
||||
suppressed: const [],
|
||||
);
|
||||
BaselineEntry entryOf(Finding finding, String label) => BaselineEntry(
|
||||
fingerprint: finding.fingerprint,
|
||||
rule: finding.rule,
|
||||
file: finding.file!,
|
||||
line: finding.line,
|
||||
label: label,
|
||||
);
|
||||
final verification = verifyAgainstBaseline(
|
||||
report: report,
|
||||
baseline: [
|
||||
entryOf(present, 'StillBroken'),
|
||||
entryOf(fixed, 'loadNow'),
|
||||
entryOf(outsideDirs, 'OnlyInTests'),
|
||||
],
|
||||
unscannedFiles: {'lib/b.dart'},
|
||||
scannedDirectories: ['lib'],
|
||||
);
|
||||
|
||||
expect(verification.resolved.map((e) => e.label), ['StillBroken']);
|
||||
expect(
|
||||
verification.notScanned.map((e) => e.label),
|
||||
unorderedEquals(['loadNow', 'OnlyInTests']),
|
||||
);
|
||||
expect(verification.allResolved, isFalse);
|
||||
expect(verification.toJson(), containsPair('notScanned', 2));
|
||||
expect(verification.describe(), contains('NOT SCANNED'));
|
||||
});
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue