Skip to content

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

Merged
merged 15 commits into from
Apr 30, 2025

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 29, 2025

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:

createEsmAndCjsTests(
    __dirname,
    'scenario.mjs',
    'instrument.mjs',
    (createRunner, test) => {
      test('it works when importing the http module', async () => {
        const runner = createRunner(); // normal test as before
       });
});

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":

  1. It requires a .mjs based instrument file as well as an .mjs based scenario
  2. No relative imports are supported (all content must be in these two files)
  3. It simply regex replaces the esm imports with require for the CJS tests. not perfect, but it kind of works

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 using test.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 (😭 )

@mydea mydea self-assigned this Apr 29, 2025
@mydea mydea force-pushed the fn/node-integration-test-esm branch from 1f8b4bd to 4713fa6 Compare April 30, 2025 07:43
@mydea mydea force-pushed the fn/node-integration-test-esm branch from 9c5f571 to 4e41faf Compare April 30, 2025 08:03
@mydea mydea requested review from Lms24 and s1gr1d April 30, 2025 08:08
@mydea mydea marked this pull request as ready for review April 30, 2025 08:08
@@ -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 }, () => {
Copy link
Member Author

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...

Copy link
Member

@Lms24 Lms24 left a 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!

const cjsInstrumentPath = join(cwd, `tmp_${instrumentPath.replace('.mjs', '.cjs')}`);

// For the CJS runner, we create some temporary files...
if (!options?.failsOnCjs) {
Copy link
Member

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?

Copy link
Member Author

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 {
Copy link
Member

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

Copy link
Member Author

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!)

Copy link
Member

@s1gr1d s1gr1d left a 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!

@mydea mydea merged commit 2e164e1 into develop Apr 30, 2025
31 checks passed
@mydea mydea deleted the fn/node-integration-test-esm branch April 30, 2025 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants