Agent Manager commited on
Commit
dcfa89e
·
1 Parent(s): cf4bd43

Review: report what discovery skipped, wherever it stopped

Browse files

Three of the four findings on #91, all in the runner.

A `*.test.mjs` below `test/` or the package root was ignored in silence — the
same coverage-goes-quiet failure this script exists to end, arriving by a
different route. Discovery still stops at those two depths (so `test/fixtures/`
stays fixtures), but anything suite-shaped underneath is now listed at the end
of every run with the three ways out: move it up, mark it manual, or rename it.
Verified with the reviewer's own probe — a `process.exit(9)` in
`server/test/fixtures/nested.test.mjs` is named now instead of vanishing.

The skip list only printed when the run passed: `process.exit(code)` returned
before it. Both exits now go through one `report()`, so "here is what did not
run" survives the moment someone is actually reading the output.

`npm test -- reader-info` said no suites matched, when in fact one matched and
was deliberately excluded — the filter is advertised as the run-it-by-hand path
and the manual suites are exactly the ones worth running that way. It now names
what matched, why it is held back, and offers `--manual`, which lets an explicit
filter reach them. A bare filter still cannot drag a Chromium suite in.

The fourth finding (the runner missing from the image) does not reproduce:
Dockerfile:159 has copied the whole scripts/ directory since #10, and
/app/scripts in the running Space holds all eight files. Answered on the PR.

Files changed (1) hide show
  1. scripts/run-suites.mjs +60 -9
scripts/run-suites.mjs CHANGED
@@ -27,10 +27,19 @@
27
  // OPTING OUT. A suite that must not run in the default set says so in its own
28
  // header, on a line containing `am-test: manual` plus the reason. It is declared
29
  // where a reader will see it rather than by absence from a list somewhere else,
30
- // and every run prints what it skipped and why, so coverage cannot go quiet.
 
31
  //
32
- // Usage: node ../scripts/run-suites.mjs [substring …]
33
- // (a substring filters to matching suites for running one by hand)
 
 
 
 
 
 
 
 
34
  import fs from 'node:fs';
35
  import path from 'node:path';
36
  import { spawnSync } from 'node:child_process';
