-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: enable parallel mocha testing #5011
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
Changes from all commits
7334f20
b0e956c
20da28e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Copyright IBM Corp. 2020. All Rights Reserved. | ||
| // Node module: loopback-next | ||
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| // Reuse the mocha config from `@loopback/cli` | ||
| const config = require('./packages/cli/.mocharc.js'); | ||
|
|
||
| // Set max listeners to 16 for testing to avoid the following warning: | ||
| // (node:11220) MaxListenersExceededWarning: Possible EventEmitter | ||
| // 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is the change in |
||
|
|
||
| module.exports = config; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,9 @@ Usage: | |
|
|
||
| 'use strict'; | ||
|
|
||
| const path = require('path'); | ||
| const fs = require('fs-extra'); | ||
|
|
||
| function run(argv, options) { | ||
| const utils = require('./utils'); | ||
|
|
||
|
|
@@ -24,7 +27,6 @@ function run(argv, options) { | |
| !utils.isOptionSet( | ||
| mochaOpts, | ||
| '--config', // mocha 6.x | ||
| '--opts', // legacy | ||
| '--package', // mocha 6.x | ||
| '--no-config', // mocha 6.x | ||
| ) && !utils.mochaConfiguredForProject(); | ||
|
|
@@ -59,6 +61,17 @@ function run(argv, options) { | |
| mochaOpts.splice(lang, 2); | ||
| } | ||
|
|
||
| // Set `--parallel` for `@loopback/*` packages | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if this is a good idea, because it affects code outside our monorepo too. Two examples:
I am proposing to enable parallel testing by changing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| if (!mochaOpts.includes('--parallel')) { | ||
| const pkgFile = path.join(utils.getPackageDir(), 'package.json'); | ||
| if (fs.existsSync(pkgFile)) { | ||
| const pkg = fs.readJsonSync(pkgFile); | ||
| if (pkg.name.startsWith('@loopback/')) { | ||
| mochaOpts.push('--parallel'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const args = [...mochaOpts]; | ||
|
|
||
| return utils.runCLI('mocha/bin/mocha', args, options); | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // Copyright IBM Corp. 2020. All Rights Reserved. | ||
| // Node module: @loopback/cli | ||
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| // A workaround to use `--require ./test/snapshot-matcher.js` so that root | ||
| // hooks are executed by mocha parallel testing for each job | ||
|
|
||
| const debug = require('debug')('loopback:cli:test'); | ||
|
|
||
| /** | ||
| * Build mocha config for `@loopback/cli` | ||
| */ | ||
| function buildConfig() { | ||
| // Use the default config from `@loopback/build` | ||
| const mochaConfig = require('@loopback/build/config/.mocharc.json'); | ||
| debug('Default mocha config:', mochaConfig); | ||
| // 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}; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #5724, I have proposed a fix that does not leak the knowledge about snapshot hooks outside of CLI tests. |
||
|
|
||
| // Allow env var `MOCHA_JOBS` to override parallel testing parameters | ||
| const jobs = +process.env.MOCHA_JOBS; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel we don't need to support |
||
| if (jobs === 0) { | ||
| // Disable parallel testing | ||
| config.parallel = false; | ||
| } else if (jobs > 0) { | ||
| // Override the default number of concurrent jobs | ||
| config.parallel = true; | ||
| config.jobs = jobs; | ||
| } | ||
| addRequire(config, mochaHooksFile); | ||
| debug('Final mocha config:', config); | ||
| return config; | ||
| } | ||
|
|
||
| /** | ||
| * Add a new entry to the mocha config.require | ||
| * @param {object} config - Mocha config | ||
| * @param {string} mochaHooksFile - A module to be loaded by mocha | ||
| */ | ||
| function addRequire(config, mochaHooksFile) { | ||
| if (typeof config.require === 'string') { | ||
| config.require = [config.require, mochaHooksFile]; | ||
| } else if (Array.isArray(config.require)) { | ||
| config.require = config.require.concat(mochaHooksFile); | ||
| } else { | ||
| config.require = mochaHooksFile; | ||
| } | ||
| } | ||
|
|
||
| module.exports = buildConfig(); | ||
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.
I think it's a problem in our test suite when we start apps but don't stop them. Instead of suppressing the helpful warning, can we please fix our tests to always stop the applications started?
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.
I cannot find any more places that
stopis not called withstart.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.
I am running Node.js 14.4.0 and haven't encountered
MaxListenersExceededWarning. Could you please double check that this is still a problem? Is it perhaps specific to older versions of Node or a timing conditions on CI build servers?