Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce ESM testing strategy #1731

Closed
3 tasks
pichlermarc opened this issue Oct 12, 2023 · 1 comment · Fixed by #1735
Closed
3 tasks

Introduce ESM testing strategy #1731

pichlermarc opened this issue Oct 12, 2023 · 1 comment · Fixed by #1735
Labels
enhancement New feature or request

Comments

@pichlermarc
Copy link
Member

Is your feature request related to a problem? Please describe

In the @opentelemetry/instrumentation package, we added experimental ESM support a while ago (open-telemetry/opentelemetry-js#3698). As more and more users started using this, the lack of ESM-releated testing became apparent. A high-priority bug that's present in a large number of instrumentations causes applications to crash as wrapping ES modules fails.

There have been PRs opened to address this; however, as we don't test for ESM, it is hard to verify fixes and catch regressions.

Describe the solution you'd like to see

We should start testing if instrumentations also work with ESM. This issue tracks

  • documenting an ESM testing strategy for existing packages
  • implementing the suggested tests in at least two instrumentation packages
  • adding a step in the unit tests workflow to run the ESM tests
@pichlermarc pichlermarc added the enhancement New feature or request label Oct 12, 2023
This was referenced Oct 12, 2023
Merged
Merged
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Oct 13, 2023
@trentm
feat(test-utils): runTestFixture utility for running out-of-process t…
86dbd54 
…ests

Running scripts out-of-process for testing allows running tests with
different node options and with isolation. The former is useful for
ESM testing. This change includes adding an ESM test for ioredis.
This depends on open-telemetry#1694 to pass.

Closes: open-telemetry#1731
@trentm trentm mentioned this issue Oct 13, 2023
Merged
1 task
@pichlermarc
Copy link
Member Author

I've been experimenting a bit, and we may be able to re-use the existing mocha tests in ESM mode by using the ts-node/esm loader and chaining it with import-in-the-middle to get full coverage.

At the moment one thing preventing us from doing so is that once I add @opentelemetry/instrumentation/hook.mjs as a loader, the require.main === module check in ./node_modules/mocha/lib/cli/cli.js fails and mocha exits without running tests. I think this may be related to how import-in-the-middle works, so I opened nodejs/import-in-the-middle#34.

trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Oct 25, 2023
@trentm
test(ioredis): test ioredis ESM usage
fb9140a 
This is an attempt to test ESM usage of ioredis based on Marc's comment at
open-telemetry#1731 (comment)

This *passes*, but the thing that scares me is enabling the ESM hook for
.test.ts tests as well. The IORedisInstrumentation patch method is being
called *both* for ESM and CommonJS for ioredis.test.ts -- I don't
understand that.

Refs: open-telemetry#1731
Refs: open-telemetry#1735
@trentm trentm mentioned this issue Oct 25, 2023
Closed
@pichlermarc pichlermarc mentioned this issue Nov 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
1 participant
@pichlermarc