Fixes bug in diffing algorithm & improves fixture testing method.

Author: hediet

src/vs/editor/common/diff/algorithms/joinSequenceDiffs.ts

@@ -85,11 +85,11 @@ export function shiftSequenceDiffs(sequence1: ISequence, sequence2: ISequence, s
 		const diff = sequenceDiffs[i];
 		if (diff.seq1Range.isEmpty) {
 			const seq2PrevEndExclusive = (i > 0 ? sequenceDiffs[i - 1].seq2Range.endExclusive : -1);
-			const seq2NextStart = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1].seq2Range.start : sequence2.length + 1);
+			const seq2NextStart = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1].seq2Range.start : sequence2.length);
 			sequenceDiffs[i] = shiftDiffToBetterPosition(diff, sequence1, sequence2, seq2NextStart, seq2PrevEndExclusive);
 		} else if (diff.seq2Range.isEmpty) {
 			const seq1PrevEndExclusive = (i > 0 ? sequenceDiffs[i - 1].seq1Range.endExclusive : -1);
-			const seq1NextStart = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1].seq1Range.start : sequence1.length + 1);
+			const seq1NextStart = (i + 1 < sequenceDiffs.length ? sequenceDiffs[i + 1].seq1Range.start : sequence1.length);
 			sequenceDiffs[i] = shiftDiffToBetterPosition(diff.reverse(), sequence2, sequence1, seq1NextStart, seq1PrevEndExclusive).reverse();
 		}
 	}
