Skip to content

Commit 8cd500c

Browse files
Ahad690claude
andcommitted
fix(ci): stop the coverage step hiding test failures
GitHub's default Linux shell is 'bash -e {0}', which has no pipefail. So 'npm run test:coverage | tee coverage.txt' returned tee's exit code, and a failing suite reported success. A green job had 1 of 157 tests failing in it - 'port collision retries +1', a test asserting the auto-relocate behaviour that was intentionally removed a commit earlier. 'shell: bash' is 'bash -eo pipefail', so the pipeline now propagates the test result. This has been latent since the tee was introduced. Also fixed the stale test itself to assert the new contract (fail loudly by default, relocate only on opt-in), and wrapped it in try/finally: a thrown assert left the squatter's server handle open, which timed out the whole file and buried the real failure under an unrelated one. The CLI suite needed more than the 20s per-test budget, so it now has 60s. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 74c85a5 commit 8cd500c

4 files changed

Lines changed: 34 additions & 15 deletions

File tree

‎.github/workflows/gateway.yml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,17 @@ jobs:
3434

3535
- name: Run test suite with coverage
3636
working-directory: gateway
37+
# shell: bash, not the default. The default Linux shell is `bash -e {0}`,
38+
# which has NO pipefail - so `... | tee coverage.txt` returns tee's exit
39+
# code and a failing suite still reports success. That happened: a green
40+
# job with 1 of 157 tests failing. `shell: bash` is `bash -eo pipefail`.
41+
shell: bash
3742
run: npm run test:coverage 2>&1 | tee coverage.txt
3843

3944
- name: Enforce coverage floor
4045
working-directory: gateway
41-
# line 85 / branch 80 / funcs 80. Current: 92.70 / 84.53 / 88.64.
46+
# line 85 / branch 80 / funcs 80. Current: 94.24 / 83.51 / 90.73.
47+
shell: bash
4248
run: node scripts/check-coverage.cjs coverage.txt 85 80 80
4349

4450
- name: Upload coverage report

‎gateway/bin/proxima-gw.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ function table(rows, headers) {
174174

175175
async function cmdCheck() {
176176
const { port, via } = resolvePort();
177-
const res = await call('ping', null, {}, 4000);
177+
const res = await call('ping', null, {}, 2500);
178178
if (res.ok) {
179179
say(`gateway RUNNING on 127.0.0.1:${port} (port from ${via})`);
180180
return EXIT.OK;
@@ -188,7 +188,7 @@ async function cmdCheck() {
188188
}
189189

190190
async function cmdStatus() {
191-
const health = await call('ping', null, {}, 4000);
191+
const health = await call('ping', null, {}, 2500);
192192
if (!health.ok) {
193193
fail(`Gateway not reachable: ${health.error}`);
194194
fail('Start it with: node src\\index.js');

‎gateway/package.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@
77
"main": "src/index.js",
88
"scripts": {
99
"start": "node src/index.js",
10-
"test": "node --test --test-reporter=tap --test-timeout=20000 test/unit.test.js test/integration.test.js test/contract.test.js test/redact.test.js test/leak.test.js test/engines.test.js test/regression.test.js test/cli.test.js",
11-
"test:coverage": "node --test --test-reporter=tap --experimental-test-coverage --test-timeout=20000 test/unit.test.js test/integration.test.js test/contract.test.js test/redact.test.js test/leak.test.js test/engines.test.js test/regression.test.js test/cli.test.js",
10+
"test": "node --test --test-reporter=tap --test-timeout=60000 test/unit.test.js test/integration.test.js test/contract.test.js test/redact.test.js test/leak.test.js test/engines.test.js test/regression.test.js test/cli.test.js",
11+
"test:coverage": "node --test --test-reporter=tap --experimental-test-coverage --test-timeout=60000 test/unit.test.js test/integration.test.js test/contract.test.js test/redact.test.js test/leak.test.js test/engines.test.js test/regression.test.js test/cli.test.js",
1212
"test:browser": "node --test --test-reporter=tap --test-timeout=60000 test/browser.test.js",
1313
"test:all": "npm run test:coverage && npm run test:browser",
1414
"coverage:check": "node scripts/check-coverage.cjs",

‎gateway/test/unit.test.js‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -233,18 +233,31 @@ test('server refuses a non-loopback bind (P7)', () => {
233233
);
234234
});
235235

236-
test('port collision retries +1 then reports both ports (F2)', async () => {
236+
test('a port collision fails loudly rather than relocating (F2)', async () => {
237237
const s = new StateStore(tmpDir());
238238
const squatter = new IpcServer({ handler: async () => ({ success: true }), stateStore: s, port: 0 });
239-
const taken = await squatter._tryListen(0);
240-
const takenPort = squatter.server.address().port;
241-
242-
const second = new IpcServer({ handler: async () => ({ success: true }), stateStore: s, port: takenPort });
243-
const bound = await second.listen();
244-
assert.equal(bound, takenPort + 1, 'must land on the next port');
245-
246-
await second.close();
247-
await squatter.close();
239+
const second = new IpcServer({ handler: async () => ({ success: true }), stateStore: s, port: 0 });
240+
241+
try {
242+
await squatter._tryListen(0);
243+
const takenPort = squatter.server.address().port;
244+
const colliding = new IpcServer({ handler: async () => ({ success: true }), stateStore: s, port: takenPort });
245+
246+
// Relocation is opt-in. Silently moving to port+1 would leave a fact pointing
247+
// a client at whichever process won the real port.
248+
await assert.rejects(() => colliding.listen(), /already in use/);
249+
250+
// Opting in relocates, and is explicit about it.
251+
const moved = await colliding.listen({ allowRelocate: true });
252+
assert.equal(moved, takenPort + 1);
253+
254+
await colliding.close();
255+
} finally {
256+
// Without this the squatter's handle stays open and the whole test FILE times
257+
// out, which masks the real failure behind an unrelated one.
258+
await second.close();
259+
await squatter.close();
260+
}
248261
});
249262

250263
// ---------------------------------------------------------------- engines

0 commit comments

Comments
 (0)