Further helper selection improvements

* Add test to ensure matched test files with the wrong extension are not treated as test files

* Update files and sources configuration docs

* Combine isTest() and isSource()

isSource() calls isTest(), so it's more efficient to classify whether a file is either in a single call.

* Don't rerun tests when detecting changes to files that are not tests, helpers or sources

Since ignore patterns are no longer passed to Chokidar, if a file is not
a test, helper or source, then tests do not need to be re-run.

* Support helper glob configuration

Fixes #2105.

Author: novemberborn

docs/06-configuration.md

@@ -4,21 +4,23 @@ Translations: [Français](https://github.com/avajs/ava-docs/blob/master/fr_FR/do
 
 All of the [CLI options](./05-command-line.md) can be configured in the `ava` section of either your `package.json` file, or an `ava.config.js` file. This allows you to modify the default behavior of the `ava` command, so you don't have to repeatedly type the same options on the command prompt.
 
-To ignore a file or directory, prefix the pattern with an `!` (exclamation mark).
+To ignore files, prefix the pattern with an `!` (exclamation mark).
 
 **`package.json`:**
 
 ```json
 {
 	"ava": {
 		"files": [
-			"my-test-directory/**/*.js",
-			"!my-test-directory/exclude-this-directory",
-			"!**/exclude-this-file.js"
+			"test/**/*",
+			"!test/exclude-files-in-this-directory",
+			"!**/exclude-files-with-this-name.*"
+		],
+		"helpers": [
+			"**/helpers/**/*"
 		],
 		"sources": [
-			"**/*.{js,jsx}",
-			"!dist"
+			"src/**/*"
 		],
 		"match": [
 			"*oo",
@@ -35,7 +37,7 @@ To ignore a file or directory, prefix the pattern with an `!` (exclamation mark)
 			"@babel/register"
 		],
 		"babel": {
-			"extensions": ["jsx"],
+			"extensions": ["js", "jsx"],
 			"testOptions": {
 				"babelrc": false
 			}
@@ -48,8 +50,9 @@ Arguments passed to the CLI will always take precedence over the CLI options con
 
 ## Options
 
-- `files`: glob patterns that select which files AVA will run tests from. Files with an underscore prefix are ignored. By default only selects files with `js` extensions, even if the glob pattern matches other files. Specify `extensions` and `babel.extensions` to allow other file extensions
-- `sources`: files that, when changed, cause tests to be re-run in watch mode. See the [watch mode recipe for details](https://github.com/avajs/ava/blob/master/docs/recipes/watch-mode.md#source-files-and-test-files)
+- `files`: an array of glob patterns to select test files. Files with an underscore prefix are ignored. By default only selects files with `js` extensions, even if the pattern matches other files. Specify `extensions` and `babel.extensions` to allow other file extensions
+- `helpers`: an array of glob patterns to select helper files. Files matched here are never considered as tests. By default only selects files with `js` extensions, even if the pattern matches other files. Specify `extensions` and `babel.extensions` to allow other file extensions
+- `sources`: an array of glob patterns to match files that, when changed, cause tests to be re-run (when in watch mode). See the [watch mode recipe for details](https://github.com/avajs/ava/blob/master/docs/recipes/watch-mode.md#source-files-and-test-files)
 - `match`: not typically useful in the `package.json` configuration, but equivalent to [specifying `--match` on the CLI](./05-command-line.md#running-tests-with-matching-titles)
 - `cache`: cache compiled test and helper files under `node_modules/.cache/ava`. If `false`, files are cached in a temporary directory instead
 - `failFast`: stop running further tests once a test fails

lib/api.js

@@ -110,12 +110,17 @@ class Api extends Emittery {
 			const precompiler = await this._setupPrecompiler();
 			let helpers = [];
 			if (files.length === 0 || precompiler.enabled) {
-				const found = await globs.findHelpersAndTests({cwd: this.options.resolveTestsFrom, ...apiOptions.globs});
+				let found;
+				if (precompiler.enabled) {
+					found = await globs.findHelpersAndTests({cwd: this.options.resolveTestsFrom, ...apiOptions.globs});
+					helpers = found.helpers;
+				} else {
+					found = await globs.findTests({cwd: this.options.resolveTestsFrom, ...apiOptions.globs});
+				}
+
 				if (files.length === 0) {
 					({tests: files} = found);
 				}
-
-				({helpers} = found);
 			}
 
 			if (this.options.parallelRuns) {

lib/cli.js

@@ -180,7 +180,7 @@ exports.run = async () => { // eslint-disable-line complexity
 
 	let globs;
 	try {
-		globs = normalizeGlobs(conf.files, conf.sources, extensions.all);
+		globs = normalizeGlobs(conf.files, conf.helpers, conf.sources, extensions.all);
 	} catch (error) {
 		exit(error.message);
 	}

lib/globs.js

@@ -28,11 +28,15 @@ const normalizePatterns = patterns => {
 	});
 };
 
-function normalizeGlobs(testPatterns, sourcePatterns, extensions) {
+function normalizeGlobs(testPatterns, helperPatterns, sourcePatterns, extensions) {
 	if (typeof testPatterns !== 'undefined' && (!Array.isArray(testPatterns) || testPatterns.length === 0)) {
 		throw new Error('The \'files\' configuration must be an array containing glob patterns.');
 	}
 
+	if (typeof helperPatterns !== 'undefined' && (!Array.isArray(helperPatterns) || helperPatterns.length === 0)) {
+		throw new Error('The \'helpers\' configuration must be an array containing glob patterns.');
+	}
+
 	if (sourcePatterns && (!Array.isArray(sourcePatterns) || sourcePatterns.length === 0)) {
 		throw new Error('The \'sources\' configuration must be an array containing glob patterns.');
 	}
@@ -58,6 +62,12 @@ function normalizeGlobs(testPatterns, sourcePatterns, extensions) {
 		testPatterns = defaultTestPatterns;
 	}
 
+	if (helperPatterns) {
+		helperPatterns = normalizePatterns(helperPatterns);
+	} else {
+		helperPatterns = [];
+	}
+
 	const defaultSourcePatterns = [
 		'**/*.snap',
 		'ava.config.js',
@@ -75,11 +85,13 @@ function normalizeGlobs(testPatterns, sourcePatterns, extensions) {
 		sourcePatterns = defaultSourcePatterns;
 	}
 
-	return {extensions, testPatterns, sourcePatterns};
+	return {extensions, testPatterns, helperPatterns, sourcePatterns};
 }
 
 exports.normalizeGlobs = normalizeGlobs;
 
+const hasExtension = (extensions, file) => extensions.includes(path.extname(file).slice(1));
+
 const findFiles = async (cwd, patterns) => {
 	const files = await globby(patterns, {
 		absolute: true,
@@ -108,26 +120,60 @@ const findFiles = async (cwd, patterns) => {
 	return files;
 };
 
-async function findHelpersAndTests({cwd, extensions, testPatterns}) {
-	const helpers = [];
+async function findHelpersAndTests({cwd, extensions, testPatterns, helperPatterns}) {
+	// Search for tests concurrently with finding helpers.
+	const findingTests = findFiles(cwd, testPatterns);
+
+	const uniqueHelpers = new Set();
+	if (helperPatterns.length > 0) {
+		for (const file of await findFiles(cwd, helperPatterns)) {
+			if (!hasExtension(extensions, file)) {
+				continue;
+			}
+
+			uniqueHelpers.add(file);
+		}
+	}
+
 	const tests = [];
-	for (const file of await findFiles(cwd, testPatterns)) {
-		if (!extensions.includes(path.extname(file).slice(1))) {
+	for (const file of await findingTests) {
+		if (!hasExtension(extensions, file)) {
 			continue;
 		}
 
 		if (path.basename(file).startsWith('_')) {
-			helpers.push(file);
-		} else {
+			uniqueHelpers.add(file);
+		} else if (!uniqueHelpers.has(file)) { // Helpers cannot be tests.
 			tests.push(file);
 		}
 	}
 
-	return {helpers, tests};
+	return {helpers: [...uniqueHelpers], tests};
 }
 
 exports.findHelpersAndTests = findHelpersAndTests;
 
+async function findTests({cwd, extensions, testPatterns, helperPatterns}) {
+	const rejectHelpers = helperPatterns.length > 0;
+
+	const tests = [];
+	for (const file of await findFiles(cwd, testPatterns)) {
+		if (!hasExtension(extensions, file) || path.basename(file).startsWith('_')) {
+			continue;
+		}
+
+		if (rejectHelpers && matches(normalizeFileForMatching(cwd, file), helperPatterns)) {
+			continue;
+		}
+
+		tests.push(file);
+	}
+
+	return {tests};
+}
+
+exports.findTests = findTests;
+
 function getChokidarPatterns({sourcePatterns, testPatterns}) {
 	const paths = [];
 	const ignored = defaultIgnorePatterns.map(pattern => `${pattern}/**/*`);
@@ -175,14 +221,57 @@ const matches = (file, patterns) => {
 	return micromatch.some(file, patterns, {ignore});
 };
 
-function isSource(file, {testPatterns, sourcePatterns}) {
-	return !isTest(file, {testPatterns}) && matches(file, sourcePatterns);
-}
+const NOT_IGNORED = ['**/*'];
+
+const normalizeFileForMatching = (cwd, file) => {
+	if (process.platform === 'win32') {
+		cwd = slash(cwd);
+		file = slash(file);
+	}
+
+	if (!cwd) { // TODO: Ensure tests provide an actual value.
+		return file;
+	}
+
+	// TODO: If `file` is outside `cwd` we can't normalize it. Need to figure
+	// out if that's a real-world scenario, but we may have to ensure the file
+	// isn't even selected.
+	if (!file.startsWith(cwd)) {
+		return file;
+	}
 
-exports.isSource = isSource;
+	// Assume `cwd` does *not* end in a slash.
+	return file.slice(cwd.length + 1);
+};
+
+function classify(file, {cwd, extensions, helperPatterns, testPatterns, sourcePatterns}) {
+	let isHelper = false;
+	let isTest = false;
+	let isSource = false;
+
+	file = normalizeFileForMatching(cwd, file);
+
+	if (hasExtension(extensions, file)) {
+		if (path.basename(file).startsWith('_')) {
+			isHelper = matches(file, NOT_IGNORED);
+		} else {
+			isHelper = helperPatterns.length > 0 && matches(file, helperPatterns);
+
+			if (!isHelper) {
+				isTest = testPatterns.length > 0 && matches(file, testPatterns);
+
+				if (!isTest) {
+					// Note: Don't check sourcePatterns.length since we still need to
+					// check the file against the default ignore patterns.
+					isSource = matches(file, sourcePatterns);
+				}
+			}
+		}
+	} else {
+		isSource = matches(file, sourcePatterns);
+	}
 
-function isTest(file, {testPatterns}) {
-	return !path.basename(file).startsWith('_') && matches(file, testPatterns);
+	return {isHelper, isTest, isSource};
 }
 
-exports.isTest = isTest;
+exports.classify = classify;

lib/watcher.js

@@ -68,13 +68,13 @@ class Debouncer {
 }
 
 class TestDependency {
-	constructor(file, sources) {
+	constructor(file, dependencies) {
 		this.file = file;
-		this.sources = sources;
+		this.dependencies = dependencies;
 	}
 
-	contains(source) {
-		return this.sources.includes(source);
+	contains(dependency) {
+		return this.dependencies.includes(dependency);
 	}
 }
 
@@ -177,14 +177,17 @@ class Watcher {
 					return;
 				}
 
-				const sourceDeps = evt.dependencies.map(x => relative(x)).filter(filePath => globs.isSource(filePath, this.globs));
-				this.updateTestDependencies(evt.testFile, sourceDeps);
+				const dependencies = evt.dependencies.map(x => relative(x)).filter(filePath => {
+					const {isHelper, isSource} = globs.classify(filePath, this.globs);
+					return isHelper || isSource;
+				});
+				this.updateTestDependencies(evt.testFile, dependencies);
 			});
 		});
 	}
 
-	updateTestDependencies(file, sources) {
-		if (sources.length === 0) {
+	updateTestDependencies(file, dependencies) {
+		if (dependencies.length === 0) {
 			this.testDependencies = this.testDependencies.filter(dep => dep.file !== file);
 			return;
 		}
@@ -194,13 +197,13 @@ class Watcher {
 				return false;
 			}
 
-			dep.sources = sources;
+			dep.dependencies = dependencies;
 
 			return true;
 		});
 
 		if (!isUpdate) {
-			this.testDependencies.push(new TestDependency(file, sources));
+			this.testDependencies.push(new TestDependency(file, dependencies));
 		}
 	}
 
@@ -360,8 +363,19 @@ class Watcher {
 
 			return true;
 		});
-		const dirtyTests = dirtyPaths.filter(filePath => globs.isTest(filePath, this.globs));
-		const dirtySources = diff(dirtyPaths, dirtyTests);
+		const dirtyHelpersAndSources = [];
+		const dirtyTests = [];
+		for (const filePath of dirtyPaths) {
+			const {isHelper, isSource, isTest} = globs.classify(filePath, this.globs);
+			if (isHelper || isSource) {
+				dirtyHelpersAndSources.push(filePath);
+			}
+
+			if (isTest) {
+				dirtyTests.push(filePath);
+			}
+		}
+
 		const addedOrChangedTests = dirtyTests.filter(path => dirtyStates[path] !== 'unlink');
 		const unlinkedTests = diff(dirtyTests, addedOrChangedTests);
 
@@ -372,14 +386,14 @@ class Watcher {
 			return;
 		}
 
-		if (dirtySources.length === 0) {
+		if (dirtyHelpersAndSources.length === 0) {
 			// Run any new or changed tests
 			this.run(addedOrChangedTests);
 			return;
 		}
 
 		// Try to find tests that depend on the changed source files
-		const testsBySource = dirtySources.map(path => {
+		const testsByHelpersOrSource = dirtyHelpersAndSources.map(path => {
 			return this.testDependencies.filter(dep => dep.contains(path)).map(dep => {
 				debug('%s is a dependency of %s', path, dep.file);
 				return dep.file;
@@ -388,15 +402,15 @@ class Watcher {
 
 		// Rerun all tests if source files were changed that could not be traced to
 		// specific tests
-		if (testsBySource.length !== dirtySources.length) {
-			debug('Sources remain that cannot be traced to specific tests: %O', dirtySources);
+		if (testsByHelpersOrSource.length !== dirtyHelpersAndSources.length) {
+			debug('Helpers & sources remain that cannot be traced to specific tests: %O', dirtyHelpersAndSources);
 			debug('Rerunning all tests');
 			this.run();
 			return;
 		}
 
 		// Run all affected tests
-		this.run(union(addedOrChangedTests, uniq(flatten(testsBySource))));
+		this.run(union(addedOrChangedTests, uniq(flatten(testsByHelpersOrSource))));
 	}
 }