@@ -53,7 +62,10 @@ const listDir = (dir) => {
53
  // test/) at the front of the run.
54
  const found = [...listDir('test'), ...listDir('.')];
55
 
56
- const filters = process.argv.slice(2);
 
 
 
57
  const suites = [];
58
  const skipped = [];
59
  for (const file of found) {
@@ -65,17 +77,55 @@ for (const file of found) {
65
  fs.closeSync(fd);
66
  } catch { /* unreadable: let node report it */ }
67
  const manual = head.match(MANUAL);
68
- if (manual) { skipped.push({ file, why: manual[1].trim() }); continue; }
69
- if (filters.length && !filters.some((f) => file.includes(f))) continue;
 
 
 
 
 
70
  suites.push(file);
71
  }
72
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
73
  const pkg = path.basename(process.cwd());
74
  if (!suites.length) {
75
- console.error(`no suites found in ${pkg}/${filters.length ? ` matching ${filters.join(', ')}` : ''}`);
 
 
 
 
 
 
 
76
  process.exit(1);
77
  }
78
- const plural = (n) => `${n} suite${n === 1 ? '' : 's'}`;
79
  console.log(`${pkg}: ${plural(suites.length)}\n`);
80
 
81
  for (const [i, file] of suites.entries()) {
@@ -85,10 +135,11 @@ for (const [i, file] of suites.entries()) {
85
  if (code !== 0) {
86
  console.error(`\n${file} FAILED (${r.signal ? `signal ${r.signal}` : `exit ${code}`})`);
87
  console.error(`${plural(i)} had passed before it; the rest were not started.`);
 
88
  process.exit(code);
89
  }
90
  console.log('');
91
  }
92
 
93
  console.log(`${pkg}: ${plural(suites.length)} passed`);
94
- for (const { file, why } of skipped) console.log(` skipped ${file} — ${why || 'marked manual'}`);
 
27
  // OPTING OUT. A suite that must not run in the default set says so in its own
28
  // header, on a line containing `am-test: manual` plus the reason. It is declared
29
  // where a reader will see it rather than by absence from a list somewhere else,
30
+ // and every run prints what it skipped and why on the failure path too, since
31
+ // that is the moment someone is actually reading this output.
32
  //
33
+ // DISCOVERY IS TWO DEEP, ON PURPOSE: `test/` and the package root, so a
34
+ // `test/fixtures/` directory is fixtures rather than a source of surprise runs.
35
+ // A `*.test.mjs` anywhere below that is reported at the end of every run instead
36
+ // of being ignored — a file that looks like a suite and never runs is the exact
37
+ // failure this script exists to end, and it does not matter that the cause is a
38
+ // subdirectory rather than a hand-edited list.
39
+ //
40
+ // Usage: node ../scripts/run-suites.mjs [substring …] [--manual]
41
+ // a substring filters to matching suites — for running one by hand;
42
+ // --manual lets that filter reach the suites marked manual.
43
  import fs from 'node:fs';
44
  import path from 'node:path';
45
  import { spawnSync } from 'node:child_process';
 
62
  // test/) at the front of the run.
63
  const found = [...listDir('test'), ...listDir('.')];
64
 
65
+ const args = process.argv.slice(2);
66
+ const wantManual = args.includes('--manual');
67
+ const filters = args.filter((a) => !a.startsWith('--'));
68
+ const matches = (file) => !filters.length || filters.some((f) => file.includes(f));
69
  const suites = [];
70
  const skipped = [];
71
  for (const file of found) {
 
77
  fs.closeSync(fd);
78
  } catch { /* unreadable: let node report it */ }
79
  const manual = head.match(MANUAL);
80
+ // Named explicitly with --manual, a manual suite runs: the filter is the
81
+ // run-one-by-hand path, and the suites worth running by hand are mostly these.
82
+ if (manual && !(wantManual && filters.length && matches(file))) {
83
+ if (matches(file)) skipped.push({ file, why: manual[1].trim() });
84
+ continue;
85
+ }
86
+ if (!matches(file)) continue;
87
  suites.push(file);
88
  }
89
 
90
+ // Anything that looks like a suite but sits below the two scanned depths.
91
+ const stray = [];
92
+ (function walk(dir, depth) {
93
+ let entries = [];
94
+ try { entries = fs.readdirSync(dir, { withFileTypes: true }); } catch { return; }
95
+ for (const e of entries) {
96
+ const p = path.join(dir, e.name);
97
+ if (e.isDirectory()) {
98
+ if (['node_modules', '.git', 'dist', 'coverage'].includes(e.name)) continue;
99
+ walk(p, depth + 1);
100
+ } else if (e.name.endsWith('.test.mjs') && depth > 0 && path.dirname(p) !== 'test') {
101
+ stray.push(p);
102
+ }
103
+ }
104
+ }('.', 0));
105
+
106
+ const plural = (n) => `${n} suite${n === 1 ? '' : 's'}`;
107
+ // Printed by BOTH exits. What did not run is most worth saying when something
108
+ // failed, and that is exactly when an early `process.exit` used to swallow it.
109
+ const report = () => {
110
+ for (const { file, why } of skipped) console.log(` skipped ${file} — ${why || 'marked manual'}`);
111
+ for (const file of stray) {
112
+ console.log(` NOT RUN ${file} — below \`test/\` and the package root, where discovery looks.`);
113
+ console.log(' Move it up, or mark it `am-test: manual` with a reason, or rename it.');
114
+ }
115
+ };
116
+
117
  const pkg = path.basename(process.cwd());
118
  if (!suites.length) {
119
+ // Blaming the filter here sends people looking for a typo when the file was
120
+ // found and deliberately excluded.
121
+ const manualOnly = skipped.length && filters.length;
122
+ console.error(manualOnly
123
+ ? `${pkg}/: ${plural(skipped.length)} matched ${filters.join(', ')}, all marked manual:`
124
+ : `no suites found in ${pkg}/${filters.length ? ` matching ${filters.join(', ')}` : ''}`);
125
+ report();
126
+ if (manualOnly) console.error(`Run one anyway with: npm test -- ${filters.join(' ')} --manual`);
127
  process.exit(1);
128
  }
 
129
  console.log(`${pkg}: ${plural(suites.length)}\n`);
130
 
131
  for (const [i, file] of suites.entries()) {
 
135
  if (code !== 0) {
136
  console.error(`\n${file} FAILED (${r.signal ? `signal ${r.signal}` : `exit ${code}`})`);
137
  console.error(`${plural(i)} had passed before it; the rest were not started.`);
138
+ report();
139
  process.exit(code);
140
  }
141
  console.log('');
142
  }
143
 
144
  console.log(`${pkg}: ${plural(suites.length)} passed`);
145
+ report();