Remove snapshot files when a test file stops using snapshots

Fixes #1424.

Co-authored-by: Mark Wubben <mark@novemberborn.net>

Author: ninevra

lib/runner.js

@@ -29,6 +29,8 @@ class Runner extends Emittery {
 
 		this.activeRunnables = new Set();
 		this.boundCompareTestSnapshot = this.compareTestSnapshot.bind(this);
+		this.skippedSnapshots = false;
+		this.boundSkipSnapshot = this.skipSnapshot.bind(this);
 		this.interrupted = false;
 		this.snapshots = null;
 		this.nextTaskIndex = 0;
@@ -199,8 +201,19 @@ class Runner extends Emittery {
 		return this.snapshots.compare(options);
 	}
 
+	skipSnapshot() {
+		this.skippedSnapshots = true;
+	}
+
 	saveSnapshotState() {
-		if (this.updateSnapshots && (this.runOnlyExclusive || this.skippingTests)) {
+		if (
+			this.updateSnapshots &&
+			(
+				this.runOnlyExclusive ||
+				this.skippingTests ||
+				this.skippedSnapshots
+			)
+		) {
 			return {cannotSave: true};
 		}
 
@@ -209,9 +222,11 @@ class Runner extends Emittery {
 		}
 
 		if (this.updateSnapshots) {
-			// TODO: There may be unused snapshot files if no test caused the
-			// snapshots to be loaded. Prune them. But not if tests (including hooks!)
-			// were skipped. Perhaps emit a warning if this occurs?
+			return {touchedFiles: snapshotManager.cleanSnapshots({
+				file: this.file,
+				fixedLocation: this.snapshotDir,
+				projectDir: this.projectDir
+			})};
 		}
 
 		return {};
@@ -297,6 +312,7 @@ class Runner extends Emittery {
 				task.implementation :
 				t => task.implementation.apply(null, [t].concat(task.args)),
 			compareTestSnapshot: this.boundCompareTestSnapshot,
+			skipSnapshot: this.boundSkipSnapshot,
 			updateSnapshots: this.updateSnapshots,
 			metadata: {...task.metadata, associatedTaskIndex},
 			powerAssert: this.powerAssert,
@@ -349,6 +365,7 @@ class Runner extends Emittery {
 					task.implementation :
 					t => task.implementation.apply(null, [t].concat(task.args)),
 				compareTestSnapshot: this.boundCompareTestSnapshot,
+				skipSnapshot: this.boundSkipSnapshot,
 				updateSnapshots: this.updateSnapshots,
 				metadata: task.metadata,
 				powerAssert: this.powerAssert,

lib/snapshot-manager.js

@@ -449,12 +449,49 @@ const determineSnapshotDir = mem(({file, fixedLocation, projectDir}) => {
 
 exports.determineSnapshotDir = determineSnapshotDir;
 
-function load({file, fixedLocation, projectDir, recordNewSnapshots, updating}) {
+function determineSnapshotPaths({file, fixedLocation, projectDir}) {
 	const dir = determineSnapshotDir({file, fixedLocation, projectDir});
 	const relFile = path.relative(projectDir, resolveSourceFile(file));
 	const name = path.basename(relFile);
 	const reportFile = `${name}.md`;
 	const snapFile = `${name}.snap`;
+
+	return {
+		dir,
+		relFile,
+		snapFile,
+		reportFile
+	};
+}
+
+function cleanFile(file) {
+	try {
+		fs.unlinkSync(file);
+		return [file];
+	} catch (error) {
+		if (error.code === 'ENOENT') {
+			return [];
+		}
+
+		throw error;
+	}
+}
+
+// Remove snapshot and report if they exist. Returns an array containing the
+// paths of the touched files.
+function cleanSnapshots({file, fixedLocation, projectDir}) {
+	const {dir, snapFile, reportFile} = determineSnapshotPaths({file, fixedLocation, projectDir});
+
+	return [
+		...cleanFile(path.join(dir, snapFile)),
+		...cleanFile(path.join(dir, reportFile))
+	];
+}
+
+exports.cleanSnapshots = cleanSnapshots;
+
+function load({file, fixedLocation, projectDir, recordNewSnapshots, updating}) {
+	const {dir, relFile, snapFile, reportFile} = determineSnapshotPaths({file, fixedLocation, projectDir});
 	const snapPath = path.join(dir, snapFile);
 
 	let appendOnly = !updating;

lib/test.js

@@ -249,6 +249,10 @@ class Test {
 		};
 
 		this.skipSnapshot = () => {
+			if (typeof options.skipSnapshot === 'function') {
+				options.skipSnapshot();
+			}
+
 			if (options.updateSnapshots) {
 				this.addFailedAssertion(new Error('Snapshot assertions cannot be skipped when updating snapshots'));
 			} else {

package.json

@@ -123,6 +123,7 @@
 		"delay": "^4.4.0",
 		"esm": "^3.2.25",
 		"execa": "^5.0.0",
+		"fs-extra": "^9.0.1",
 		"get-stream": "^6.0.0",
 		"it-first": "^1.0.4",
 		"proxyquire": "^2.1.3",

test-tap/fixture/snapshots/watcher-rerun-unlink/package.json

@@ -0,0 +1 @@
+{}

test-tap/fixture/snapshots/watcher-rerun-unlink/test.js

@@ -0,0 +1,21 @@
+const test = require('../../../..');
+
+if (process.env.TEMPLATE) {
+	test('test title', t => {
+		t.snapshot({foo: 'bar'});
+		t.snapshot({answer: 42});
+		t.pass();
+	});
+
+	test('another test', t => {
+		t.snapshot(new Map());
+	});
+} else {
+	test('test title', t => {
+		t.pass();
+	});
+
+	test('another test', t => {
+		t.pass();
+	});
+}

test-tap/integration/watcher.js

@@ -82,6 +82,52 @@ test('watcher does not rerun test files when they write snapshot files', t => {
 	});
 });
 
+test('watcher does not rerun test files when they unlink snapshot files', t => {
+	// Run fixture as template to generate snapshots
+	execCli(
+		['--update-snapshots'],
+		{
+			dirname: 'fixture/snapshots/watcher-rerun-unlink',
+			env: {AVA_FORCE_CI: 'not-ci', TEMPLATE: 'true'}
+		},
+		err => {
+			t.ifError(err);
+
+			// Run fixture in watch mode; snapshots should be removed, and watcher should not rerun
+			let killed = false;
+
+			const child = execCli(
+				['--verbose', '--watch', '--update-snapshots', 'test.js'],
+				{
+					dirname: 'fixture/snapshots/watcher-rerun-unlink',
+					env: {AVA_FORCE_CI: 'not-ci'}
+				},
+				err => {
+					t.ok(killed);
+					t.ifError(err);
+					t.end();
+				}
+			);
+
+			let buffer = '';
+			let passedFirst = false;
+			child.stdout.on('data', string => {
+				buffer += string;
+				if (buffer.includes('2 tests passed') && !passedFirst) {
+					buffer = '';
+					passedFirst = true;
+					setTimeout(() => {
+						child.kill();
+						killed = true;
+					}, 500);
+				} else if (passedFirst && !killed) {
+					t.is(buffer.replace(/\s/g, '').replace(END_MESSAGE.replace(/\s/g, ''), ''), '');
+				}
+			});
+		}
+	);
+});
+
 test('watcher does not rerun test files when ignored files change', t => {
 	let killed = false;
 

test-tap/test.js

@@ -696,6 +696,36 @@ test('snapshot assertion can be skipped', t => {
 	});
 });
 
+// Snapshots reused from test/assert.js
+test('snapshot assertions call options.skipSnapshot when skipped', async t => {
+	const projectDir = path.join(__dirname, 'fixture');
+	const manager = snapshotManager.load({
+		file: path.join(projectDir, 'assert.js'),
+		projectDir,
+		fixedLocation: null,
+		updating: false
+	});
+
+	const skipSnapshot = sinon.spy();
+
+	const test = new Test({
+		compareTestSnapshot: options => manager.compare(options),
+		skipSnapshot,
+		updateSnapshots: false,
+		metadata: {},
+		title: 'passes',
+		fn(t) {
+			t.snapshot.skip({not: {a: 'match'}});
+			t.snapshot.skip({not: {b: 'match'}});
+			t.snapshot(React.createElement(HelloMessage, {name: 'Sindre'}));
+		}
+	});
+
+	await test.run();
+
+	t.true(skipSnapshot.calledTwice);
+});
+
 test('snapshot assertion cannot be skipped when updating snapshots', t => {
 	return new Test({
 		updateSnapshots: true,

test/snapshot-removal/fixtures/fixed-snapshot-dir/package.json

@@ -0,0 +1,5 @@
+{
+  "ava": {
+    "snapshotDir": "fixedSnapshotDir"
+  }
+}

test/snapshot-removal/fixtures/fixed-snapshot-dir/test.js

@@ -0,0 +1,22 @@
+const test = require(process.env.AVA_PATH); // This fixture is copied to a temporary directory, so require AVA through its configured path.
+
+if (process.env.TEMPLATE) {
+	test('some snapshots', t => {
+		t.snapshot('foo');
+		t.snapshot('bar');
+		t.pass();
+	});
+
+	test('another snapshot', t => {
+		t.snapshot('baz');
+		t.pass();
+	});
+} else {
+	test('some snapshots', t => {
+		t.pass();
+	});
+
+	test('another snapshot', t => {
+		t.pass();
+	});
+}