feat: enable parallel mocha testing by raymondfeng · Pull Request #5011 · loopbackio/loopback-next

raymondfeng · 2020-04-02T00:02:55Z

Enable parallel mocha testing - see #3159 (comment)

I'm starting to break down this PR into a few smaller ones:

refactor: use sandbox.path instead of SANDBOX to access the sandbox directory refactor: use sandbox.path instead of SANDBOX to access the sandbox directory #5014
fix(example-express-composition): use an assinged port number for testing fix(example-express-composition): use an assinged port number for testing #5022
feat(core): improve application signal handler registration feat(core): improve application signal handler registration #5020
feat: improve test sandbox feat: improve test sandbox #5016
fix(http-server): ensure rejection handler is registered before the error event is emitted fix(http-server): ensure rejection handler is registered before the error event is emitted #5031
fix(context): fix context subscription manager to not emit errors to its context if there is an error listener is in place fix(context): fix context subscription manager to not emit errors to its context if there is an error listener is in place #5021
Rework snapshot matcher to support parallel mocha Rework snapshot matcher to support parallel mocha #5813

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

raymondfeng · 2020-04-05T20:50:19Z

The snapshot-related cli tests are still failing. I think I have a theory as follows:

The snapshots.js creates a global beforeEach hook (which is done once per process with node.js module require)
Parallel mocha uses a pool of worker processes
When the worker process tests the 1st file, beforeEach is correctly called
When the worker process is reused to test a 2nd file, beforeEach is not available to mocha as the node require cache is not flushed. As a result, beforeEach is not invoked to refresh currentTest
As a result, the last currentTest from the 1st test file is accidentally used for other tests and it fails

raymondfeng · 2020-04-05T20:52:23Z

@boneskull Is it possible to have something as follows to be picked up by mocha without ---require or --file?

'use strict';
exports.mochaHooks = {
  beforeAll() {
    console.log('beforeAll');
  },
  beforeEach() {
    console.log('beforeEach');
  },
  afterAll() {
    console.log('afterAll');
  },
  afterEach() {
    console.log('afterEach');
  }
};

bajtos

I am not happy with using root-level hooks for snapshot setup and want to investigate different approaches to see if I can find a more elegant solution. I am planning to work on this tomorrow (Friday), please give me few more days before continuing with the solution proposed here.

packages/build/config/.mocharc.json

bajtos · 2020-06-11T07:52:06Z

Let's not mix the upgrade to Mocha v8.x with the transition to parallel tests please. I'd like to see a standalone PR to upgrade to Mocha 8.x and make the necessary updates in our config files. I think commits like cbba724 and f9af11e can be landed independently too.

Mocha 8.x dropped support for mocha.opts file, which we still support in run-mocha script. This should be fixed as part of the upgrade to Mocha 8.x.

boneskull · 2020-06-11T17:10:56Z

agree that parallel mode shouldn’t be the default for users.

BREAKING CHANGE: We have upgraded to `mocha@8.0.1` and removed support for `--opts` and `test/mocha.opts`. It may break your application if it depends on earlier version of `@loopback/build` for `npm test`. See a list of breaking changes of mocha 8.x at: https://github.com/mochajs/mocha/releases/tag/v8.0.0

…ocha`

…sting - try with require to register root hooks - add .mocharc.js for packages/cli to register root hooks - add the option to use env var MOCHA_JOBS to override parallel testing parameters

bajtos · 2020-06-12T07:30:28Z

@raymondfeng can you please extract 7334f20 in to a standalone pull request to get it landed sooner, and make it easier for others (me in particular) to experiment with different approaches to parallel testing?

bajtos · 2020-06-12T08:10:58Z

.mocharc.js

+// memory leak detected. 11 SIGTERM listeners added to [process].
+// Use emitter.setMaxListeners() to increase limit
+// It only happens when multiple app instances are started but not stopped
+process.setMaxListeners(16);


How is the change in setMaxListeners(16) relevant to snapshot matching? I think this belongs to a different commit.

bajtos · 2020-06-12T08:12:32Z

packages/cli/.mocharc.js

+  // Resolve `./test/snapshot-matcher.js` to get the absolute path
+  const mochaHooksFile = require.resolve('./test/snapshot-matcher.js');
+  debug('Root hooks for --require %s', mochaHooksFile);
+  const config = {...mochaConfig, timeout: 5000};


I find this approach brittle because it requires us to deal with root hooks both inside the package test setup and in monorepo test setup.

In #5724, I have proposed a fix that does not leak the knowledge about snapshot hooks outside of CLI tests.

bajtos · 2020-06-12T08:16:23Z

packages/build/bin/run-mocha.js

    mochaOpts.splice(lang, 2);
  }

+  // Set `--parallel` for `@loopback/*` packages


I am not sure if this is a good idea, because it affects code outside our monorepo too. Two examples:

Example projects created via lb4 example <name>. Besides the extra complexity related to parallel testing, I am concerned about the impact on performance. Based on my measurement, examples/todo tests are ~15x slower when executed in parallel:

examples/todo$ npm t (...) 29 passing (481ms) examples/todo$ npm t -- --parallel (...) 29 passing (7s)

External repositories like https://github.com/strongloop/loopback4-example-shopping. I am not sure how much work is required to support parallel testing in the shopping example and I would rather not block upgrading to the latest @loopback/build version until we can figure out parallel testing.

I am proposing to enable parallel testing by changing npm run mocha script to add --parallel flag to mocha CLI options.

See e.g. b44c7ee

-"mocha": "node packages/build/bin/run-mocha --lang en_US.UTF-8 --timeout 5000 \"packages/*/dist/__tests__/**/*.js\" \"extensions/*/dist/__tests__/**/*.js\" \"examples/*/dist/__tests__/**/*.js\" \"packages/cli/test/**/*.js\" \"packages/build/test/*/*.js\"", +"mocha": "node packages/build/bin/run-mocha --lang en_US.UTF-8 --timeout 15000 --parallel \"packages/*/dist/__tests__/**/*.js\" \"extensions/*/dist/__tests__/**/*.js\" \"examples/*/dist/__tests__/**/*.js\" \"packages/cli/test/**/*.js\" \"packages/build/test/*/*.js\"",

