From fdd797082cd78ff956f677038745f6f7518922bf Mon Sep 17 00:00:00 2001 From: James Talmage Date: Fri, 20 May 2016 01:09:57 -0400 Subject: [PATCH] refactor watcher (#766) * refactor watcher tests * move some methods from watcher to AvaFiles * add some tests for ava-files * add more test coverage * fix linter error after merge * PR Feedback * use cross-platform split --- lib/ava-files.js | 154 ++++++++++++++++++++++++++++++++++++++++++- lib/watcher.js | 164 +++------------------------------------------- test/ava-files.js | 97 +++++++++++++++++++++++++++ test/watcher.js | 137 +++++++++++++++++++------------------- 4 files changed, 327 insertions(+), 225 deletions(-) create mode 100644 test/ava-files.js diff --git a/lib/ava-files.js b/lib/ava-files.js index f1ce69746..6d97302a7 100644 --- a/lib/ava-files.js +++ b/lib/ava-files.js @@ -4,6 +4,8 @@ var Promise = require('bluebird'); var slash = require('slash'); var globby = require('globby'); var flatten = require('arr-flatten'); +var defaultIgnore = require('ignore-by-default').directories(); +var multimatch = require('multimatch'); function defaultExcludePatterns() { return [ @@ -23,7 +25,7 @@ function defaultIncludePatterns() { ]; } -function AvaFiles(files) { +function AvaFiles(files, sources) { if (!(this instanceof AvaFiles)) { throw new TypeError('Class constructor AvaFiles cannot be invoked without \'new\''); } @@ -35,12 +37,162 @@ function AvaFiles(files) { this.excludePatterns = defaultExcludePatterns(); this.files = files; + this.sources = sources || []; } AvaFiles.prototype.findTestFiles = function () { return handlePaths(this.files, this.excludePatterns); }; +function getDefaultIgnorePatterns() { + return defaultIgnore.map(function (dir) { + return dir + '/**/*'; + }); +} + +// Used on paths before they're passed to multimatch to harmonize matching +// across platforms. +var matchable = process.platform === 'win32' ? slash : function (path) { + return path; +}; + +AvaFiles.prototype.makeSourceMatcher = function () { + var mixedPatterns = []; + var defaultIgnorePatterns = getDefaultIgnorePatterns(); + var overrideDefaultIgnorePatterns = []; + + var hasPositivePattern = false; + this.sources.forEach(function (pattern) { + mixedPatterns.push(pattern); + // TODO: why not just pattern[0] !== '!' + if (!hasPositivePattern && pattern[0] !== '!') { + hasPositivePattern = true; + } + + // Extract patterns that start with an ignored directory. These need to be + // rematched separately. + if (defaultIgnore.indexOf(pattern.split('/')[0]) >= 0) { + overrideDefaultIgnorePatterns.push(pattern); + } + }); + + // Same defaults as used for Chokidar. + if (!hasPositivePattern) { + mixedPatterns = ['package.json', '**/*.js'].concat(mixedPatterns); + } + + return function (path) { + path = matchable(path); + + // Ignore paths outside the current working directory. They can't be matched + // to a pattern. + if (/^\.\.\//.test(path)) { + return false; + } + + var isSource = multimatch(path, mixedPatterns).length === 1; + if (!isSource) { + return false; + } + + var isIgnored = multimatch(path, defaultIgnorePatterns).length === 1; + if (!isIgnored) { + return true; + } + + var isErroneouslyIgnored = multimatch(path, overrideDefaultIgnorePatterns).length === 1; + if (isErroneouslyIgnored) { + return true; + } + + return false; + }; +}; + +AvaFiles.prototype.makeTestMatcher = function () { + var excludePatterns = this.excludePatterns; + var initialPatterns = this.files.concat(excludePatterns); + + return function (filepath) { + // Like in api.js, tests must be .js files and not start with _ + if (path.extname(filepath) !== '.js' || path.basename(filepath)[0] === '_') { + return false; + } + + // Check if the entire path matches a pattern. + if (multimatch(matchable(filepath), initialPatterns).length === 1) { + return true; + } + + // Check if the path contains any directory components. + var dirname = path.dirname(filepath); + if (dirname === '.') { + return false; + } + + // Compute all possible subpaths. Note that the dirname is assumed to be + // relative to the working directory, without a leading `./`. + var subpaths = dirname.split(/[\\\/]/).reduce(function (subpaths, component) { + var parent = subpaths[subpaths.length - 1]; + if (parent) { + // Always use / to makes multimatch consistent across platforms. + subpaths.push(parent + '/' + component); + } else { + subpaths.push(component); + } + return subpaths; + }, []); + + // Check if any of the possible subpaths match a pattern. If so, generate a + // new pattern with **/*.js. + var recursivePatterns = subpaths.filter(function (subpath) { + return multimatch(subpath, initialPatterns).length === 1; + }).map(function (subpath) { + // Always use / to makes multimatch consistent across platforms. + return subpath + '/**/*.js'; + }); + + // See if the entire path matches any of the subpaths patterns, taking the + // excludePatterns into account. This mimicks the behavior in api.js + return multimatch(matchable(filepath), recursivePatterns.concat(excludePatterns)).length === 1; + }; +}; + +AvaFiles.prototype.getChokidarPatterns = function () { + var paths = []; + var ignored = []; + + this.sources.forEach(function (pattern) { + if (pattern[0] === '!') { + ignored.push(pattern.slice(1)); + } else { + paths.push(pattern); + } + }); + + // Allow source patterns to override the default ignore patterns. Chokidar + // ignores paths that match the list of ignored patterns. It uses anymatch + // under the hood, which supports negation patterns. For any source pattern + // that starts with an ignored directory, ensure the corresponding negation + // pattern is added to the ignored paths. + var overrideDefaultIgnorePatterns = paths.filter(function (pattern) { + return defaultIgnore.indexOf(pattern.split('/')[0]) >= 0; + }).map(function (pattern) { + return '!' + pattern; + }); + ignored = getDefaultIgnorePatterns().concat(ignored, overrideDefaultIgnorePatterns); + + if (paths.length === 0) { + paths = ['package.json', '**/*.js']; + } + paths = paths.concat(this.files); + + return { + paths: paths, + ignored: ignored + }; +}; + function handlePaths(files, excludePatterns) { // convert pinkie-promise to Bluebird promise files = Promise.resolve(globby(files.concat(excludePatterns))); diff --git a/lib/watcher.js b/lib/watcher.js index 8d0cd244d..a2ae9569d 100644 --- a/lib/watcher.js +++ b/lib/watcher.js @@ -5,9 +5,6 @@ var diff = require('arr-diff'); var flatten = require('arr-flatten'); var union = require('array-union'); var uniq = require('array-uniq'); -var defaultIgnore = require('ignore-by-default').directories(); -var multimatch = require('multimatch'); -var slash = require('slash'); var AvaError = require('./ava-error'); var AvaFiles = require('./ava-files'); @@ -27,22 +24,11 @@ function rethrowAsync(err) { }); } -function getDefaultIgnorePatterns() { - return defaultIgnore.map(function (dir) { - return dir + '/**/*'; - }); -} - -// Used on paths before they're passed to multimatch to Harmonize matching -// across platforms. -var matchable = process.platform === 'win32' ? slash : function (path) { - return path; -}; - function Watcher(logger, api, files, sources) { this.debouncer = new Debouncer(this); + this.avaFiles = new AvaFiles(files, sources); - this.isTest = makeTestMatcher(files, AvaFiles.defaultExcludePatterns()); + this.isTest = this.avaFiles.makeTestMatcher(); this.clearLogOnNextRun = true; this.runVector = 0; @@ -100,15 +86,15 @@ function Watcher(logger, api, files, sources) { this.trackFailures(api); this.dirtyStates = {}; - this.watchFiles(files, sources); + this.watchFiles(); this.rerunAll(); } module.exports = Watcher; -Watcher.prototype.watchFiles = function (files, sources) { +Watcher.prototype.watchFiles = function () { var self = this; - var patterns = getChokidarPatterns(files, sources); + var patterns = this.avaFiles.getChokidarPatterns(); requireChokidar().watch(patterns.paths, { ignored: patterns.ignored, @@ -122,9 +108,10 @@ Watcher.prototype.watchFiles = function (files, sources) { }); }; -Watcher.prototype.trackTestDependencies = function (api, sources) { +Watcher.prototype.trackTestDependencies = function (api) { var self = this; - var isSource = makeSourceMatcher(sources); + var isSource = this.avaFiles.makeSourceMatcher(); + var relative = function (absPath) { return nodePath.relative('.', absPath); }; @@ -365,141 +352,6 @@ Debouncer.prototype.cancel = function () { } }; -function getChokidarPatterns(files, sources) { - var paths = []; - var ignored = []; - - sources.forEach(function (pattern) { - if (pattern[0] === '!') { - ignored.push(pattern.slice(1)); - } else { - paths.push(pattern); - } - }); - - // Allow source patterns to override the default ignore patterns. Chokidar - // ignores paths that match the list of ignored patterns. It uses anymatch - // under the hood, which supports negation patterns. For any source pattern - // that starts with an ignored directory, ensure the corresponding negation - // pattern is added to the ignored paths. - var overrideDefaultIgnorePatterns = paths.filter(function (pattern) { - return defaultIgnore.indexOf(pattern.split('/')[0]) >= 0; - }).map(function (pattern) { - return '!' + pattern; - }); - ignored = getDefaultIgnorePatterns().concat(ignored, overrideDefaultIgnorePatterns); - - if (paths.length === 0) { - paths = ['package.json', '**/*.js']; - } - paths = paths.concat(files); - - return { - paths: paths, - ignored: ignored - }; -} - -function makeSourceMatcher(sources) { - var mixedPatterns = []; - var defaultIgnorePatterns = getDefaultIgnorePatterns(); - var overrideDefaultIgnorePatterns = []; - - var hasPositivePattern = false; - sources.forEach(function (pattern) { - mixedPatterns.push(pattern); - if (!hasPositivePattern && pattern[0] !== '!') { - hasPositivePattern = true; - } - - // Extract patterns that start with an ignored directory. These need to be - // rematched separately. - if (defaultIgnore.indexOf(pattern.split('/')[0]) >= 0) { - overrideDefaultIgnorePatterns.push(pattern); - } - }); - - // Same defaults as used for Chokidar. - if (!hasPositivePattern) { - mixedPatterns = ['package.json', '**/*.js'].concat(mixedPatterns); - } - - return function (path) { - path = matchable(path); - - // Ignore paths outside the current working directory. They can't be matched - // to a pattern. - if (/^\.\.\//.test(path)) { - return false; - } - - var isSource = multimatch(path, mixedPatterns).length === 1; - if (!isSource) { - return false; - } - - var isIgnored = multimatch(path, defaultIgnorePatterns).length === 1; - if (!isIgnored) { - return true; - } - - var isErroneouslyIgnored = multimatch(path, overrideDefaultIgnorePatterns).length === 1; - if (isErroneouslyIgnored) { - return true; - } - - return false; - }; -} - -function makeTestMatcher(files, excludePatterns) { - var initialPatterns = files.concat(excludePatterns); - - return function (path) { - // Like in api.js, tests must be .js files and not start with _ - if (nodePath.extname(path) !== '.js' || nodePath.basename(path)[0] === '_') { - return false; - } - - // Check if the entire path matches a pattern. - if (multimatch(matchable(path), initialPatterns).length === 1) { - return true; - } - - // Check if the path contains any directory components. - var dirname = nodePath.dirname(path); - if (dirname === '.') { - return false; - } - - // Compute all possible subpaths. Note that the dirname is assumed to be - // relative to the working directory, without a leading `./`. - var subpaths = dirname.split(nodePath.sep).reduce(function (subpaths, component) { - var parent = subpaths[subpaths.length - 1]; - if (parent) { - // Always use / to makes multimatch consistent across platforms. - subpaths.push(parent + '/' + component); - } else { - subpaths.push(component); - } - return subpaths; - }, []); - - // Check if any of the possible subpaths match a pattern. If so, generate a - // new pattern with **/*.js. - var recursivePatterns = subpaths.filter(function (subpath) { - return multimatch(subpath, initialPatterns).length === 1; - }).map(function (subpath) { - // Always use / to makes multimatch consistent across platforms. - return subpath + '/**/*.js'; - }); - - // See if the entire path matches any of the subpaths patterns, taking the - // excludePatterns into account. This mimicks the behavior in api.js - return multimatch(matchable(path), recursivePatterns.concat(excludePatterns)).length === 1; - }; -} - function TestDependency(file, sources) { this.file = file; this.sources = sources; diff --git a/test/ava-files.js b/test/ava-files.js new file mode 100644 index 000000000..2519d8a78 --- /dev/null +++ b/test/ava-files.js @@ -0,0 +1,97 @@ +'use strict'; +var test = require('tap').test; +var AvaFiles = require('../lib/ava-files'); + +test('requires new', function (t) { + var avaFiles = AvaFiles; + t.throws(function () { + avaFiles(['**/foo*']); + }, 'Class constructor AvaFiles cannot be invoked without \'new\''); + t.end(); +}); + +test('testMatcher', function (t) { + var avaFiles = new AvaFiles(['**/foo*']); + + var matcher = avaFiles.makeTestMatcher(); + + function isTest(file) { + t.true(matcher(file), file + ' should be a test'); + } + + function notTest(file) { + t.false(matcher(file), file + ' should not be a test'); + } + + isTest('foo-bar.js'); + isTest('foo.js'); + isTest('foo/blah.js'); + isTest('bar/foo.js'); + isTest('bar/foo-bar/baz/buz.js'); + notTest('bar/baz/buz.js'); + notTest('bar.js'); + notTest('bar/bar.js'); + notTest('_foo-bar.js'); + notTest('foo/_foo-bar.js'); + notTest('foo-bar.txt'); + notTest('node_modules/foo.js'); + notTest('fixtures/foo.js'); + notTest('helpers/foo.js'); + t.end(); +}); + +test('sourceMatcher - defaults', function (t) { + var avaFiles = new AvaFiles(['**/foo*']); + + var matcher = avaFiles.makeSourceMatcher(); + + function isSource(file) { + t.true(matcher(file), file + ' should be a source'); + } + + function notSource(file) { + t.false(matcher(file), file + ' should not be a source'); + } + + isSource('foo-bar.js'); + isSource('foo.js'); + isSource('foo/blah.js'); + isSource('bar/foo.js'); + + isSource('_foo-bar.js'); + isSource('foo/_foo-bar.js'); + isSource('fixtures/foo.js'); + isSource('helpers/foo.js'); + + // TODO: Watcher should probably track any required file that matches the source pattern and has a require extension installed for the given extension. + notSource('foo-bar.json'); + notSource('foo-bar.coffee'); + + // These seem OK + isSource('bar.js'); + isSource('bar/bar.js'); + notSource('node_modules/foo.js'); + + t.end(); +}); + +test('sourceMatcher - allow matching specific node_modules directories', function (t) { + var avaFiles = new AvaFiles(['**/foo*'], ['node_modules/foo/**']); + + var matcher = avaFiles.makeSourceMatcher(); + + t.true(matcher('node_modules/foo/foo.js')); + t.false(matcher('node_modules/bar/foo.js')); + t.end(); +}); + +test('sourceMatcher - providing negation patterns', function (t) { + var avaFiles = new AvaFiles(['**/foo*'], ['!**/bar*']); + + var matcher = avaFiles.makeSourceMatcher(); + + t.false(matcher('node_modules/foo/foo.js')); + t.false(matcher('bar.js')); + t.false(matcher('foo/bar.js')); + t.end(); +}); diff --git a/test/watcher.js b/test/watcher.js index 34909be38..993fd2729 100644 --- a/test/watcher.js +++ b/test/watcher.js @@ -8,6 +8,7 @@ var lolex = require('lolex'); var proxyquire = require('proxyquire'); var sinon = require('sinon'); var test = require('tap').test; +var AvaFiles = require('../lib/ava-files'); var setImmediate = require('../lib/globals').setImmediate; @@ -51,88 +52,71 @@ test('chokidar is not installed', function (t) { }); group('chokidar is installed', function (beforeEach, test, group) { - var chokidar = { - watch: sinon.stub() - }; + var chokidar; + var debug; + var logger; + var api; + var avaFiles; + var Subject; + var runStatus; + var resetRunStatus; + var clock; + var chokidarEmitter; + var stdin; + var files; - var debug = sinon.spy(); + function proxyWatcher(opts) { + return proxyquire.noCallThru().load('../lib/watcher', opts || + { + 'chokidar': chokidar, + 'debug': function (name) { + return function () { + var args = [name]; + args.push.apply(args, arguments); + debug.apply(null, args); + }; + }, + './ava-files': avaFiles + }); + } - var logger = { - start: sinon.spy(), - finish: sinon.spy(), - section: sinon.spy(), - clear: sinon.stub().returns(true), - reset: sinon.spy() - }; + beforeEach(function () { + chokidar = { + watch: sinon.stub() + }; - var api = { - on: function () {}, - run: sinon.stub() - }; + debug = sinon.spy(); - var avaFiles = sinon.stub(); - avaFiles.defaultExcludePatterns = sinon.stub(); - avaFiles.defaultIncludePatterns = sinon.stub(); + logger = { + start: sinon.spy(), + finish: sinon.spy(), + section: sinon.spy(), + clear: sinon.stub().returns(true), + reset: sinon.spy() + }; - var Subject = proxyquire.noCallThru().load('../lib/watcher', { - 'chokidar': chokidar, - 'debug': function (name) { - return function () { - var args = [name]; - args.push.apply(args, arguments); - debug.apply(null, args); - }; - }, - './ava-files': avaFiles - }); + api = { + on: function () {}, + run: sinon.stub() + }; - var runStatus; - var resetRunStatus = function () { - runStatus = {failCount: 0, rejectionCount: 0, exceptionCount: 0}; - return runStatus; - }; + resetRunStatus = function () { + runStatus = {failCount: 0, rejectionCount: 0, exceptionCount: 0}; + return runStatus; + }; - var clock; - var chokidarEmitter; - var stdin; - var files; - beforeEach(function () { if (clock) { clock.uninstall(); } clock = lolex.install(0, ['setImmediate', 'setTimeout', 'clearTimeout']); chokidarEmitter = new EventEmitter(); - chokidar.watch.reset(); chokidar.watch.returns(chokidarEmitter); - debug.reset(); - - logger.start.reset(); - logger.finish.reset(); - logger.section.reset(); - logger.reset.reset(); - - logger.clear.reset(); logger.clear.returns(true); - avaFiles.reset(); - avaFiles.defaultExcludePatterns.reset(); - avaFiles.defaultIncludePatterns.reset(); + avaFiles = AvaFiles; - avaFiles.defaultExcludePatterns.returns([ - '!**/node_modules/**', - '!**/fixtures/**', - '!**/helpers/**' - ]); - - avaFiles.defaultIncludePatterns.returns([ - 'test.js', - 'test-*.js', - 'test' - ]); - - api.run.reset(); api.run.returns(new Promise(function () {})); files = [ 'test.js', @@ -144,6 +128,8 @@ group('chokidar is installed', function (beforeEach, test, group) { stdin = new PassThrough(); stdin.pause(); + + Subject = proxyWatcher(); }); var start = function (sources) { @@ -567,9 +553,16 @@ group('chokidar is installed', function (beforeEach, test, group) { test('initial exclude patterns override whether something is a test file', function (t) { t.plan(2); + avaFiles = function (files, sources) { + var ret = new AvaFiles(files, sources); + // Note: There is no way for users to actually set exclude patterns yet. + // This test just validates that internal updates to the default excludes pattern will be obeyed. + ret.excludePatterns = ['!*bar*']; + return ret; + }; + Subject = proxyWatcher(); + files = ['foo-{bar,baz}.js']; - // TODO(@jamestalmage, @novemberborn): There is no way for users to actually set exclude patterns yet. - avaFiles.defaultExcludePatterns.returns(['!*bar*']); api.run.returns(Promise.resolve(runStatus)); start(); @@ -639,9 +632,17 @@ group('chokidar is installed', function (beforeEach, test, group) { test('exclude patterns override directory matches', function (t) { t.plan(2); + avaFiles = function (files, sources) { + var ret = new AvaFiles(files, sources); + // Note: There is no way for users to actually set exclude patterns yet. + // This test just validates that internal updates to the default excludes pattern will be obeyed. + ret.excludePatterns = ['!**/exclude/**']; + return ret; + }; + + Subject = proxyWatcher(); + files = ['dir']; - // TODO(@jamestalmage, @novemberborn): There is no way for users to actually set exclude patterns yet. - avaFiles.defaultExcludePatterns.returns(['!**/exclude/**']); api.run.returns(Promise.resolve(runStatus)); start();