Repository navigation
test: add deployed HTTP and WebSocket smoke checks - #355
Conversation
|
@krsnak Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
The smoke check itself is very good, and better than what I asked for.
It asserts freshness, not presence — fresh(data.ingestLagSeconds), fresh(data.lastTradeAgeSeconds), and a rejection if stale !== false. That is the distinction that makes a smoke check worth running: a service returning a confidently wrong three-day-old price passes every "is it up" probe ever written.
The websocket round-trip is real too — correct graphql-transport-ws subprotocol, connection_init and a wait for connection_ack before subscribing, a handshake timeout, a close handler that rejects rather than hanging, and clearTimeout + terminate() in a finally. Plenty of "smoke tests" open a socket, see it connect and call that a pass.
And scripts/test-deployed-smoke.mjs — a test for the smoke script — is the detail I did not expect. A check that silently passes because of a bug in the checker is the worst outcome available here.
One change: /ready belongs to someone else, and that is my fault
This PR adds the route:
app.get('/ready', { config: { public: true } }, async (_req, reply) => {
try { await pgPool.query('SELECT 1'); return { ok: true } }
catch { return reply.status(503).send({ ok: false }) }
})#334 — "There is no readiness check, only a status page" — is open and assigned to @CollinsKRO, and asks for /live and /ready, both excluded from rate limiting and from httpMetrics route cardinality, with /live deliberately not touching Postgres so a database blip cannot restart healthy processes in a loop.
You added it because my acceptance criteria on #346 said the smoke check should assert /status and /ready were healthy — I wrote that while #334 was still unwritten and did not reconcile the two. So this is my error, not scope creep on your part.
The cleanest resolution:
- Drop the
/readyroute and its openapi entries from this PR. Leave #334 to deliver it properly, with/livealongside it. - Make the smoke check tolerant of it being absent: treat a 404 on
/readyas skipped rather than failed, and assert it properly once #334 lands. A smoke check that fails against today's production for a route that does not exist yet is not usable today, which defeats the point.
Keep everything else exactly as it is.
Small note
fresh() rejects age > maxAge but also age < 0, which is right — a negative age means clock skew between the service and the database, and silently accepting it would mask a real problem.
Drop the route, make /ready skippable, and I will merge this straight away.
|
Addressed the review: removed the /ready route and related OpenAPI/test additions, and made the deployed smoke check skip only HTTP 404 for /ready while still failing on other readiness errors. Added a regression test for the 404 case. Validation: npm run test:smoke (6/6 passed), npm run build (passed), git diff --check (passed). Ready for another look, thanks! |
Miracle656
left a comment
There was a problem hiding this comment.
Exactly right on both counts.
The /ready route and its openapi entries are gone, so #334 is untouched and still entirely @CollinsKRO's. And the smoke check handles its absence properly:
if (response.status === 404) {
console.log('SKIP /ready: route not available yet (#334)')
return
}A skip that says why it skipped and points at the issue is the difference between a check that degrades gracefully and one that quietly stops testing something. When #334 lands this starts asserting on its own, with no change needed here.
Everything I praised the first time still stands — freshness rather than presence, a real graphql-transport-ws round-trip with the handshake, and a test for the smoke script itself so a bug in the checker cannot produce a silent pass.
Merging.
Fixes #346\n\nAdds a single-command deployed smoke check for /status, /ready, a fresh validated XLM/USDC price, and a real GraphQL WebSocket subscription update. Includes timeout and failure diagnostics, regression tests, and README usage.\n\nValidation: npm run test:smoke (5 passed); npm run build.