Having said that, I think it's time to move flags from run-mocha CLI args into the monorepo-level .mocharc.js file. That way they are always applied, for example when running a subset of tests manually via node packages/build/bin/run-mocha path/to/some/files.

bajtos · 2020-06-23T15:27:44Z

Opened #5813 to rework the snapshot matcher to use Mocha global hooks, without actually enabling the parallel mode yet.

bajtos · 2020-06-26T08:31:40Z

packages/cli/.mocharc.js

+  const config = {...mochaConfig, timeout: 5000};
+
+  // Allow env var `MOCHA_JOBS` to override parallel testing parameters
+  const jobs = +process.env.MOCHA_JOBS;


I feel we don't need to support process.env.MOCHA_JOBS as it's already possible to change the number of jobs (worker processes) by using -j command line argument and disable parallel execution by setting the number of jobs to 1.

$ npm run mocha -- -j 1

bajtos · 2020-06-26T08:41:26Z

I am proposing to close this pull request in favor of #5831.

raymondfeng requested a review from bajtos April 2, 2020 00:03

This was referenced Apr 2, 2020

refactor: use sandbox.path instead of SANDBOX to access the sandbox directory #5014

Merged

chore(core): set max listeners to 256 for process to avoid warnings #5015

Closed

feat: improve test sandbox #5016

Merged

raymondfeng force-pushed the test-parallel-mocha branch from 4703747 to 14eb31a Compare April 2, 2020 20:02

raymondfeng added the developer-experience Issues affecting ease of use and overall experience of LB users label Apr 2, 2020

raymondfeng force-pushed the test-parallel-mocha branch 10 times, most recently from 87fc694 to b0a93e8 Compare April 5, 2020 18:30

raymondfeng force-pushed the test-parallel-mocha branch 12 times, most recently from dcb0093 to 377a949 Compare April 10, 2020 05:56

raymondfeng changed the title ~~[RFC] Enable parallel mocha testing~~ feat: enable parallel mocha testing Jun 10, 2020

raymondfeng marked this pull request as ready for review June 10, 2020 23:37

raymondfeng requested review from agnes512, emonddr and jannyHou as code owners June 10, 2020 23:37

bajtos suggested changes Jun 11, 2020

View reviewed changes

bajtos reviewed Jun 11, 2020

View reviewed changes

packages/build/config/.mocharc.json Outdated Show resolved Hide resolved

raymondfeng mentioned this pull request Jun 11, 2020

fix(boot): make sure sandbox directories are removed after tests #5720

Merged

7 tasks

raymondfeng force-pushed the test-parallel-mocha branch 3 times, most recently from 5d9b6b8 to 0268f67 Compare June 11, 2020 20:44

raymondfeng added 3 commits June 11, 2020 16:20

feat(cli): enable parallel mode for @loopback/* packages with `lb-m…

b0e956c

…ocha`

feat(cli): improve snapshot matcher to be compatible with parallel te…

20da28e

…sting - try with require to register root hooks - add .mocharc.js for packages/cli to register root hooks - add the option to use env var MOCHA_JOBS to override parallel testing parameters

raymondfeng force-pushed the test-parallel-mocha branch from 0268f67 to 20da28e Compare June 11, 2020 23:22

raymondfeng requested a review from bajtos June 12, 2020 04:50

bajtos reviewed Jun 12, 2020

View reviewed changes

bajtos suggested changes Jun 12, 2020

View reviewed changes

bajtos mentioned this pull request Jun 12, 2020

test(cli): rework snapshots to support parallel test execution #5724

Closed

4 tasks

bajtos mentioned this pull request Jun 23, 2020

Rework snapshot matcher to support parallel mocha #5813

Merged

3 tasks

bajtos reviewed Jun 26, 2020

View reviewed changes

bajtos mentioned this pull request Jun 26, 2020

test: enable parallel execution of Mocha tests #5831

Merged

7 tasks

raymondfeng closed this Jun 27, 2020

bajtos mentioned this pull request Sep 11, 2020

Spike: migrate tests to Jest (ts-jest) #3159

Closed

5 tasks

Conversation

raymondfeng commented Apr 2, 2020 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

raymondfeng commented Apr 5, 2020

Uh oh!

raymondfeng commented Apr 5, 2020

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos commented Jun 11, 2020

Uh oh!

boneskull commented Jun 11, 2020

Uh oh!

bajtos commented Jun 12, 2020

Uh oh!

bajtos Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 23, 2020

Uh oh!

bajtos Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

raymondfeng commented Apr 2, 2020 •

edited by bajtos

Loading

bajtos Jun 12, 2020 •

edited

Loading

bajtos Jun 12, 2020 •

edited

Loading

bajtos Jun 12, 2020 •

edited

Loading