warmup the nyc cache before running the tests - #8130
Conversation
Overall package sizeSelf size: 5.56 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: f77a447 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8130 +/- ##
==========================================
- Coverage 73.82% 73.81% -0.01%
==========================================
Files 784 784
Lines 36369 36369
==========================================
- Hits 26848 26847 -1
- Misses 9521 9522 +1 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9847d4440
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "test:integration:bun": "mocha --timeout 60000 \"integration-tests/bun/*.spec.js\"", | ||
| "test:integration:cucumber": "mocha --timeout 60000 \"integration-tests/cucumber/*.spec.js\"", | ||
| "test:integration:cypress": "mocha --timeout 60000 \"integration-tests/cypress/${SPEC:-cypress-*}.spec.js\"", | ||
| "test:integration:cypress": "mocha --timeout 60000 \"integration-tests/cypress/*.spec.js\"", |
There was a problem hiding this comment.
Removing ${SPEC:-cypress-*} from this script makes the SPEC environment variable a no-op, but the integration-cypress job in .github/workflows/test-optimization.yml still sets SPEC: ${{ matrix.spec }} for sharding. With the new glob, every matrix leg runs all Cypress integration specs instead of one, which significantly inflates CI runtime and flake exposure and invalidates the intended per-spec matrix split.
Useful? React with 👍 / 👎.
| "test:integration:jest": "mocha --timeout 60000 \"integration-tests/jest/*.spec.js\"", | ||
| "test:integration:mocha": "mocha --timeout 60000 \"integration-tests/mocha/*.spec.js\"", | ||
| "test:integration:playwright": "mocha --timeout 60000 \"integration-tests/playwright/${SPEC:-playwright-*}.spec.js\"", | ||
| "test:integration:playwright": "mocha --timeout 60000 \"integration-tests/playwright/*.spec.js\"", |
There was a problem hiding this comment.
This change drops ${SPEC:-playwright-*} even though .github/workflows/test-optimization.yml passes SPEC: ${{ matrix.spec }} in the integration-playwright matrix. As a result, each matrix leg now executes every Playwright integration spec instead of the targeted one, multiplying job duration and retry surface while defeating the matrix's spec-level partitioning.
Useful? React with 👍 / 👎.
pabloerhard
left a comment
pabloerhard
left a comment
There was a problem hiding this comment.
LGTM, but wouldn’t this hide the issue that our init is now occasionally taking longer than 5s and therefore failing? Is this expected as the codebase grows?
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM, this is a good idea and will reduce the main flakiness!
I wonder if it would make sense to move this inside of the workflows at a later point.
|
@pabloerhard the init just took longer due to the NYC instrumentation. Without that, it will be faster. We can theoretically reduce the time budget from 5 seconds to a smaller number, if we make sure all our setups are fast enough. |
* warmup the nyc cache before running the tests * hide warmup output
* warmup the nyc cache before running the tests * hide warmup output
c8 uses V8's built-in coverage instead of source-level instrumentation, so there is no per-file cold-instrumentation cost during the first test of a suite — that's the recurring `Timeout of 5000ms exceeded` flake we've been chasing on plugin tests. The nyc cache warmup added in #8130 becomes unnecessary and is removed. Side benefits: - Native ESM coverage works out of the box (the four .mjs files in nyc.config.js's include were never instrumented under nyc's CJS-only hook). - yarn.lock loses ~520 lines of nyc/istanbul transitive deps. Tradeoffs: - Higher peak memory under load — V8 keeps coverage maps for every loaded script. Should be fine on standard CI runners. - Branch counts come from V8 block coverage rather than istanbul AST instrumentation; reports may shift slightly. `scripts/run-c8.js` mirrors the per-Node-version-and-script output dir naming that `nyc.config.js` was doing dynamically (so multiple Node versions running sequentially in one job don't collide); static config lives in `.c8rc.json`. `verify-coverage.js` keeps working unchanged because the output layout (`coverage/node-${version}${label}/lcov.info`) is identical. Smoke-tested locally: `npm run test:core:ci` produces the expected lcov artifacts and `verify-coverage.js` passes.
c8 uses V8's built-in coverage instead of source-level instrumentation, so there is no per-file cold-instrumentation cost during the first test of a suite — that's the recurring `Timeout of 5000ms exceeded` flake we've been chasing on plugin tests. The nyc cache warmup added in #8130 becomes unnecessary and is removed. Side benefits: - Native ESM coverage works out of the box (the four .mjs files in nyc.config.js's include were never instrumented under nyc's CJS-only hook). - yarn.lock loses ~520 lines of nyc/istanbul transitive deps. Tradeoffs: - Higher peak memory under load — V8 keeps coverage maps for every loaded script. Should be fine on standard CI runners. - Branch counts come from V8 block coverage rather than istanbul AST instrumentation; reports may shift slightly. `scripts/run-c8.js` mirrors the per-Node-version-and-script output dir naming that `nyc.config.js` was doing dynamically (so multiple Node versions running sequentially in one job don't collide); static config lives in `.c8rc.json`. `verify-coverage.js` keeps working unchanged because the output layout (`coverage/node-${version}${label}/lcov.info`) is identical. Smoke-tested locally: `npm run test:core:ci` produces the expected lcov artifacts and `verify-coverage.js` passes.
3 participants
What does this PR do?
Warmup the nyc cache before running the tests.
Motivation
When running our tests, we defer calling
tracer.init()until the first call toagent.load(). This means that the cost of instrumenting all of the project files for code coverage is deferred until that moment, which can be quite heavy without an existing nyc cache. This causes the firstbeforeEachto callagent.load()to sometimes take more than 5 seconds and ultimately timeout. By warming up the cache before running the tests, we avoid this issue entirely. Not all files are included in the warmup to keep it fast.Additional Notes
Before the change, running the Express test locally without any cache made
agent.load()take 2.3s compared to 230ms with the cache. With the partial warmup, it now takes ~400ms instead. Not as good as a full warmup, but good enough and takes way less time to generate.