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

Tree shaking tests fail if ESM build not yet completed #3398

Closed
pichlermarc opened this issue Nov 9, 2022 · 4 comments · Fixed by #3432
Closed

Tree shaking tests fail if ESM build not yet completed #3398

pichlermarc opened this issue Nov 9, 2022 · 4 comments · Fixed by #3432
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization

Comments

@pichlermarc
Copy link
Member

What happened?

Steps to Reproduce

Using Node 18.12.1 (latest) and npm 8.19.2

  • git clone https://github.com/open-telemetry/opentelemetry-js.git
  • cd opentelemetry-js
  • npm install
  • npm run compile
  • NODE_OPTIONS=--openssl-legacy-provider npm run test

Expected Result

Tests passing, as they do in CI.

Actual Result

@opentelemetry/api tree-shaking tests fail (see "Relevant log output below")

Additional Details

Also failing on Node 16.

OpenTelemetry Setup Code

n/a

package.json

n/a

Relevant log output

tree-shaking
    1) verify MetricsAPI
    2) verify PropagationAPI
    3) verify TraceAPI


  1110 passing (917ms)
  3 failing

  1) tree-shaking
       verify MetricsAPI:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
    'MetricsAPI',
+   'PropagationAPI',
+   'TraceAPI'
  ]
      + expected - actual

       [
         "MetricsAPI"
      -  "PropagationAPI"
      -  "TraceAPI"
       ]
      
      at Context.<anonymous> (test/tree-shaking/tree-shaking.test.ts:116:14)

  2) tree-shaking
       verify PropagationAPI:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
+   'MetricsAPI',
+   'PropagationAPI',
+   'TraceAPI'
-   'PropagationAPI'
  ]
      + expected - actual

       [
      -  "MetricsAPI"
         "PropagationAPI"
      -  "TraceAPI"
       ]
      
      at Context.<anonymous> (test/tree-shaking/tree-shaking.test.ts:116:14)

  3) tree-shaking
       verify TraceAPI:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
+   'MetricsAPI',
+   'PropagationAPI',
    'TraceAPI'
  ]
      + expected - actual

       [
      -  "MetricsAPI"
      -  "PropagationAPI"
         "TraceAPI"
       ]
      
      at Context.<anonymous> (test/tree-shaking/tree-shaking.test.ts:116:14)
@pichlermarc pichlermarc added bug Something isn't working triage labels Nov 9, 2022
@pichlermarc pichlermarc changed the title Build failing locally but not in CI Tree shaking tests failing locally but not in CI Nov 9, 2022
@dyladan dyladan removed bug Something isn't working triage labels Nov 9, 2022
@dyladan
Copy link
Member

dyladan commented Nov 9, 2022

This can happen if the ESM version of the API package is not generated. You have to run npm run compile in the api directory before running tests. The non-esm version cannot be tree shaken at least by these tests

@dyladan dyladan added bug Something isn't working triage labels Nov 9, 2022
@dyladan
Copy link
Member

dyladan commented Nov 9, 2022

I see that you actually did run npm run compile... Can you please try again and confirm that you are compiling the ESM versions? Passes for me locally if I run the ESM compile.

@legendecas
Copy link
Member

Yeah, this requires the ESM targets are generated with npm run compile in the API directory. I'm experimenting with a dedicated script for managing those tsconfig targets at #3169. But it still needs more time to polish.

@dyladan dyladan added priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization and removed triage labels Nov 9, 2022
@dyladan dyladan changed the title Tree shaking tests failing locally but not in CI Tree shaking tests fail if ESM build not yet completed Nov 9, 2022
@dyladan dyladan added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Nov 9, 2022
@legendecas
Copy link
Member

Submitted #3432 to solve this problem. Please have a try on your local environments, thanks! :)

@legendecas legendecas removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants