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.

Sign up for GitHub

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

Basic watcher setup #465

Closed
wants to merge 2 commits into from
Closed

Basic watcher setup #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df37cfb
Basic watcher setup
tomazzaman Jan 23, 2016
0a18f8a
Watcher should ignore node_modules and git files
tomazzaman Jan 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 35 additions & 15 deletions cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var meow = require('meow');
var Promise = require('bluebird');
var pkgConf = require('pkg-conf');
var isCi = require('is-ci');
var chokidar = require('chokidar');
var colors = require('./lib/colors');
var verboseReporter = require('./lib/reporters/verbose');
var miniReporter = require('./lib/reporters/mini');
Expand All @@ -48,6 +49,7 @@ var cli = meow([
' --tap, -t Generate TAP output',
' --verbose, -v Enable verbose output',
' --no-cache Disable the transpiler cache',
' --watch, -w Run tests when files change',
Copy link
Member

Choose a reason for hiding this comment

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

This should be commented out. We decided to not yet expose this. #70 (comment)

'',
'Examples',
' ava',
Expand All @@ -68,14 +70,16 @@ var cli = meow([
'fail-fast',
'verbose',
'serial',
'tap'
'tap',
'watch'
],
default: conf,
alias: {
t: 'tap',
v: 'verbose',
r: 'require',
s: 'serial'
s: 'serial',
w: 'watch'
}
});

Expand All @@ -95,6 +99,7 @@ var api = new Api(cli.input.length ? cli.input : arrify(conf.files), {

var logger = new Logger();
logger.api = api;
logger.watch = cli.flags.watch;

if (cli.flags.tap) {
logger.use(tapReporter());
Expand All @@ -112,17 +117,32 @@ api.on('error', logger.unhandledError);
api.on('stdout', logger.stdout);
api.on('stderr', logger.stderr);

api.run()
.then(function () {
logger.finish();
logger.exit(api.failCount > 0 || api.rejectionCount > 0 || api.exceptionCount > 0 ? 1 : 0);
})
.catch(function (err) {
if (err.name === 'AvaError') {
console.log(' ' + colors.error(figures.cross) + ' ' + err.message);
} else {
console.error(colors.stack(err.stack));
}

logger.exit(1);
function run() {
api.run()
.then(function () {
logger.finish();
logger.exit(api.failCount > 0 || api.rejectionCount > 0 || api.exceptionCount > 0 ? 1 : 0);
})
.catch(function (err) {
if (err.name === 'AvaError') {
console.log(' ' + colors.error(figures.cross) + ' ' + err.message);
} else {
console.error(colors.stack(err.stack));
}

logger.exit(1);
});
}

run();

var cwd = process.cwd();
var watcher = chokidar.watch(cwd, {
persistent: cli.flags.watch,
Copy link
Member

Choose a reason for hiding this comment

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

I think no chokidar code should be run unless cli.flags.watch is true. Currently if it's false it means Chokidar probably won't emit changes, but I don't find the documentation entirely clear on this.

ignored: [cwd + '/node_modules', cwd + '/.git'],
Copy link
Member

Choose a reason for hiding this comment

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

You may be able to specify the cwd option and then keep these patterns relative. Might not hurt excluding other VCS paths either.

Copy link
Member

Choose a reason for hiding this comment

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

Should also use path.join()

followSymlinks: false
});

watcher.on('change', function (filepath) {
run();
Copy link
Member

Choose a reason for hiding this comment

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

Since the tests run asynchronously, you may end up starting a new batch before the previous execution has finished. It doesn't look like api.run() has any logic to defend against that (which it wouldn't have needed before).

Also filepath is unused.

Copy link
Member

Choose a reason for hiding this comment

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

👍

});
4 changes: 4 additions & 0 deletions lib/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ Logger.prototype.exit = function (code) {
process.stdout.write('');
process.stderr.write('');

if (this.watch) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO you should place this guard in the cli module, the logger shouldn't have to know about watchers.

Copy link
Member

Choose a reason for hiding this comment

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

👍

return;
}

// timeout required to correctly flush IO on Node.js 0.10 on Windows
setTimeout(function () {
process.exit(code);
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
"bluebird": "^3.0.0",
"caching-transform": "^1.0.0",
"chalk": "^1.0.0",
"chokidar": "^1.4.2",
"cli-cursor": "^1.0.2",
"co-with-promise": "^4.6.0",
"commondir": "^1.0.1",
Expand Down