-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
chore(test): Use finished
to check streams
#462
Conversation
Node's promise `finished` function is only available in Node 16.
This reverts commit b236db5.
@@ -18,18 +18,16 @@ async function main() { | |||
maxMemUsage = Math.max(maxMemUsage, stats.used_heap_size); | |||
}); | |||
|
|||
const statsPromise = new Promise((resolve) => memwatch.once('stats', resolve)); |
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 event was updated to use the promise constructor, which allowed us to get rid of promisify-event
.
@@ -21,7 +22,7 @@ global.upstreamParser = parse5Upstream; | |||
global.hugePage = readFileSync(hugePagePath).toString(); | |||
|
|||
// Micro data | |||
global.microTests = loadTreeConstructionTestData([treeConstructionPath], treeAdapters.default) | |||
global.microTests = loadTreeConstructionTestData(treeConstructionPath, treeAdapters.default) |
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.
The perf
benchmark was broken, as I forgot to update this call site to loadTreeConstructionTestData
when changing the argument to a URL.
finished
to check streamsfinished
to check streams
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.
I’m abstaining as I don’t feel qualified to review this :)
No worries — as this only relates to tests, I'm feeling quite confident in the changes and will go ahead with things. |
The
finish
event doesn't actually fire every time a stream ends. This was already discovered in #432. This PR cleans up all other call locations.Also switches us to using promises instead of callbacks for these tests. This has the advantage that errors will actually be reported by the test runner, instead of being uncaught inside a hanging test.