-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test: Fix node-integration-test timeouts & cleanup #13280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d095016
30ba5de
ff129be
2aac9ce
aabf9ff
a8ef24d
4a21ce2
a49fdad
6fa1b8d
69aac74
54bbfa9
8e18a7c
4132ce4
8ac5f0b
3287f76
2a037dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
const { cleanupChildProcesses } = require('./utils/runner'); | ||
|
||
// Increases test timeout from 5s to 45s | ||
jest.setTimeout(45000); | ||
// Default timeout: 15s | ||
jest.setTimeout(15000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
afterEach(() => { | ||
cleanupChildProcesses(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,15 +54,19 @@ app.get('/test/error/:id', (req, res) => { | |
|
||
setTimeout(() => { | ||
// We flush to ensure we are sending exceptions in a certain order | ||
Sentry.flush(3000).then( | ||
Sentry.flush(1000).then( | ||
() => { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
res.send({}); | ||
}, | ||
() => { | ||
res.send({}); | ||
}, | ||
); | ||
}, 1000); | ||
}, 1); | ||
}); | ||
|
||
Sentry.setupExpressErrorHandler(app); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renaming these in a few places for consistency sake, init was a bit confusing IMHO. |
||
const app = connect(); | ||
|
||
app.use('/', function (req, res, next) { | ||
|
@@ -34,4 +34,4 @@ const init = async () => { | |
sendPortToRunner(port); | ||
}; | ||
|
||
init(); | ||
run(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => { | |
test('outgoing fetch requests create breadcrumbs', done => { | ||
createTestServer(done) | ||
.start() | ||
.then(SERVER_URL => { | ||
.then(([SERVER_URL, closeTestServer]) => { | ||
createRunner(__dirname, 'scenario.ts') | ||
.withEnv({ SERVER_URL }) | ||
.ensureNoErrorOutput() | ||
|
@@ -72,7 +72,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => { | |
}, | ||
}, | ||
}) | ||
.start(done); | ||
.start(closeTestServer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the main change, when we use |
||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not actually used anywhere anymore, so removed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or, actually moved it to the remix package, where it is used/needed.