-
Notifications
You must be signed in to change notification settings - Fork 306
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
[ci-visibility] Support mocha parallel mode #4314
Conversation
Overall package sizeSelf size: 6.5 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
8d720da
to
5ae4dec
Compare
BenchmarksBenchmark execution time: 2024-05-22 10:27:39 Comparing candidate commit ef92015 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics. |
const testToStartLine = new WeakMap() | ||
|
||
// mocha-each support | ||
addHook({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ these two hooks are not modified at all
// skippable and known tests. | ||
// It is called but skipped in parallel mode. In parallel mode, we'll use | ||
// the run-helpers hook to grab the data. | ||
addHook({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ this hook was not modified, I just removed a warning log for parallel mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, thanks for these comments! more than 500 lines that I can ignore :-D
file: 'lib/cli/run-helpers.js' | ||
}, (run) => { | ||
shimmer.wrap(run, 'runMocha', runMocha => async function () { | ||
if (!testStartCh.hasSubscribers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ this hook was not modified
// In serial mode, this hook is used to set the correct async resource to the test. | ||
// In parallel mode, the same hook is executed in the worker, so this needs to be repeated in | ||
// mocha/worker.js | ||
addHook({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ the hook was not modified, but we extracted the function to runnableWrapper
|
||
// Only used in parallel mode (--parallel flag is passed) | ||
// Used to generate suite events and receive test payloads from workers | ||
addHook({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
// Only used in parallel mode (--parallel flag is passed) | ||
// Used to start and finish test session and test module | ||
addHook({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4314 +/- ##
==========================================
+ Coverage 85.23% 86.12% +0.89%
==========================================
Files 252 250 -2
Lines 11042 10742 -300
Branches 33 33
==========================================
- Hits 9412 9252 -160
+ Misses 1630 1490 -140 ☔ View full report in Codecov by Sentry. |
4d3edbf
to
2939b88
Compare
@@ -0,0 +1,23 @@ | |||
// eslint-disable-next-line | |||
const workerpool = require('workerpool') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ to test that running workerpool
outside of a test does not crash
@@ -1,674 +1,5 @@ | |||
const { createCoverageMap } = require('istanbul-lib-coverage') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ this file is mostly copied to mocha/main.js
packages/dd-trace/test/ci-visibility/exporters/test-worker/exporter.spec.js
Outdated
Show resolved
Hide resolved
packages/dd-trace/test/ci-visibility/exporters/test-worker/exporter.spec.js
Show resolved
Hide resolved
ℹ️ I missed a |
could you have a look at the latest commit @uurien ? |
What does this PR do?
Add support for
mocha --parallel
.Changes:
mocha.js
instrumentation file into three:main.js
andworker.js
, which correspond to the main process and the worker processes, andcommon.js
for common pieces. Additionally extract some common functionality intoutils.js
worker.js
file will only instrument tests. Suites, modules and sessions will be handled bymain.js
.main.js
will mostly include everything thatmocha.js
had, but we've added two extra hooks forlib/nodejs/parallel-buffered-runner.js
andworkerpool
to support parallel mode.workerpool
library because we need to intercept interprocess communication between mocha's main process and the workers, so we can report the test data.Motivation
Closes #4046
We want to be able to support mocha when it's run in parallel mode: https://mochajs.org/#parallel-tests
Plugin Checklist