test: Make node integration test runner more resilient #14245
Conversation
mydea
force-pushed
the
fn/node-test-runner
branch
from
7c5ecba
to
462ae64
Compare
|
Update on this: While working on this, I found some further, actually more serious problems - some tests were simply not running at all. Especially all the session aggregrate tests were simply not being run and just "passed" accidentally. The core problem there was that the path to the scenario was incorrect, so nothing was even started, plus some things where we seemed to catch errors and still pass (?? I do not think I fixed all of the issues there, but at least some of them...) Now, we assert that the scenario file actually exists. Plus, instead of setting |
| }); | ||
|
|
||
| const runner = createRunner(__dirname, 'server.ts') | ||
| test('should aggregate successful and crashed sessions', done => { |
There was a problem hiding this comment.
This and the other tests below were simply not doing anything at all, if you look through it in detail you see a bunch of things that can't work:
- the path to the server.ts is wrong
- The route paths are wrong
| @@ -52,4 +50,4 @@ app.get('/test/error_handled', (_req, res) => { | |||
|
|
|||
| Sentry.setupExpressErrorHandler(app); | |||
|
|
|||
| export default app; | |||
| startExpressServerAndSendPortToRunner(app); | |||
There was a problem hiding this comment.
Also this scenario file could not work at all and needed adjustment.
| @@ -1,5 +0,0 @@ | |||
| import express from 'express'; | |||
There was a problem hiding this comment.
This was not actually used anymore anwhere.
Oops, #14245 broke our remix integration tests. Instead of re-exporting this from a path (which is not resolved nicely etc. and not reflected by our dependency graph) we can just inline the two methods we need here, they are not too complicated.
Noticed while debugging some test problems, that if axios throws an error (e.g. you get a 500 error), jest cannot serialize the error because it contains recursive data, leading to super hard to debug error messages.
This makes our node integration test runner more resilient by normalizing errors we get there to ensure circular references are resolved.
Update
While working on this, I found some further, actually more serious problems - some tests were simply not running at all. Especially all the session aggregrate tests were simply not being run and just "passed" accidentally.
The core problem there was that the path to the scenario was incorrect, so nothing was even started, plus some things where we seemed to catch errors and still pass (?? I do not think I fixed all of the issues there, but at least some of them...)
Now, we assert that the scenario file actually exists. Plus, instead of setting
.expectError()on the whole runner, you now have to pass this to a specificmakeRequestcall as an optional option, and it will also assert if we expect an error but do not get one. This way, the tests can be more explicit and clear in what they do.