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
 		}
 	]
-}
+}