Remove support for snapshot IDs

ninevra · web-flow · commit d8b1e89e6e8b · 2021-02-13T16:24:48.000+01:00
diff --git a/docs/03-assertions.md b/docs/03-assertions.md
@@ -322,9 +322,10 @@ Assert that `contents` matches `regex`.
 Assert that `contents` does not match `regex`.
 
 ### `.snapshot(expected, message?)`
-### `.snapshot(expected, options?, message?)`
 
-Compares the `expected` value with a previously recorded snapshot. Snapshots are stored for each test, so ensure you give your tests unique titles. Alternatively pass an `options` object to select a specific snapshot, for instance `{id: 'my snapshot'}`.
+Compares the `expected` value with a previously recorded snapshot. Snapshots are stored for each test, so ensure you give your tests unique titles.
+
+AVA 3 supports an  `options` object that lets you select a specific snapshot, for instance `{id: 'my snapshot'}`. This is buggy and will be removed in AVA 4.
 
 Snapshot assertions cannot be skipped when snapshots are being updated.
 
diff --git a/index.d.ts b/index.d.ts
@@ -32,12 +32,6 @@ export type CommitDiscardOptions = {
 	retainLogs?: boolean;
 };
 
-/** Options that can be passed to the `t.snapshot()` assertion. */
-export type SnapshotOptions = {
-	/** If provided and not an empty string, used to select the snapshot to compare the `expected` value against. */
-	id?: string;
-};
-
 export interface Assertions {
 	/** Assert that `actual` is [truthy](https://developer.mozilla.org/en-US/docs/Glossary/Truthy). Comes with power-assert. */
 	assert: AssertAssertion;
@@ -241,18 +235,8 @@ export interface SnapshotAssertion {
 	 */
 	(expected: any, message?: string): void;
 
-	/**
-	 * Assert that `expected` is [deeply equal](https://github.com/concordancejs/concordance#comparison-details) to a
-	 * previously recorded [snapshot](https://github.com/concordancejs/concordance#serialization-details) (selected
-	 * through `options.id` if provided), or if necessary record a new snapshot.
-	 */
-	(expected: any, options: SnapshotOptions, message?: string): void;
-
 	/** Skip this assertion. */
 	skip(expected: any, message?: string): void;
-
-	/** Skip this assertion. */
-	skip(expected: any, options: SnapshotOptions, message?: string): void;
 }
 
 export interface ThrowsAssertion {
diff --git a/lib/assert.js b/lib/assert.js
@@ -698,7 +698,7 @@ class Assertions {
 			return handlePromise(retval, true);
 		});
 
-		this.snapshot = withSkip((expected, ...rest) => {
+		this.snapshot = withSkip((expected, message) => {
 			if (disableSnapshots) {
 				fail(new AssertionError({
 					assertion: 'snapshot',
@@ -708,30 +708,33 @@ class Assertions {
 				return;
 			}
 
-			let message;
-			let snapshotOptions;
-			if (rest.length > 1) {
-				[snapshotOptions, message] = rest;
-			} else {
-				const [optionsOrMessage] = rest;
-				if (typeof optionsOrMessage === 'object') {
-					snapshotOptions = optionsOrMessage;
-				} else {
-					message = optionsOrMessage;
-				}
+			if (message && message.id !== undefined) {
+				fail(new AssertionError({
+					assertion: 'snapshot',
+					message: 'AVA 4 no longer supports snapshot IDs',
+					improperUsage: true,
+					values: [formatWithLabel('Called with id:', message.id)]
+				}));
+				return;
 			}
 
 			if (!checkMessage('snapshot', message)) {
 				return;
 			}
 
+			if (message === '') {
+				fail(new AssertionError({
+					assertion: 'snapshot',
+					improperUsage: true,
+					message: 'The snapshot assertion message must be a non-empty string',
+					values: [formatWithLabel('Called with:', message)]
+				}));
+				return;
+			}
+
 			let result;
 			try {
-				result = compareWithSnapshot({
-					expected,
-					id: snapshotOptions ? snapshotOptions.id : undefined,
-					message
-				});
+				result = compareWithSnapshot({expected, message});
 			} catch (error) {
 				if (!(error instanceof snapshotManager.SnapshotError)) {
 					throw error;
diff --git a/lib/test.js b/lib/test.js
@@ -222,13 +222,12 @@ class Test {
 
 		const deferRecording = this.metadata.inline;
 		this.deferredSnapshotRecordings = [];
-		this.compareWithSnapshot = ({expected, id, message}) => {
+		this.compareWithSnapshot = ({expected, message}) => {
 			this.snapshotCount++;
 
-			// TODO: In a breaking change, reject non-undefined, falsy IDs and messages.
-			const belongsTo = id || snapshotBelongsTo;
-			const index = id ? 0 : this.nextSnapshotIndex++;
-			const label = id ? '' : message || `Snapshot ${index + 1}`; // Human-readable labels start counting at 1.
+			const belongsTo = snapshotBelongsTo;
+			const index = this.nextSnapshotIndex++;
+			const label = message || `Snapshot ${index + 1}`; // Human-readable labels start counting at 1.
 
 			const {taskIndex, associatedTaskIndex} = this.metadata;
 			const {record, ...result} = options.compareTestSnapshot({
diff --git a/test-d/snapshot.ts b/test-d/snapshot.ts
@@ -0,0 +1,14 @@
+import {expectError} from 'tsd';
+import test from '..';
+
+test('snapshot', t => {
+	t.snapshot({foo: 'bar'});
+	t.snapshot(null, 'a snapshot with a message');
+	expectError(t.snapshot('hello world', null));
+});
+
+test('snapshot.skip', t => {
+	t.snapshot.skip({foo: 'bar'});
+	t.snapshot.skip(null, 'a snapshot with a message');
+	expectError(t.snapshot.skip('hello world', null));
+});
diff --git a/test-tap/assert.js b/test-tap/assert.js
@@ -1650,10 +1650,10 @@ test('.snapshot()', t => {
 				super({
 					compareWithSnapshot: assertionOptions => {
 						const {record, ...result} = manager.compare({
-							belongsTo: assertionOptions.id || this.title,
+							belongsTo: this.title,
 							expected: assertionOptions.expected,
-							index: assertionOptions.id ? 0 : this.snapshotInvocationCount++,
-							label: assertionOptions.id ? '' : assertionOptions.message || `Snapshot ${this.snapshotInvocationCount}`
+							index: this.snapshotInvocationCount++,
+							label: assertionOptions.message || `Snapshot ${this.snapshotInvocationCount}`
 						});
 						if (record) {
 							record();
@@ -1679,10 +1679,6 @@ test('.snapshot()', t => {
 			const {snapshot} = assertions;
 			snapshot({foo: 'bar'});
 		});
-
-		passes(t, () => {
-			assertions.snapshot({foo: 'bar'}, {id: 'fixed id'}, 'message not included in snapshot report');
-		});
 	}
 
 	{
@@ -1700,15 +1696,6 @@ test('.snapshot()', t => {
 		}
 	}
 
-	failsWith(t, () => {
-		const assertions = setup('fails (fixed id)');
-		assertions.snapshot({foo: 'not bar'}, {id: 'fixed id'}, 'different message, also not included in snapshot report');
-	}, {
-		assertion: 'snapshot',
-		message: 'different message, also not included in snapshot report',
-		values: [{label: 'Difference:', formatted: '  {\n-   foo: \'not bar\',\n+   foo: \'bar\',\n  }'}]
-	});
-
 	{
 		const assertions = setup('fails');
 		if (updating) {
@@ -1727,7 +1714,7 @@ test('.snapshot()', t => {
 	{
 		const assertions = setup('bad message');
 		failsWith(t, () => {
-			assertions.snapshot(null, null, null);
+			assertions.snapshot(null, null);
 		}, {
 			assertion: 'snapshot',
 			improperUsage: true,
@@ -1737,6 +1724,34 @@ test('.snapshot()', t => {
 				formatted: /null/
 			}]
 		});
+
+		failsWith(t, () => {
+			assertions.snapshot(null, '');
+		}, {
+			assertion: 'snapshot',
+			improperUsage: true,
+			message: 'The snapshot assertion message must be a non-empty string',
+			values: [{
+				label: 'Called with:',
+				formatted: '\'\''
+			}]
+		});
+	}
+
+	{
+		// See https://github.com/avajs/ava/issues/2669
+		const assertions = setup('id');
+		failsWith(t, () => {
+			assertions.snapshot({foo: 'bar'}, {id: 'an id'});
+		}, {
+			assertion: 'snapshot',
+			improperUsage: true,
+			message: 'AVA 4 no longer supports snapshot IDs',
+			values: [{
+				label: 'Called with id:',
+				formatted: '\'an id\''
+			}]
+		});
 	}
 
 	manager.save();
diff --git a/test/snapshot-order/fixtures/randomness/test.js b/test/snapshot-order/fixtures/randomness/test.js
@@ -10,7 +10,6 @@ test('B - declare some snapshots', async t => {
 	t.snapshot(id(0));
 	t.snapshot(id(1), 'has a message');
 	t.snapshot(id(2), 'also has a message');
-	t.snapshot(id(3), {id: 'has an ID'});
 });
 
 test('A - declare some more snapshots', async t => {
@@ -36,12 +35,8 @@ test('E - discard some snapshots in a try()', async t => {
 	t.snapshot(id(10), 'outer again');
 });
 
-test('D - more snapshots with IDs', async t => {
+test('D - more snapshots', async t => {
 	await randomDelay();
-	t.snapshot(id(11), {id: 'the first in test D'});
 	t.snapshot(id(12));
-	// These have to be reported in reverse declaration order, because they can't
-	// be reported under the same header
-	t.snapshot(id(14), {id: 'the second-to-last in test D'});
 	t.snapshot(id(13));
 });
diff --git a/test/snapshot-order/fixtures/randomness/test.js.md b/test/snapshot-order/fixtures/randomness/test.js.md
@@ -18,10 +18,6 @@ Generated by [AVA](https://avajs.dev).
 
     'index: 2'
 
-## has an ID
-
-    'index: 3'
-
 ## A - declare some more snapshots
 
 > Snapshot 1
@@ -52,11 +48,7 @@ Generated by [AVA](https://avajs.dev).
 
     'index: 10'
 
-## the first in test D
-
-    'index: 11'
-
-## D - more snapshots with IDs
+## D - more snapshots
 
 > Snapshot 1
 
@@ -65,7 +57,3 @@ Generated by [AVA](https://avajs.dev).
 > Snapshot 2
 
     'index: 13'
-
-## the second-to-last in test D
-
-    'index: 14'
diff --git a/test/snapshot-order/fixtures/randomness/test.js.snap b/test/snapshot-order/fixtures/randomness/test.js.snap
diff --git a/test/snapshot-order/fixtures/report-declaration-order/test.js b/test/snapshot-order/fixtures/report-declaration-order/test.js
@@ -6,7 +6,6 @@ test('B - declare some snapshots', t => {
 	t.snapshot(id(0));
 	t.snapshot(id(1), 'has a message');
 	t.snapshot(id(2), 'also has a message');
-	t.snapshot(id(3), {id: 'has an ID'});
 });
 
 test('A - declare some more snapshots', t => {
@@ -30,10 +29,6 @@ test('E - discard some snapshots in a try()', async t => {
 });
 
 test('D - more snapshots with IDs', t => {
-	t.snapshot(id(11), {id: 'the first in test D'});
 	t.snapshot(id(12));
-	// These have to be reported in reverse declaration order, because they can't
-	// be reported under the same header
-	t.snapshot(id(14), {id: 'the second-to-last in test D'});
 	t.snapshot(id(13));
 });
diff --git a/test/snapshot-order/snapshots/randomness.js.md b/test/snapshot-order/snapshots/randomness.js.md
@@ -9,14 +9,12 @@ Generated by [AVA](https://avajs.dev).
 > resulting snapshot in binary encoding
 
     Buffer @Uint8Array [
-      41564120 536e6170 73686f74 2076320a 02008017 6c36567a 01a19351 43f9bd06
-      d5301f8b 08000000 00000003 63636460 e06090fc 96d9226e 3e3150e0 20d3b5ec
-      8deb7702 0581401c 8863433f 89d6966f 7976ffe9 b56ded65 1a898c60 515d208e
-      5f7b77a5 e07b763f b9ee6b93 72cfb0fe 63068b3a 43712414 e70371e9 bcb5a916
-      dc227cea 5c3fa552 5852af32 82455b81 b8e980f6 f3c7a28b 4527ee67 8e3cf37e
-      e62e46b0 e81c209e bcffafc6 d457fce1 e5e13782 366d5c15 c80416dd 0cc5a780
-      f8922bf3 cddd07b7 a6b64e5c 51b5b3b7 4897092c fa008abf 03f11781 f9ea3e07
-      e7db3ff3 792cbef3 817e0733 58949711 8295a1d8 12889919 d8c13e66 64651064
-      e4cccc4b 49adb052 30344111 e6800a63 1735c52a 6a8655d4 1caba831 76471862
-      1736c22e 6c8cd568 0bec8a0d b02ac62e 6a8855d4 08002fd4 dcd73f02 0000
+      41564120 536e6170 73686f74 2076320a 020089ef 307bf8df 7d1d9bef fcd38d56
+      76211f8b 08000000 00000003 dbc1c0c0 c0ca101b fa49b4b6 7ccbb3fb 4faf6d6b
+      2fd34864 64000131 208e5f7b 77a5e07b 763fb9ee 6b9372cf b0fe6306 8bea40b1
+      13144700 f1f94533 4f1d923e 71cf844d 86f7e85e 934626b0 683e14b7 01f12557
+      e69bbb0f 6e4d6d9d b8a26a67 6f912e13 58740e14 6f06e22f 02f3d57d 0eceb77f
+      e6f3587c e703fd0e 66b0e849 28be0fc5 5f819899 811dec46 46560641 468eccbc
+      94d40a2b 0513aca2 a65845cd b08a9aa3 88724245 0d8db00b 1b6335c3 02bb6203
+      ac8ab18b 1a621535 02004e86 b593ad01 0000
     ]
diff --git a/test/snapshot-order/snapshots/randomness.js.snap b/test/snapshot-order/snapshots/randomness.js.snap