@@ -102,15 +102,15 @@ function shiftDiffToBetterPosition(diff: SequenceDiff, sequence1: ISequence, seq
 
 	// don't touch previous or next!
 	let deltaBefore = 1;
-	while (diff.seq1Range.start - deltaBefore > seq2PrevEndExclusive &&
+	while (diff.seq2Range.start - deltaBefore > seq2PrevEndExclusive &&
 		sequence2.getElement(diff.seq2Range.start - deltaBefore) ===
 		sequence2.getElement(diff.seq2Range.endExclusive - deltaBefore) && deltaBefore < maxShiftLimit) {
 		deltaBefore++;
 	}
 	deltaBefore--;
 
 	let deltaAfter = 1;
-	while (diff.seq1Range.start + deltaAfter < seq2NextStart &&
+	while (diff.seq2Range.start + deltaAfter < seq2NextStart &&
 		sequence2.getElement(diff.seq2Range.start + deltaAfter) ===
 		sequence2.getElement(diff.seq2Range.endExclusive + deltaAfter) && deltaAfter < maxShiftLimit) {
 		deltaAfter++;

src/vs/editor/test/node/diffing/diffing.test.ts

@@ -5,20 +5,22 @@
 
 import assert = require('assert');
 import { readdirSync, readFileSync, existsSync, writeFileSync } from 'fs';
-import { join } from 'path';
+import { join, resolve } from 'path';
 import { FileAccess } from 'vs/base/common/network';
 import { SmartLinesDiffComputer } from 'vs/editor/common/diff/smartLinesDiffComputer';
 import { StandardLinesDiffComputer } from 'vs/editor/common/diff/standardLinesDiffComputer';
 
 suite('diff fixtures', () => {
-	const fixturesDir = FileAccess.asFileUri('vs/editor/test/node/diffing/fixtures').fsPath;
-	const folders = readdirSync(fixturesDir);
+	const fixturesOutDir = FileAccess.asFileUri('vs/editor/test/node/diffing/fixtures').fsPath;
+	// We want the dir in src, so we can directly update the source files if they disagree and create invalid files to capture the previous state.
+	// This makes it very easy to update the fixtures.
+	const fixturesSrcDir = resolve(fixturesOutDir).replaceAll('\\', '/').replace('/out/vs/editor/', '/src/vs/editor/');
+	const folders = readdirSync(fixturesSrcDir);
 
 	for (const folder of folders) {
 		for (const diffingAlgoName of ['smart', 'experimental']) {
-
 			test(`${folder}-${diffingAlgoName}`, () => {
-				const folderPath = join(fixturesDir, folder);
+				const folderPath = join(fixturesSrcDir, folder);
 				const files = readdirSync(folderPath);
 
 				const firstFileName = files.find(f => f.startsWith('1.'))!;
@@ -31,7 +33,7 @@ suite('diff fixtures', () => {
 
 				const diff = diffingAlgo.computeDiff(firstContentLines, secondContentLines, { ignoreTrimWhitespace: false, maxComputationTime: Number.MAX_SAFE_INTEGER });
 
-				const diffingResult: DiffingResult = {
+				const actualDiffingResult: DiffingResult = {
 					originalFileName: `./${firstFileName}`,
 					modifiedFileName: `./${secondFileName}`,
 					diffs: diff.changes.map<IDetailedDiff>(c => ({
@@ -44,29 +46,34 @@ suite('diff fixtures', () => {
 					}))
 				};
 
-				const actualFilePath = join(folderPath, `${diffingAlgoName}.actual.diff.json`);
 				const expectedFilePath = join(folderPath, `${diffingAlgoName}.expected.diff.json`);
+				const invalidFilePath = join(folderPath, `${diffingAlgoName}.invalid.diff.json`);
+
+				const expectedFileContentFromActual = JSON.stringify(actualDiffingResult, null, '\t');
 
-				const expectedFileContent = JSON.stringify(diffingResult, null, '\t');
+				const invalidExists = existsSync(invalidFilePath);
 
-				if (!existsSync(actualFilePath)) {
-					writeFileSync(actualFilePath, expectedFileContent);
-					writeFileSync(expectedFilePath, expectedFileContent);
-					throw new Error('No actual file! Actual and expected files were written.');
+				if (!existsSync(expectedFilePath)) {
+					writeFileSync(expectedFilePath, expectedFileContentFromActual);
+					writeFileSync(invalidFilePath, '');
+					throw new Error('No expected file! Expected and invalid files were written. Delete the invalid file to make the test pass.');
 				} else {
-					const actualFileContent = readFileSync(actualFilePath, 'utf8');
-					const actualFileDiffResult: DiffingResult = JSON.parse(actualFileContent);
+					const expectedFileContent = readFileSync(invalidExists ? invalidFilePath : expectedFilePath, 'utf8');
+					const expectedFileDiffResult: DiffingResult = JSON.parse(expectedFileContent);
 
 					try {
-						assert.deepStrictEqual(actualFileDiffResult, diffingResult);
+						assert.deepStrictEqual(actualDiffingResult, expectedFileDiffResult);
 					} catch (e) {
-						writeFileSync(expectedFilePath, expectedFileContent);
+						if (!invalidExists) {
+							writeFileSync(invalidFilePath, expectedFileContent);
+						}
+						writeFileSync(expectedFilePath, expectedFileContentFromActual);
 						throw e;
 					}
 				}
 
-				if (existsSync(expectedFilePath)) {
-					throw new Error('Expected file exists! Please delete it.');
+				if (invalidExists) {
+					throw new Error('Invalid file exists and agrees with expected file! Delete the invalid file to make the test pass.');
 				}
 			});
 		}

src/vs/editor/test/node/diffing/fixtures/bracket-aligning/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/bracket-aligning/experimental.expected.diff.json

@@ -61,8 +61,8 @@
 			]
 		},
 		{
-			"originalRange": "[86,95)",
-			"modifiedRange": "[88,98)",
+			"originalRange": "[86,98)",
+			"modifiedRange": "[88,101)",
 			"innerChanges": [
 				{
 					"originalRange": "[86,1 -> 86,1]",
@@ -102,9 +102,21 @@
 				},
 				{
 					"originalRange": "[95,1 -> 95,1]",
-					"modifiedRange": "[97,1 -> 98,1]"
+					"modifiedRange": "[97,1 -> 97,2]"
+				},
+				{
+					"originalRange": "[96,1 -> 96,1]",
+					"modifiedRange": "[98,1 -> 98,2]"
+				},
+				{
+					"originalRange": "[97,1 -> 97,1]",
+					"modifiedRange": "[99,1 -> 99,2]"
+				},
+				{
+					"originalRange": "[98,1 -> 98,1]",
+					"modifiedRange": "[100,1 -> 101,1]"
 				}
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/bracket-aligning/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/bracket-aligning/smart.expected.diff.json

@@ -13,4 +13,4 @@
 			"innerChanges": null
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/fuzzy-matching/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/fuzzy-matching/experimental.expected.diff.json

@@ -75,4 +75,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/fuzzy-matching/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/fuzzy-matching/smart.expected.diff.json

@@ -13,4 +13,4 @@
 			"innerChanges": null
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/indentation/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/indentation/experimental.expected.diff.json

@@ -0,0 +1,16 @@
+{
+	"originalFileName": "./1.json",
+	"modifiedFileName": "./2.json",
+	"diffs": [
+		{
+			"originalRange": "[301,301)",
+			"modifiedRange": "[301,305)",
+			"innerChanges": [
+				{
+					"originalRange": "[301,1 -> 301,1]",
+					"modifiedRange": "[301,1 -> 305,1]"
+				}
+			]
+		}
+	]
+}

src/vs/editor/test/node/diffing/fixtures/indentation/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/indentation/smart.expected.diff.json

@@ -215,8 +215,8 @@
 					"modifiedRange": "[7,130 -> 7,130]"
 				},
 				{
-					"originalRange": "[8,5 -> 8,10]",
-					"modifiedRange": "[7,130 -> 7,130]"
+					"originalRange": "[8,6 -> 8,11]",
+					"modifiedRange": "[7,131 -> 7,131]"
 				},
 				{
 					"originalRange": "[8,15 -> 8,21]",
@@ -251,8 +251,8 @@
 					"modifiedRange": "[7,181 -> 7,182]"
 				},
 				{
-					"originalRange": "[9,1 -> 10,1]",
-					"modifiedRange": "[8,1 -> 8,1]"
+					"originalRange": "[8,66 -> 9,3]",
+					"modifiedRange": "[7,183 -> 7,183]"
 				}
 			]
 		},
@@ -275,4 +275,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/intra-block-align/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/intra-block-align/experimental.expected.diff.json

@@ -75,4 +75,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/intra-block-align/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/intra-block-align/smart.expected.diff.json

@@ -27,4 +27,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/json-brackets/experimental.actual.diff.json

@@ -23,4 +23,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/json-brackets/experimental.expected.diff.json

@@ -23,4 +23,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/json-brackets/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/json-brackets/smart.expected.diff.json

@@ -18,4 +18,4 @@
 			"innerChanges": null
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/just-whitespace/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/just-whitespace/experimental.expected.diff.json

@@ -0,0 +1,16 @@
+{
+	"originalFileName": "./1.tst",
+	"modifiedFileName": "./2.tst",
+	"diffs": [
+		{
+			"originalRange": "[7,7)",
+			"modifiedRange": "[7,12)",
+			"innerChanges": [
+				{
+					"originalRange": "[7,1 -> 7,1]",
+					"modifiedRange": "[7,1 -> 12,1]"
+				}
+			]
+		}
+	]
+}

src/vs/editor/test/node/diffing/fixtures/just-whitespace/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/just-whitespace/smart.expected.diff.json

@@ -8,4 +8,4 @@
 			"innerChanges": null
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/method-splitting/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/method-splitting/experimental.expected.diff.json

@@ -177,8 +177,8 @@
 					"modifiedRange": "[16,81 -> 16,83]"
 				},
 				{
-					"originalRange": "[14,208 -> 14,208]",
-					"modifiedRange": "[16,84 -> 16,118]"
+					"originalRange": "[14,209 -> 14,209]",
+					"modifiedRange": "[16,85 -> 16,119]"
 				},
 				{
 					"originalRange": "[14,210 -> 14,211]",
@@ -251,8 +251,8 @@
 					"modifiedRange": "[23,25 -> 23,25]"
 				},
 				{
-					"originalRange": "[19,9 -> 19,13]",
-					"modifiedRange": "[23,27 -> 23,27]"
+					"originalRange": "[19,10 -> 19,14]",
+					"modifiedRange": "[23,28 -> 23,28]"
 				},
 				{
 					"originalRange": "[19,14 -> 19,17]",
@@ -303,8 +303,8 @@
 					"modifiedRange": "[23,79 -> 23,83]"
 				},
 				{
-					"originalRange": "[20,37 -> 20,40]",
-					"modifiedRange": "[23,85 -> 23,85]"
+					"originalRange": "[20,36 -> 20,39]",
+					"modifiedRange": "[23,84 -> 23,84]"
 				},
 				{
 					"originalRange": "[20,41 -> 20,44]",
@@ -391,8 +391,8 @@
 			"modifiedRange": "[26,38)",
 			"innerChanges": [
 				{
-					"originalRange": "[28,1 -> 28,1]",
-					"modifiedRange": "[26,1 -> 27,1]"
+					"originalRange": "[28,4 -> 28,4]",
+					"modifiedRange": "[26,4 -> 27,4]"
 				},
 				{
 					"originalRange": "[28,15 -> 28,20]",
@@ -505,4 +505,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/method-splitting/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/method-splitting/smart.expected.diff.json

@@ -152,4 +152,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/minimal-diff-character/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/minimal-diff-character/experimental.expected.diff.json

@@ -109,4 +109,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/minimal-diff-character/smart.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/minimal-diff-character/smart.expected.diff.json

@@ -45,4 +45,4 @@
 			]
 		}
 	]
-}
+}

src/vs/editor/test/node/diffing/fixtures/subword/experimental.actual.diff.json renamed to src/vs/editor/test/node/diffing/fixtures/subword/experimental.expected.diff.json

@@ -43,4 +43,4 @@
 			]
 		}
 	]
-}
+}