test(rivetkit): re-enable gateway URL and direct-registry coverage#4659
test(rivetkit): re-enable gateway URL and direct-registry coverage#4659NathanFlurry wants to merge 1 commit intobreak-up/keep-error-exposure-consistentfrom
Conversation
ecf268c to
4c4e267
Compare
ccc38b5 to
8293235
Compare
|
PR Review (updated): test(rivetkit): re-enable gateway URL and direct-registry coverage This is an updated review reflecting the latest changes. Summary This PR re-enables two test suites (raw-http-direct-registry and raw-websocket-direct-registry), rewrites driver-engine-ping.test.ts to use the serverless runner pattern, adds a waitForClientWarmup readiness barrier, adds retry logic to refreshRunnerMetadata, re-enables the dynamic registry test variant, and switches engine driver tests to describe.sequential. Significant: Silent swallow of refreshRunnerMetadata errors In driver-engine.test.ts (~line 196), all errors from refreshRunnerMetadata are caught and discarded. A permanent failure (wrong token, misconfigured endpoint) will be invisible and any downstream test failure will be misattributed. Consider logging the caught error at warn level so the CI trace retains a breadcrumb without blocking setup. Significant: Non-null assertion on serverlessHandleStart In src/test/mod.ts (~line 168) and driver-engine-ping.test.ts, the call actorDriver.serverlessHandleStart!(ctx) will throw an unhelpful TypeError if the driver does not implement this method. A guard returning HTTP 501 with a clear message would make failures immediately actionable. Moderate: Duplicated helpers across test files refreshRunnerMetadata and closeNodeServer appear in src/test/mod.ts, driver-engine.test.ts, and driver-engine-ping.test.ts. waitForOpen appears in driver-engine-ping.test.ts and raw-websocket-direct-registry.ts with differing listener cleanup quality (ping file properly cleans up; WebSocket file uses once:true without cleaning the other paths). buildGatewayRequestUrl appears in raw-http-direct-registry.ts and driver-engine-ping.test.ts with differing double-prefix guards. These should be consolidated into a shared tests/engine-test-utils.ts. Moderate: serverless config block duplicated verbatim The serverless runner config block is duplicated between src/test/mod.ts (~line 185) and driver-engine-ping.test.ts (~line 1665). Extracting it into a shared constant prevents the copies from diverging silently. Minor: waitForClientWarmup uses a fixed key across parallel tests The constant key driver-test-warmup works today because each test has its own namespace/pool, but a per-test ID would make isolation explicit and robust against future config changes. Minor: Retry sleep uses raw setTimeout instead of antiox Per CLAUDE.md, antiox is the preferred TypeScript concurrency primitive. The retry loops in refreshRunnerMetadata use raw setTimeout directly. Minor: Missing conn_params test in the HTTP gateway suite raw-http-direct-registry.ts does not include a conn_params test (the old code had one via HEADER_CONN_PARAMS). The WebSocket suite does test conn params. If the HTTP gateway path supports conn_params, this coverage gap should be tracked. Positive Observations
Review generated by Claude Sonnet 4.6 |
PR 4659 Review: test(rivetkit) re-enable gateway URL and direct-registry coverageOverviewThis PR adds two new test scenarios to the engine driver smoke test suite and a minimal
IssuesUnexplained initial-message wait before sending pingIn the new WebSocket gateway test, an Suggestion: Add a comment such as Listener registered after send (pre-existing, worth noting)In both the existing and new WebSocket tests, the No cleanup on failure
warmupActor purpose is unclear
Minor Observations
SummaryThe main concern is the unexplained initial-message await in the WebSocket gateway test. If the actor/gateway does not send a frame on connect, the test will hang. The other points (listener ordering, missing |
8293235 to
eeabed8
Compare
4c4e267 to
e823f78
Compare
cde8213 to
8cf4223
Compare
6cbcfce to
c4f0322
Compare
6987ecc to
d17056c
Compare
c4f0322 to
6b63eed
Compare
d17056c to
df32e78
Compare
6b63eed to
392102e
Compare
392102e to
e81836a
Compare
e5ca28f to
49982a7
Compare
49982a7 to
e6b5003
Compare
079403b to
eb49dfb
Compare
|
Landed in main via stack-merge fast-forward push. Commits are in main; closing to match. |

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: