test: Fix node-integration-test timeouts & cleanup #13280
Conversation
mydea
changed the title
| @@ -38,14 +38,6 @@ requests, and assertions. Test server, interceptors and assertions are all run o | |||
|
|
|||
| `utils/` contains helpers and Sentry-specific assertions that can be used in (`test.ts`). | |||
|
|
|||
| `TestEnv` class contains methods to create and execute requests on a test server instance. `TestEnv.init()` which starts | |||
There was a problem hiding this comment.
This is not actually used anywhere anymore, so removed this.
There was a problem hiding this comment.
or, actually moved it to the remix package, where it is used/needed.
| // Increases test timeout from 5s to 45s | ||
| jest.setTimeout(45000); | ||
| // Default timeout: 15s | ||
| jest.setTimeout(15000); |
There was a problem hiding this comment.
Reducing this by default, in cases where we need more we can opt-in to a higher timeout in a case-by-case basis (which also leads to us being conscious about which tests take long)
| () => { | ||
| // We send this so we can wait for this, to know the test is ended & server can be closed | ||
| if (id === '3') { | ||
| Sentry.captureException(new Error('Final exception was captured')); |
There was a problem hiding this comment.
this lead to a "race condition" in tests, which did not fail the test, but showed errors in the test log:
we wait for the error event to be sent, then end the test. at this point the server is shut down, which leads to the http request being aborted.
now, we specifically wait for this event, so we know everything is done.
There was a problem hiding this comment.
oh, I think my mistake here was that I though the test was terminated when both the request ends and the error was received. Maybe that is actually what we wanna do in the test? idk
| @@ -13,7 +13,7 @@ Sentry.init({ | |||
| const connect = require('connect'); | |||
| const http = require('http'); | |||
|
|
|||
| const init = async () => { | |||
| const run = async () => { | |||
There was a problem hiding this comment.
renaming these in a few places for consistency sake, init was a bit confusing IMHO.
| * @param {string} url | ||
| * @return {*} {Promise<void>} | ||
| */ | ||
| export async function runScenario(url: string): Promise<void> { |
There was a problem hiding this comment.
all of this is not used anymore anywhere, so can remove!
| spawnSync('docker', ['compose', 'down', '--volumes'], { cwd }); | ||
| spawnSync('docker', ['compose', 'down', '--volumes'], { | ||
| cwd, | ||
| stdio: process.env.DEBUG ? 'inherit' : undefined, |
| @@ -72,7 +72,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => { | |||
| }, | |||
| }, | |||
| }) | |||
| .start(done); | |||
| .start(closeTestServer); | |||
There was a problem hiding this comment.
the main change, when we use createTestServer, make sure to use this instead of done (which under the hood also calls done), to make sure everything is closed properly.
mydea
force-pushed
the
fn/streamline-test-timeout
branch
from
ec780b9
to
657939a
Compare
mydea
changed the title
dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
Show resolved
Hide resolved
mydea
force-pushed
the
fn/streamline-test-timeout
branch
from
657939a
to
4132ce4
Compare
This PR streamlines and fixes some timing/cleanup issues we had in integration tests, which sometimes lead to issues.
One problem was introduced here: #13253
This change lead to the server being shut down (because
doneis called) before the HTTP requests are finished, leading to error logs.Another problem was that in some cases we had leaking processes, where we did not properly close servers we started - this was everywhere we used
createTestServer.I also moved some code from the node-integration-tests package to the remix package, that was only used there (and not properly depended/imported on).
For future debugging, this was shown by running tests with
--detectOpenHandles.