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

Improved watching with chokidar #3980

Merged
merged 2 commits into from Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 27 additions & 7 deletions docs/index.md
Expand Up @@ -1080,16 +1080,14 @@ Specify an explicit path to a [`package.json` file](#configuring-mocha-nodejs) (

By default, Mocha looks for a `package.json` in the current working directory or nearest ancestor, and will use the first file found (regardless of whether it contains a `mocha` property); to suppress `package.json` lookup, use `--no-package`.

### `--extension <ext>, --watch-extensions <ext>`

> _Updated in v6.0.0. Previously `--watch-extensions`, but now expanded to affect general test file loading behavior. `--watch-extensions` is now an alias_
### `--extension <ext>`

Files having this extension will be considered test files. Defaults to `js`.
juergba marked this conversation as resolved.
Show resolved Hide resolved

Affects `--watch` behavior.

juergba marked this conversation as resolved.
Show resolved Hide resolved
Specifying `--extension` will _remove_ `.js` as a test file extension; use `--extension js` to re-add it. For example, to load `.mjs` and `.js` test files, you must supply `--extension mjs --extension js`.

The option can be given multiple times. The option accepts a comma-delimited list: `--extension a,b` is equivalent to `--extension a --extension b`

### `--file <file|directory|glob>`

Explicitly _include_ a test file to be loaded before other test files. Multiple uses of `--file` are allowed, and will be loaded in order given.
Expand Down Expand Up @@ -1133,9 +1131,31 @@ Sort test files (by absolute path) using [Array.prototype.sort][mdn-array-sort].

### `--watch, -w`

Executes tests on changes to JavaScript in the current working directory (and once initially).
Rerun tests on file changes.

The `--watch-files` and `--watch-ignore` options can be used to control which files are watched for changes.

### `--watch-files <file|directory|glob>`

> _New in v7.0.0_

List of paths or globs to watch when `--watch` is set. If a file matching the given glob changes or is added or removed mocha will rerun all tests.

If the path is a directory all files and subdirectories will be watched.

By default all files in the current directory having one of the extensions provided by `--extension` and not contained in the `node_modules` or `.git` folders are watched.

The option can be given multiple times. The option accepts a comma-delimited list: `--watch-files a,b` is equivalent to `--watch-files a --watch-files b`

### `--watch-ignore <file|directory|glob>`

> _New in v7.0.0_

List of paths or globs to exclude from watching. Defaults to `node_modules` and `.git`.

To exclude all files in a directory it is preferable to use `foo/bar` instead of `foo/bar/**/*`. The latter will still watch the directory `foo/bar` but will ignore all changes to the content of that directory.

By default, only files with extension `.js` are watched. Use `--extension` to change this behavior.
The option can be given multiple times. The option accepts a comma-delimited list: `--watch-ignore a,b` is equivalent to `--watch-ignore a --watch-ignore b`

### `--fgrep <string>, -f <string>`

Expand Down
4 changes: 3 additions & 1 deletion example/config/.mocharc.js
Expand Up @@ -12,5 +12,7 @@ module.exports = {
reporter: 'spec',
slow: 75,
timeout: 2000,
ui: 'bdd'
ui: 'bdd',
'watch-files': ['lib/**/*.js', 'test/**/*.js'],
'watch-ignore': ['lib/vendor']
};
4 changes: 3 additions & 1 deletion example/config/.mocharc.json
Expand Up @@ -10,5 +10,7 @@
"reporter": "spec",
"slow": 75,
"timeout": 2000,
"ui": "bdd"
"ui": "bdd",
"watch-files": ["lib/**/*.js", "test/**/*.js"],
"watch-ignore": ["lib/vendor"]
}
5 changes: 4 additions & 1 deletion example/config/.mocharc.jsonc
Expand Up @@ -10,5 +10,8 @@
"reporter": /* 📋 */ "spec",
"slow": 75,
"timeout": 2000,
"ui": "bdd"
"ui": "bdd",
// Camel-casing options are also accepted
"watchFiles": ["lib/**/*.js", "test/**/*.js"],
juergba marked this conversation as resolved.
Show resolved Hide resolved
"watchIgnore": ["lib/vendor"]
}
5 changes: 5 additions & 0 deletions example/config/.mocharc.yml
Expand Up @@ -44,3 +44,8 @@ trace-warnings: true # node flags ok
ui: bdd
v8-stack-trace-limit: 100 # V8 flags are prepended with "v8-"
watch: false
watch-files:
- 'lib/**/*.js'
- 'test/**/*.js'
watch-ignore:
- 'lib/vendor'
7 changes: 4 additions & 3 deletions lib/cli/run-helpers.js
Expand Up @@ -118,13 +118,14 @@ exports.runMocha = (mocha, options) => {
const {
watch = false,
extension = [],
ui = 'bdd',
exit = false,
ignore = [],
file = [],
recursive = false,
sort = false,
spec = []
spec = [],
watchFiles,
watchIgnore
juergba marked this conversation as resolved.
Show resolved Hide resolved
} = options;

const fileCollectParams = {
Expand All @@ -137,7 +138,7 @@ exports.runMocha = (mocha, options) => {
};

if (watch) {
watchRun(mocha, {ui}, fileCollectParams);
watchRun(mocha, {watchFiles, watchIgnore}, fileCollectParams);
} else {
exports.singleRun(mocha, {exit}, fileCollectParams);
}
Expand Down
7 changes: 4 additions & 3 deletions lib/cli/run-option-metadata.js
Expand Up @@ -18,9 +18,11 @@ exports.types = {
'file',
'global',
'ignore',
'require',
'reporter-option',
'spec'
'require',
'spec',
'watch-files',
'watch-ignore'
Copy link
Member

@juergba juergba Aug 14, 2019

Choose a reason for hiding this comment

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

Something is wrong with loadOptions() in "cli/options". I have to check the camel-case option of yargs-parser.
When I use --watch-files <path> the result is - as it should be - an array , when I use --watchFiles <path> it's a string.
Inputs of watchFiles via CLI plus config files are not combined correctly this way.

Copy link
Member

@juergba juergba Aug 14, 2019

Choose a reason for hiding this comment

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

Test case:

  • input via CLI: --watchFiles ./path1 --watchFiles ./path2
  • plus input via .mocharc.yml: watchFiles: ['./path3']

current result: {watchFiles: [ "./path1", "./path2"]
Makes sense, since the type of watchFiles is undefined, not array.

Setting camel-case-expansion to true (in "cli/options"), it's still wrong:
result: {watch-files: [ "./path1", "./path2"], {watchFiles: [ "./path1", "./path2"]}
This output seems to be a yargs-parser bug, since an arraytyped option should combine the result.

When I use --watch-files instead of --watchFiles the output is correct:
result: {watch-files: [ "./path1", "./path2, "./path3"]}

So the result is due to our camel-case settings and also due to a yargs-parser bug. I don't want to set a quick shot. Do you have an idea, otherwise let's go on and take care of this later.
see: yargs-parser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an idea

Unfortunately I don’t. I’m not familiar with yargs’ configuration loading.

Copy link
Member

Choose a reason for hiding this comment

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

I will open a separate issue for this parsing issue.

],
boolean: [
'allow-uncaught',
Expand Down Expand Up @@ -68,7 +70,6 @@ exports.aliases = {
'async-only': ['A'],
bail: ['b'],
color: ['c', 'colors'],
extension: ['watch-extensions'],
fgrep: ['f'],
global: ['globals'],
grep: ['g'],
Expand Down
15 changes: 14 additions & 1 deletion lib/cli/run.js
Expand Up @@ -88,7 +88,7 @@ exports.builder = yargs =>
extension: {
default: defaults.extension,
defaultDescription: 'js',
description: 'File extension(s) to load and/or watch',
description: 'File extension(s) to load',
group: GROUPS.FILES,
requiresArg: true,
coerce: list
Expand Down Expand Up @@ -241,6 +241,19 @@ exports.builder = yargs =>
watch: {
description: 'Watch files in the current working directory for changes',
group: GROUPS.FILES
},
'watch-files': {
description: 'List of paths or globs to watch',
group: GROUPS.FILES,
requiresArg: true,
coerce: list
},
'watch-ignore': {
description: 'List of paths or globs to exclude from watching',
group: GROUPS.FILES,
requiresArg: true,
coerce: list,
default: defaults['watch-ignore']
}
})
.positional('spec', {
Expand Down
137 changes: 108 additions & 29 deletions lib/cli/watch-run.js
@@ -1,8 +1,8 @@
'use strict';

const utils = require('../utils');
const path = require('path');
const chokidar = require('chokidar');
const Context = require('../context');
const Mocha = require('../mocha');
const collectFiles = require('./collect-files');

/**
Expand All @@ -16,36 +16,82 @@ const collectFiles = require('./collect-files');
* Run Mocha in "watch" mode
* @param {Mocha} mocha - Mocha instance
* @param {Object} opts - Options
* @param {string} opts.ui - User interface
* @param {string[]} [opts.watchFiles] - List of paths and patterns to
* watch. If not provided all files with an extension included in
* `fileColletionParams.extension` are watched. See first argument of
* `chokidar.watch`.
* @param {string[]} opts.watchIgnore - List of paths and patterns to
* exclude from watching. See `ignored` option of `chokidar`.
* @param {Object} fileCollectParams - Parameters that control test
* file collection. See `lib/cli/collect-files.js`.
* @param {string[]} fileCollectParams.extension - List of extensions to watch
* @param {string[]} fileCollectParams.extension - List of extensions
* to watch if `opts.watchFiles` is not given.
* @private
*/
module.exports = (mocha, {ui}, fileCollectParams) => {
let runner;
const files = collectFiles(fileCollectParams);
module.exports = (mocha, {watchFiles, watchIgnore}, fileCollectParams) => {
if (!watchFiles) {
watchFiles = fileCollectParams.extension.reduce(
(watchFiles, ext) => watchFiles.concat([`**/*.${ext}`, `**/.*.${ext}`]),
[]
);
}

const watcher = chokidar.watch(watchFiles, {
ignored: watchIgnore,
ignoreInitial: true
});

const rerunner = createRerunner(mocha, () => {
getWatchedFiles(watcher).forEach(file => {
delete require.cache[file];
});
mocha.files = collectFiles(fileCollectParams);
});

watcher.on('ready', () => {
rerunner.run();
});

watcher.on('all', () => {
rerunner.scheduleRun();
});

console.log();
hideCursor();
process.on('exit', () => {
showCursor();
});
process.on('SIGINT', () => {
showCursor();
console.log('\n');
// By UNIX/Posix convention this indicates that the process was
// killed by SIGINT which has portable number 2.
process.exit(128 + 2);
});
};

/**
* Create an object that allows you to rerun tests on the mocha
* instance. `beforeRun` is called everytime before `mocha.run()` is
* called.
*
* @param {Mocha} mocha - Mocha instance
* @param {function} beforeRun - Called just before `mocha.run()`
*/
const createRerunner = (mocha, beforeRun) => {
// Set to a `Runner` when mocha is running. Set to `null` when mocha is not
// running.
let runner = null;

const watchFiles = utils.files(process.cwd(), fileCollectParams.extension);
let runAgain = false;
let rerunScheduled = false;

const loadAndRun = () => {
const run = () => {
try {
mocha.files = files;
runAgain = false;
beforeRun();
resetMocha(mocha);
Copy link
Member

Choose a reason for hiding this comment

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

Even for the first run - on the ready event - we reset everything, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is not ideal but it avoids special casing the initial run.

runner = mocha.run(() => {
runner = null;
if (runAgain) {
if (rerunScheduled) {
rerun();
}
});
Expand All @@ -54,29 +100,62 @@ module.exports = (mocha, {ui}, fileCollectParams) => {
}
};

const purge = () => {
watchFiles.forEach(Mocha.unloadFile);
};

loadAndRun();

const rerun = () => {
purge();
eraseLine();
mocha.suite = mocha.suite.clone();
mocha.suite.ctx = new Context();
mocha.ui(ui);
loadAndRun();
};
const scheduleRun = () => {
if (rerunScheduled) {
return;
}

utils.watch(watchFiles, () => {
runAgain = true;
rerunScheduled = true;
if (runner) {
runner.abort();
} else {
rerun();
}
};
Copy link
Member

@juergba juergba Aug 16, 2019

Choose a reason for hiding this comment

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

Is this the common behavior of a watch functionality to abort the tests, instead of waiting for the test end and then restarting?

Out of topic: I don't trust runner.abort(). I doubt that the hook pattern is correct and all afterEach / after hooks run for correct tear-down. This is prone to errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same behavior as before. I think it makes sense to abort the tests when a file changes. If you usually want to get quick feedback when your code changes.


const rerun = () => {
rerunScheduled = false;
eraseLine();
run();
};

return {
scheduleRun,
run
};
};

/**
* Return the list of absolute paths watched by a chokidar watcher.
*
* @param watcher - Instance of a chokidar watcher
* @return {string[]} - List of absolute paths
*/
const getWatchedFiles = watcher => {
const watchedDirs = watcher.getWatched();
let watchedFiles = [];
Object.keys(watchedDirs).forEach(dir => {
watchedFiles = watchedFiles.concat(
watchedDirs[dir].map(file => path.join(dir, file))
);
});
return watchedFiles;
};

/**
* Reset the internal state of the mocha instance so that tests can be rerun.
*
* @param {Mocha} mocha - Mocha instance
* @private
*/
const resetMocha = mocha => {
mocha.unloadFiles();
juergba marked this conversation as resolved.
Show resolved Hide resolved
mocha.suite = mocha.suite.clone();
mocha.suite.ctx = new Context();
// Registers a callback on `mocha.suite` that wires new context to the DSL
// (e.g. `describe`) that is exposed as globals when the test files are
// reloaded.
mocha.ui(mocha.options.ui);
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the meaning of this. We had a similar situation with grep in another PR.
ui is set by the Mocha constructor and is read once when Mocha is launched. There is no need to reset it here again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment in 4e7c743 that explains why we need it. If you look at the code of mocha.ui() you see that it registers some hook on mocha.suite that is makes the global DSL (e.g. describe) work. If we omit the call to mocha.ui() the new test suites will never be registered.

};

/**
Expand Down
3 changes: 2 additions & 1 deletion lib/mocharc.json
Expand Up @@ -6,5 +6,6 @@
"reporter": "spec",
"slow": 75,
"timeout": 2000,
"ui": "bdd"
"ui": "bdd",
"watch-ignore": ["node_modules", ".git"]
}