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

[ci-visibility] Support mocha parallel mode #4314

Merged
merged 8 commits into from
May 23, 2024

Conversation

juan-fernandez
Copy link
Collaborator

@juan-fernandez juan-fernandez commented May 16, 2024

What does this PR do?

Add support for mocha --parallel.

Changes:

  • Split the mocha.js instrumentation file into three: main.js and worker.js, which correspond to the main process and the worker processes, and common.js for common pieces. Additionally extract some common functionality into utils.js
  • worker.js file will only instrument tests. Suites, modules and sessions will be handled by main.js.
  • main.js will mostly include everything that mocha.js had, but we've added two extra hooks for lib/nodejs/parallel-buffered-runner.js and workerpool to support parallel mode.
  • Added a hook for 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

  • Unit tests.

Copy link

github-actions bot commented May 16, 2024

Overall package size

Self size: 6.5 MB
Deduped: 60.64 MB
No deduping: 60.92 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 2.1.0 14.91 MB 14.92 MB
@datadog/native-appsec 7.1.1 14.39 MB 14.4 MB
@datadog/pprof 5.3.0 9.85 MB 10.22 MB
protobufjs 7.2.5 2.77 MB 6.56 MB
@datadog/native-iast-rewriter 2.3.1 2.15 MB 2.24 MB
@opentelemetry/core 1.14.0 872.87 kB 1.47 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
@opentelemetry/api 1.8.0 1.21 MB 1.21 MB
import-in-the-middle 1.7.4 70.19 kB 739.86 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
pprof-format 2.1.0 111.69 kB 111.69 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.1.0 60.23 kB 60.23 kB
ignore 5.2.4 51.22 kB 51.22 kB
int64-buffer 0.1.10 49.18 kB 49.18 kB
shell-quote 1.8.1 44.96 kB 44.96 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
tlhunter-sorted-set 0.1.0 24.94 kB 24.94 kB
limiter 1.1.5 23.17 kB 23.17 kB
dc-polyfill 0.1.4 23.1 kB 23.1 kB
retry 0.13.1 18.85 kB 18.85 kB
node-abort-controller 3.1.1 16.89 kB 16.89 kB
jest-docblock 29.7.0 8.99 kB 12.76 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 kB
module-details-from-path 1.0.3 4.47 kB 4.47 kB

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@juan-fernandez juan-fernandez force-pushed the juan-fernandez/support-parallel-mocha branch from 8d720da to 5ae4dec Compare May 17, 2024 08:42
@pr-commenter
Copy link

pr-commenter bot commented May 17, 2024

Benchmarks

Benchmark execution time: 2024-05-22 10:27:39

Comparing candidate commit ef92015 in PR branch juan-fernandez/support-parallel-mocha with baseline commit a88c272 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics.

@juan-fernandez juan-fernandez changed the title [do not merge] [wip] Support mocha parallel mode [ci-visibility] Support mocha parallel mode May 20, 2024
const testToStartLine = new WeakMap()

// mocha-each support
addHook({
Copy link
Collaborator Author

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({
Copy link
Collaborator Author

@juan-fernandez juan-fernandez May 20, 2024

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

Copy link
Collaborator

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) {
Copy link
Collaborator Author

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({
Copy link
Collaborator Author

@juan-fernandez juan-fernandez May 20, 2024

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({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This hook was added to support parallel mode


// Only used in parallel mode (--parallel flag is passed)
// Used to start and finish test session and test module
addHook({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This hook was added to support parallel mode

Copy link

codecov bot commented May 20, 2024

Codecov Report

Attention: Patch coverage is 16.36364% with 322 lines in your changes are missing coverage. Please review.

Project coverage is 86.12%. Comparing base (8f9e558) to head (229e738).
Report is 12 commits behind head on master.

Current head 229e738 differs from pull request most recent head 0a83fe8

Please upload reports for the commit 0a83fe8 to get more accurate results.

Files Patch % Lines
...ackages/datadog-instrumentations/src/mocha/main.js 14.42% 273 Missing ⚠️
...ckages/datadog-instrumentations/src/mocha/utils.js 6.06% 31 Missing ⚠️
...kages/datadog-instrumentations/src/mocha/common.js 36.00% 16 Missing ⚠️
packages/datadog-instrumentations/src/mocha.js 66.66% 1 Missing ⚠️
packages/dd-trace/src/plugins/index.js 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@juan-fernandez juan-fernandez force-pushed the juan-fernandez/support-parallel-mocha branch from 4d3edbf to 2939b88 Compare May 21, 2024 10:30
@@ -0,0 +1,23 @@
// eslint-disable-next-line
const workerpool = require('workerpool')
Copy link
Collaborator Author

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')
Copy link
Collaborator Author

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

@juan-fernandez juan-fernandez marked this pull request as ready for review May 21, 2024 13:01
@juan-fernandez juan-fernandez requested review from a team as code owners May 21, 2024 13:01
anmarchenko
anmarchenko previously approved these changes May 21, 2024
anmarchenko
anmarchenko previously approved these changes May 22, 2024
uurien
uurien previously approved these changes May 22, 2024
@juan-fernandez juan-fernandez dismissed stale reviews from uurien and anmarchenko via bb2c5fc May 22, 2024 13:53
@juan-fernandez
Copy link
Collaborator Author

ℹ️ I missed a require. I noticed it with some manual tests. I fixed it in bb2c5fc and added an assertion in the e2e tests.

@juan-fernandez
Copy link
Collaborator Author

could you have a look at the latest commit @uurien ?

@juan-fernandez juan-fernandez merged commit ead2f41 into master May 23, 2024
108 checks passed
@juan-fernandez juan-fernandez deleted the juan-fernandez/support-parallel-mocha branch May 23, 2024 08:03
This was referenced Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Mocha parallel tests
4 participants