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.

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

[test] tracing unit tests failing when run on *.js tests #28619

Closed
jeremymeng opened this issue Feb 20, 2024 · 3 comments · Fixed by #28631
Closed

[test] tracing unit tests failing when run on *.js tests #28619

jeremymeng opened this issue Feb 20, 2024 · 3 comments · Fixed by #28631
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. test-reliability Issue that causes tests to be unreliable

Comments

@jeremymeng
Copy link
Member

Start failing after Core ESM migration. Is it something we missed in the esm4mocha workaround?

For example, under sdk/tables/data-tables
npm run unit-test:node Passed but npm run integration-test:node Failed

https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3511152&view=logs&j=14c0e587-6dd4-5861-ebc6-f356f402a852&t=eff8af1f-6913-5d5c-f7a4-bc3189c07383&l=528

image

@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. test-reliability Issue that causes tests to be unreliable labels Feb 20, 2024
@jeremymeng
Copy link
Member Author

npm run unit-test:node use a CommonJS package, ts-node/register and esm to transform *.spec.ts files for Mocha.
npm run integration-test:node load dist-esm files as ESM and transform relative imports before feeding *.js" to Mocha.

@maorleger
Copy link
Member

core-tracing uses a module-global instrumenter

The supportsTracing helper sets the instrumenter before running a test.

I wonder, but not sure, if this is a CJS+ESM modules living side-by-side causing dual package hazards documented in tshy ?

@jeremymeng
Copy link
Member Author

Seems related. The test is loading CJS version of test-utils. Maybe it would be resolved after test-util is migrated.

@maorleger maorleger self-assigned this Feb 22, 2024
maorleger added a commit that referenced this issue Feb 22, 2024
### Packages impacted by this PR

@azure/core-tracing

### Issues associated with this PR

Resolves #28619

### Describe the problem that is addressed by this PR

If you are exporting both CommonJS and ESM forms of a package, then it
is possible for both versions to be loaded at run-time.
However, the CommonJS build is a different module from the ESM build,
and thus a different thing from the point of view of the
JavaScript interpreter in Node.js.

> https://github.com/isaacs/tshy/blob/main/README.md#dual-package-hazards

tshy handles this by building programs into separate folders and treats
"dual module hazards" as a fact of life. 

One of the hazards of dual-modules is shared module-global state.

In core-tracing we have a module-global instrumenter that is used for
hooking into and lighting up OpenTelemetry based instrumentation. In order to
ensure it works in this dual-package world we must use one of multiple-recommended
workarounds.

In this case, the tshy documentation provides a solution to this with a
well-documented path forward. This is what is implemented here.

Please refer to
https://github.com/isaacs/tshy/blob/main/README.md#module-local-state
for added context
sofiar-msft pushed a commit to sofiar-msft/azure-sdk-for-js that referenced this issue Apr 11, 2024
### Packages impacted by this PR

@azure/core-tracing

### Issues associated with this PR

Resolves Azure#28619

### Describe the problem that is addressed by this PR

If you are exporting both CommonJS and ESM forms of a package, then it
is possible for both versions to be loaded at run-time.
However, the CommonJS build is a different module from the ESM build,
and thus a different thing from the point of view of the
JavaScript interpreter in Node.js.

> https://github.com/isaacs/tshy/blob/main/README.md#dual-package-hazards

tshy handles this by building programs into separate folders and treats
"dual module hazards" as a fact of life. 

One of the hazards of dual-modules is shared module-global state.

In core-tracing we have a module-global instrumenter that is used for
hooking into and lighting up OpenTelemetry based instrumentation. In order to
ensure it works in this dual-package world we must use one of multiple-recommended
workarounds.

In this case, the tshy documentation provides a solution to this with a
well-documented path forward. This is what is implemented here.

Please refer to
https://github.com/isaacs/tshy/blob/main/README.md#module-local-state
for added context
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. test-reliability Issue that causes tests to be unreliable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants