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

fix(test): fix failing tests by preventing source-map generation #3642

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

Windows tests are currently running out of heap space, which seems to be related to source map merging. As far as I can tell, we're already generating source maps for the browser/webworker tests, and using ts-mocha already provides us with source-maps. While investigating #3628, I found that the merging of source maps (persumably merging the existing ones and the ones generated by nyc/istanbul) is what allocates a lot of memory. This PR disables the generation of source maps and reduces the amount of memory used significantly from >3.9GB to 174MB

Fixes #3628

Short description of the changes

Prevent source map generation by nyc

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Local testing (memory profiling, sanity check by looking at Maximum resident set size produced by time -v)

@pichlermarc pichlermarc changed the title fix(test): fix failing tests by using preventing source-map generation fix(test): fix failing tests by preventing source-map generation Feb 28, 2023
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #3642 (0604f02) into main (0d373bd) will increase coverage by 0.19%.
The diff coverage is n/a.

❗ Current head 0604f02 differs from pull request most recent head 5f9c33b. Consider uploading reports for the commit 5f9c33b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3642      +/-   ##
==========================================
+ Coverage   93.52%   93.72%   +0.19%     
==========================================
  Files         271      274       +3     
  Lines        7496     8058     +562     
  Branches     1521     1670     +149     
==========================================
+ Hits         7011     7552     +541     
- Misses        485      506      +21     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...y-instrumentation-http/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 94.26% <0.00%> (ø)
...es/opentelemetry-instrumentation-http/src/utils.ts 99.18% <0.00%> (ø)

@pichlermarc pichlermarc force-pushed the fix/windows-test-source-map branch from 59c3b76 to b913b24 Compare February 28, 2023 13:01
@pichlermarc pichlermarc marked this pull request as ready for review February 28, 2023 13:16
@pichlermarc pichlermarc requested a review from a team February 28, 2023 13:16
@dyladan
Copy link
Member

dyladan commented Feb 28, 2023

Are we sure this doesn't affect coverage?

@Flarna
Copy link
Member

Flarna commented Feb 28, 2023

nyc is instrumenting the code (e.g. all ifs/functions to check if they are entered) therefore line numbers,.. may change.

I would assume impact to code coverage results but it seems there is none if I look into the above posted results.

@pichlermarc
Copy link
Member Author

Are we sure this doesn't affect coverage?

I ran it locally and it did not affect coverage generated after a clean build:

  • git clean --ffdx && npm run compile
  • NODE_OPTIONS=--openssl-legacy-provider npm run test:browser
  • NODE_OPTIONS=--openssl-legacy-provider npm run test

I think nyc still uses available source maps if they are already generated. From nyc --help:

--produce-source-map        should source maps be produced?
                                                       [boolean] [default: true]
--source-map                should nyc detect and handle source maps?
                                                       [boolean] [default: true]

nyc is instrumenting the code (e.g. all ifs/functions to check if they are entered) therefore line numbers,.. may change.
I would assume impact to code coverage results but it seems there is none if I look into the above posted results.

I have to admit I'm not sure how it maps its instrumented code to the actual line numbers though (I would also assume --produce-source-map=false would break that). When I tried it out locally, however, the line numbers were still correct even when a test would fail. 🤔

I was not able to find reliable documentation about this option, so I had to rely on trial and error to come up with this solution.

@Flarna
Copy link
Member

Flarna commented Mar 1, 2023

Looking into the PR which added this option I would assume we are safe. It seems to be related to mapping of stack traces not coverage results.

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.

Windows tests are failing
4 participants