test(node): Add esm/cjs specific test runner utils#21729
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9cbddb5. Configure here.
| export function createEsmTests( | ||
| cwd: string, | ||
| scenarioPath: string, | ||
| instrumentPath: string, | ||
| callback: (createTestRunner: () => ReturnType<typeof createRunner>, testFn: typeof test, cwd: string) => void, | ||
| options?: { | ||
| /** | ||
| * `additionalDependencies` to install in the tmp dir. | ||
| */ | ||
| additionalDependencies?: Record<string, string>; | ||
| /** Copy these files/dirs into the tmp dir. */ | ||
| copyPaths?: string[]; | ||
| }, | ||
| ) { | ||
| const [tmpDirPath, createTmpDir, paths] = prepareTmpDir(cwd, scenarioPath, instrumentPath, options); | ||
| const createdRunners = new Set<ReturnType<typeof createRunner>>(); | ||
|
|
||
| callback( | ||
| () => trackRunner(createdRunners, createRunner(paths.esm.scenario).withFlags('--import', paths.esm.instrument)), | ||
| test, | ||
| tmpDirPath, | ||
| ); | ||
|
|
||
| setupAndCleanup(createTmpDir, createdRunners, tmpDirPath); | ||
| } | ||
|
|
||
| /** | ||
| * Run one or multiple tests in CJS only for a given scenario and instrument file. | ||
| */ | ||
| export function createCjsTests( | ||
| cwd: string, | ||
| scenarioPath: string, | ||
| instrumentPath: string, | ||
| callback: (createTestRunner: () => ReturnType<typeof createRunner>, testFn: typeof test, cwd: string) => void, | ||
| options?: { | ||
| /** | ||
| * `additionalDependencies` to install in the tmp dir. | ||
| */ | ||
| additionalDependencies?: Record<string, string>; | ||
| /** Copy these files/dirs into the tmp dir. */ | ||
| copyPaths?: string[]; | ||
| }, | ||
| ) { | ||
| const [tmpDirPath, createTmpDir, paths] = prepareTmpDir(cwd, scenarioPath, instrumentPath, options); | ||
| const createdRunners = new Set<ReturnType<typeof createRunner>>(); | ||
|
|
||
| callback( | ||
| () => trackRunner(createdRunners, createRunner(paths.cjs.scenario).withFlags('--require', paths.cjs.instrument)), | ||
| test, | ||
| tmpDirPath, | ||
| ); | ||
|
|
||
| setupAndCleanup(createTmpDir, createdRunners, tmpDirPath); | ||
| } |
There was a problem hiding this comment.
l: I'd have extracted the core out into a createTest function and then call that within createCjsTests and createEsmTests as they are identical except of flag and scenario passed. Seems like a bunch of duplicated code.
But this is just a test helper so feel free to disregard.
There was a problem hiding this comment.
the problem with this is that most of the actual code/work would then be duplicated as the setup stuff is the same for both 🤔 but we can def. refactor this around some more.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| /** | ||
| * Run one or multiple tests in ESM only for a given scenario and instrument file. | ||
| */ | ||
| export function createEsmTests( |
There was a problem hiding this comment.
Bug: The new functions createEsmTests and createCjsTests are not exported from the main index.ts file, making them inconsistent with the module's public API.
Severity: LOW
Suggested Fix
Re-export the createEsmTests and createCjsTests functions from the dev-packages/node-integration-tests/utils/runner/index.ts file to ensure they are part of the module's public API, consistent with other exported utilities.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/node-integration-tests/utils/runner/createEsmAndCjsTests.ts#L72
Potential issue: The new utility functions `createEsmTests` and `createCjsTests` are
defined with `export` in their source file, `createEsmAndCjsTests.ts`, but are not
re-exported from the module's public entry point, `index.ts`. While the tests updated in
this pull request work by importing directly from the source file, this creates an
inconsistent API. Other public utilities from this module are exported via `index.ts`.
Future developers will likely expect to import these new functions from the main entry
point and will find they are unavailable, leading to confusion. This represents an
incomplete refactoring that violates the principle of a single public interface for the
module.
Also affects:
dev-packages/node-integration-tests/utils/runner/createEsmAndCjsTests.ts:101~101
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
This make some small adjustments to the node test runner, as well as exposing two new methods for faster tests:
failsOnEsm: falsedefinitionsTwo new methods now exist for faster tests:
createEsmTestscreateCjsTestsThis works the same as
createEsmAndCjsTests, but does... only run a single set. For now, I did not change the code to actually generate both variants in the tmp dir (as the file i/o is actually super fast so not really the bottleneck, but may be cleaned up later). However, we can use this in places where we do not need to run things in both environments.In this PR, I have rewritten to the new methods in the following places only:
failsOnEsm: trueto ensure we find out if that stops failing, but we do not need to run every single scenario in failure mode, only one.In follow ups, we can think about only running one or two tests per integration in esm&cjs mode to verify it generally works in both, and run other scenarios only in esm or cjs, for faster test execution times.