Uh oh!
There was an error while loading. Please reload this page.
test(node): Add utility to test esm & cjs instrumentation - #16159
Conversation
1f8b4bd to
4713fa6Compare9c5f571 to
4e41fafCompare| }; | ||
| 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.
unrelated but this flaked some times, let's see if this helps...
Lms24
left a comment
There was a problem hiding this comment.
Nice, thanks for adding this! Just some l-level questions but looks good!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
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.
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.
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.
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!)
s1gr1d
left a comment
There was a problem hiding this comment.
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.failswas 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 (😭 )