diff --git a/AGENTS.md b/AGENTS.md index 788a963..0f6aa42 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -155,10 +155,14 @@ exactly 140. - `message_id` values the library generates are UUID v7. - A test that needs a dummy peer must `resume()` its sockets. An unread socket never processes the peer's FIN, so `server.close()` hangs forever — that is a test bug, not a library one. -- Everything a test opens is handed to `test/teardown.ts` as it is opened, never closed on the test's +- Everything a test opens gets its teardown registered as it is opened, never closed on the test's last line: an assertion that throws skips that line, and the listener it leaves behind keeps - `node --test` alive until CI's ten-minute cap. The teardown aborts rather than drains, so a test - that fails holding the send window still ends. + `node --test` alive until CI's ten-minute cap. `test/teardown.ts` covers a session, a server and a + listener; anything else takes a bare `t.after`. Its close aborts rather than drains, so a test that + fails holding the send window still ends. +- `t.after` hooks run in registration order, so registering at creation tears the outermost resource + down first — the wrong way round for a listener, which blocks until every connection on it is gone. + A listener's own hook goes last, after the hooks that close what is connected to it. - `assert.equal` from `node:assert/strict` narrows its first argument, so a following `?.` on the same value is flagged as unnecessary. Assert once with `assert.ok(x)` and use plain access after. diff --git a/test/interop.test.ts b/test/interop.test.ts index 5569a94..dbefb68 100644 --- a/test/interop.test.ts +++ b/test/interop.test.ts @@ -227,7 +227,6 @@ describe('a live session against the reference implementation', () => { }); }); - t.after(() => new Promise(resolve => { refServer.close(() => { resolve(); }); })); await new Promise(resolve => { refServer.listen(0, () => { resolve(); }); }); const port = refServer.address()?.port ?? 0; @@ -236,6 +235,7 @@ describe('a live session against the reference implementation', () => { assert.equal(err, undefined); assert.ok(session); closeAfter(t, session); + t.after(() => new Promise(resolve => { refServer.close(() => { resolve(); }); })); const sent = await session.sendSms({ from: 'MyBrand', diff --git a/test/readme.test.ts b/test/readme.test.ts index 4406733..23b9b7b 100644 --- a/test/readme.test.ts +++ b/test/readme.test.ts @@ -173,6 +173,7 @@ describe('README: Server', () => { assert.equal(clientErr, undefined); assert.ok(session); + closeAfter(t, session); await session.sendSms({ from: '46701113311', message: 'Hello world', to: '46709771337' }); await session.unbind(); @@ -212,6 +213,7 @@ describe('README: Server', () => { assert.equal(clientErr, undefined); assert.ok(session); + closeAfter(t, session); const reported = once(resolve => { session.on('dlr', resolve); }); const sent = await session.sendSms({ @@ -223,8 +225,6 @@ describe('README: Server', () => { assert.equal(sent.err, undefined); assert.equal((await reported).statusMsg, 'DELIVERED'); - - await session.unbind(); }); }); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index a7d3891..62ec25a 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -16,7 +16,7 @@ import { Reassembler, decodeSegments } from '../src/reassembly.ts'; import { Session } from '../src/session.ts'; import { DlrMerger } from '../src/dlr-merger.ts'; import { client } from '../src/client.ts'; -import { closeAfter, endListenerAfter } from './teardown.ts'; +import { closeAfter, closeListenerAfter } from './teardown.ts'; import { consts } from '../src/defs/constants.ts'; import { errors } from '../src/defs/errors.ts'; import { objToPdu } from '../src/pdu.ts'; @@ -575,7 +575,7 @@ describe('AbortSignal on a send', () => { }); await new Promise(resolve => silent.listen(0, resolve)); - endListenerAfter(t, silent, accepted); + closeListenerAfter(t, silent, accepted); const address = silent.address(); const port = typeof address === 'object' && address !== null ? address.port : 0; @@ -711,7 +711,7 @@ describe('graceful shutdown', () => { }); test('stops accepting the moment close() is called, not when the drain ends', async t => { - const smpp = await startServer(t, { shutdownTimeout: 30_000 }); + const smpp = await startServer(t, { shutdownTimeout: 500 }); const arrived = once(resolve => { smpp.on('session', resolve); }); const silent = net.connect({ port: smpp.port }); @@ -758,7 +758,8 @@ describe('graceful shutdown', () => { }); test('does not report a reconnect on a session closed while it was coming back up', async t => { - const listener = net.createServer(sock => { sock.resume(); }); + const accepted: net.Socket[] = []; + const listener = net.createServer(sock => { accepted.push(sock); sock.resume(); }); await new Promise(resolve => { listener.listen(0, resolve); }); @@ -793,7 +794,8 @@ describe('graceful shutdown', () => { const reported: string[] = []; closeAfter(t, session); - endListenerAfter(t, listener, opened); + t.after(() => { for (const sock of opened) sock.destroy(); }); + closeListenerAfter(t, listener, accepted); session.on('reconnected', () => reported.push('reconnected')); first.sock.destroy(); diff --git a/test/session.test.ts b/test/session.test.ts index e2de697..d42636d 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -12,7 +12,7 @@ import { PduFramer } from '../src/pdu-framer.ts'; import { ReconnectLoop } from '../src/reconnect-loop.ts'; import { Session, bindCommands } from '../src/session.ts'; import { client } from '../src/client.ts'; -import { closeAfter, endListenerAfter } from './teardown.ts'; +import { closeAfter, closeListenerAfter } from './teardown.ts'; import { consts } from '../src/defs/constants.ts'; import { isCommand, objToPdu, pduReturn, pduToObj } from '../src/pdu.ts'; import { paramText } from '../src/defs/types.ts'; @@ -316,9 +316,6 @@ describe('bind', () => { assert.ok(byDefault); assert.ok(asFive); assert.deepEqual(declared, [0x34, 0x50]); - - await byDefault.unbind(); - await asFive.unbind(); }); test('tells a 3.4 peer the version it supports in the bind response', async t => { @@ -472,12 +469,13 @@ describe('bind direction', () => { assert.ok(session); const [sms] = await Promise.all([ - incoming, + incoming.then(async received => { + await received.sendResp(); + + return received; + }), session.sendSms({ dlr: true, from: '46701113311', message: 'one way', to: '46709771337' }), ]); - - await sms.sendResp(); - const report = await sms.sendDlr(); assert.ok(report.err instanceof Error); @@ -606,7 +604,11 @@ describe('sending', () => { assert.ok(session); const [sms] = await Promise.all([ - incoming, + incoming.then(async received => { + await received.sendResp(); + + return received; + }), session.sendSms({ destinationAddrNpi: consts.NPI.ISDN, destinationAddrTon: consts.TON.NATIONAL, @@ -929,7 +931,7 @@ describe('robustness', () => { const silent = net.createServer(sock => { accepted.push(sock); sock.resume(); }); await new Promise(resolve => silent.listen(0, resolve)); - endListenerAfter(t, silent, accepted); + closeListenerAfter(t, silent, accepted); const address = silent.address(); const port = typeof address === 'object' && address !== null ? address.port : 0; @@ -1269,13 +1271,12 @@ describe('application hooks that throw or reject', () => { smpp.on('session', () => Promise.reject(new Error('session listener rejected'))); - const { session } = await connect(t, smpp); + await connect(t, smpp); + const reported = await raceWithin(500, failed); assert.ok(reported instanceof Error, 'a rejecting session listener should reach the server'); assert.equal(reported.message, 'session listener rejected'); - - await session?.close(); }); test('closes even when an application close listener throws', async t => { diff --git a/test/teardown.ts b/test/teardown.ts index 7c7181f..70632b6 100644 --- a/test/teardown.ts +++ b/test/teardown.ts @@ -9,13 +9,19 @@ export function closeAfter(t: TestContext, closable: Closable): void { t.after(() => closable.close({ signal: AbortSignal.abort() })); } -/** A listener with a socket still on it never closes, so the sockets go first. */ -export function endListenerAfter(t: TestContext, listener: Server, sockets: Socket[]): void { +/** + * Stops a listener, given every socket it accepted — one still open and it never closes. The wait is + * bounded because a caller that misses one must not hang the net itself. + */ +export function closeListenerAfter(t: TestContext, listener: Server, accepted: Socket[]): void { t.after(async () => { - for (const sock of sockets) { + for (const sock of accepted) { sock.destroy(); } - await new Promise(resolve => { listener.close(() => { resolve(); }); }); + await Promise.race([ + new Promise(resolve => { listener.close(() => { resolve(); }); }), + new Promise(resolve => { setTimeout(resolve, 1000).unref(); }), + ]); }); } diff --git a/todo.md b/todo.md index f9b1e0c..5bb0335 100644 --- a/todo.md +++ b/todo.md @@ -57,7 +57,6 @@ Rules the API follows: | Merged multipart DLRs including across a reconnect, reassembly bounds, per-send abort, the segment cap | `test/session-extras.test.ts` | | A draining `close()` and `unbind()`, bounded by `shutdownTimeout` or an abort | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | -| Teardown that survives a failing assertion, so a broken test reports rather than hangs | `test/teardown.ts` | | Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | | A listener that throws, or rejects, reaching `sessionError`/`serverError` rather than the process | `test/session.test.ts`, `test/error-from.test.ts` | | Cross-checked against node-smpp both ways and over a live session | `test/interop.test.ts` |