-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(node): Add utility to test esm & cjs instrumentation #16159
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
Conversation
1f8b4bd
to
4713fa6
Compare
9c5f571
to
4e41faf
Compare
@@ -107,7 +107,7 @@ const ANR_EVENT_WITH_DEBUG_META: Event = { | |||
}, | |||
}; | |||
|
|||
describe('should report ANR when event loop blocked', { timeout: 60_000 }, () => { | |||
describe('should report ANR when event loop blocked', { timeout: 90_000 }, () => { |
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.
unrelated but this flaked some times, let's see if this helps...
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.
Nice, thanks for adding this! Just some l-level questions but looks good!
dev-packages/node-integration-tests/suites/child-process/fork.mjs
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/child-process/worker.mjs
Outdated
Show resolved
Hide resolved
const cjsInstrumentPath = join(cwd, `tmp_${instrumentPath.replace('.mjs', '.cjs')}`); | ||
|
||
// For the CJS runner, we create some temporary files... | ||
if (!options?.failsOnCjs) { |
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.
l: if failsOnCjs
is true
and we don't convert the files, would we even run the test where we add test.fails
further down below?
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.
good catch, forgot about this - I restructured this a few times but forgot about this, this should always be run :)
* @param content The content of an ESM file | ||
* @returns The content with require statements instead of imports | ||
*/ | ||
function convertEsmToCjs(content: string): string { |
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.
l: not sure if an issue today but should we handle dynamic imports? I don't think they're covered by the conversion cases but might have missed it
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.
we do not! but this also does not directly convert to cjs well, I suppose - I'd say for cases where we want to test this, it's probably best to still do this manually (which can still be done the same as before!)
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.
Good that this string-replace works :D
Thanks for adding this!
Today, we do not have a lot of esm-specific node integration tests. Our tests make it possible to test ESM, but we only use it very rarely, sticking to cjs tests across the suite mostly. This means that we have a bunch of gaps around our ESM support.
This PR introduces a new test utility to make it easier to test stuff in ESM and CJS:
This has a few limitations based on how this works - we can make this more robust in the future, but for now it should be "OK":
For tests that are known to fail on e.g. esm or cjs, you can configure
failsOnEsm: true
. In this case, it will fail if the test does not fail (by usingtest.fails()
to ensure test failure). This way we can ensure we find out if stuff starts to fail etc.To make this work, I had to re-write the test runner code a bit, because it had issues with vitest unhandled rejection handling. Due to the way we handled test completion with a promise,
test.fails
was not working as expected, because the test indeed succeeded when it failed, but the overall test run still failed because an unhandled rejection bubbled up. Now, this should work as expected...I re-wrote a few tests already to show how it works, plus added a new test that shows an ESM test failure when importing http module (😭 )