Skip to content

Commit 0646122

Browse files
hediethediet
authored
Fixes diffing assertion failures (microsoft#166855)
* Fixes diffing assertion failures * Improves diffing fixture test
1 parent 442ebb8 commit 0646122

File tree

9 files changed

+184
-145
lines changed

9 files changed

+184
-145
lines changed

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

Lines changed: 68 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import * as assert from 'assert';
7-
import { readdirSync, readFileSync, existsSync, writeFileSync } from 'fs';
7+
import { readdirSync, readFileSync, existsSync, writeFileSync, rmSync } from 'fs';
88
import { join, resolve } from 'path';
99
import { FileAccess } from 'vs/base/common/network';
1010
import { SmartLinesDiffComputer } from 'vs/editor/common/diff/smartLinesDiffComputer';
@@ -17,67 +17,78 @@ suite('diff fixtures', () => {
1717
const fixturesSrcDir = resolve(fixturesOutDir).replaceAll('\\', '/').replace('/out/vs/editor/', '/src/vs/editor/');
1818
const folders = readdirSync(fixturesSrcDir);
1919

20-
for (const folder of folders) {
21-
for (const diffingAlgoName of ['smart', 'experimental']) {
22-
test(`${folder}-${diffingAlgoName}`, () => {
23-
const folderPath = join(fixturesSrcDir, folder);
24-
const files = readdirSync(folderPath);
25-
26-
const firstFileName = files.find(f => f.startsWith('1.'))!;
27-
const secondFileName = files.find(f => f.startsWith('2.'))!;
28-
29-
const firstContentLines = readFileSync(join(folderPath, firstFileName), 'utf8').split(/\r\n|\r|\n/);
30-
const secondContentLines = readFileSync(join(folderPath, secondFileName), 'utf8').split(/\r\n|\r|\n/);
31-
32-
const diffingAlgo = diffingAlgoName === 'smart' ? new SmartLinesDiffComputer() : new StandardLinesDiffComputer();
33-
34-
const diff = diffingAlgo.computeDiff(firstContentLines, secondContentLines, { ignoreTrimWhitespace: false, maxComputationTimeMs: Number.MAX_SAFE_INTEGER });
35-
36-
const actualDiffingResult: DiffingResult = {
37-
originalFileName: `./${firstFileName}`,
38-
modifiedFileName: `./${secondFileName}`,
39-
diffs: diff.changes.map<IDetailedDiff>(c => ({
40-
originalRange: c.originalRange.toString(),
41-
modifiedRange: c.modifiedRange.toString(),
42-
innerChanges: c.innerChanges?.map<IDiff>(c => ({
43-
originalRange: c.originalRange.toString(),
44-
modifiedRange: c.modifiedRange.toString(),
45-
})) || null
46-
}))
47-
};
48-
49-
const expectedFilePath = join(folderPath, `${diffingAlgoName}.expected.diff.json`);
50-
const invalidFilePath = join(folderPath, `${diffingAlgoName}.invalid.diff.json`);
51-
52-
const expectedFileContentFromActual = JSON.stringify(actualDiffingResult, null, '\t');
53-
54-
const invalidExists = existsSync(invalidFilePath);
55-
56-
if (!existsSync(expectedFilePath)) {
57-
writeFileSync(expectedFilePath, expectedFileContentFromActual);
58-
writeFileSync(invalidFilePath, '');
59-
throw new Error('No expected file! Expected and invalid files were written. Delete the invalid file to make the test pass.');
60-
} else {
61-
const expectedFileContent = readFileSync(invalidExists ? invalidFilePath : expectedFilePath, 'utf8');
62-
const expectedFileDiffResult: DiffingResult = JSON.parse(expectedFileContent);
63-
64-
try {
65-
assert.deepStrictEqual(actualDiffingResult, expectedFileDiffResult);
66-
} catch (e) {
67-
if (!invalidExists) {
68-
writeFileSync(invalidFilePath, expectedFileContent);
69-
}
70-
writeFileSync(expectedFilePath, expectedFileContentFromActual);
71-
throw e;
72-
}
20+
function runTest(folder: string, diffingAlgoName: 'smart' | 'experimental') {
21+
const folderPath = join(fixturesSrcDir, folder);
22+
const files = readdirSync(folderPath);
23+
24+
const firstFileName = files.find(f => f.startsWith('1.'))!;
25+
const secondFileName = files.find(f => f.startsWith('2.'))!;
26+
27+
const firstContentLines = readFileSync(join(folderPath, firstFileName), 'utf8').split(/\r\n|\r|\n/);
28+
const secondContentLines = readFileSync(join(folderPath, secondFileName), 'utf8').split(/\r\n|\r|\n/);
29+
30+
const diffingAlgo = diffingAlgoName === 'smart' ? new SmartLinesDiffComputer() : new StandardLinesDiffComputer();
31+
32+
const diff = diffingAlgo.computeDiff(firstContentLines, secondContentLines, { ignoreTrimWhitespace: false, maxComputationTimeMs: Number.MAX_SAFE_INTEGER });
33+
34+
const actualDiffingResult: DiffingResult = {
35+
originalFileName: `./${firstFileName}`,
36+
modifiedFileName: `./${secondFileName}`,
37+
diffs: diff.changes.map<IDetailedDiff>(c => ({
38+
originalRange: c.originalRange.toString(),
39+
modifiedRange: c.modifiedRange.toString(),
40+
innerChanges: c.innerChanges?.map<IDiff>(c => ({
41+
originalRange: c.originalRange.toString(),
42+
modifiedRange: c.modifiedRange.toString(),
43+
})) || null
44+
}))
45+
};
46+
47+
const expectedFilePath = join(folderPath, `${diffingAlgoName}.expected.diff.json`);
48+
const invalidFilePath = join(folderPath, `${diffingAlgoName}.invalid.diff.json`);
49+
50+
const expectedFileContentFromActual = JSON.stringify(actualDiffingResult, null, '\t');
51+
52+
const invalidExists = existsSync(invalidFilePath);
53+
54+
if (!existsSync(expectedFilePath)) {
55+
writeFileSync(expectedFilePath, expectedFileContentFromActual);
56+
writeFileSync(invalidFilePath, '');
57+
throw new Error('No expected file! Expected and invalid files were written. Delete the invalid file to make the test pass.');
58+
} else {
59+
const expectedFileContent = readFileSync(invalidExists ? invalidFilePath : expectedFilePath, 'utf8');
60+
if (invalidExists && expectedFileContent === '') {
61+
throw new Error('Delete the invalid file to make the test pass.');
62+
}
63+
const expectedFileDiffResult: DiffingResult = JSON.parse(expectedFileContent);
64+
65+
try {
66+
assert.deepStrictEqual(actualDiffingResult, expectedFileDiffResult);
67+
} catch (e) {
68+
if (!invalidExists) {
69+
writeFileSync(invalidFilePath, expectedFileContent);
7370
}
71+
writeFileSync(expectedFilePath, expectedFileContentFromActual);
72+
throw e;
73+
}
74+
}
7475

75-
if (invalidExists) {
76-
throw new Error('Invalid file exists and agrees with expected file! Delete the invalid file to make the test pass.');
77-
}
76+
if (invalidExists) {
77+
rmSync(invalidFilePath);
78+
}
79+
}
80+
81+
for (const folder of folders) {
82+
for (const diffingAlgoName of ['smart', 'experimental'] as const) {
83+
test(`${folder}-${diffingAlgoName}`, () => {
84+
runTest(folder, diffingAlgoName);
7885
});
7986
}
8087
}
88+
89+
test(`debug`, () => {
90+
runTest('penalize-fragmentation', 'experimental');
91+
});
8192
});
8293

8394
interface DiffingResult {

‎src/vs/editor/test/node/diffing/fixtures/penalize-fragmentation/1.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
import { IChange, IDiffComputationResult } from 'vs/editor/common/diff/diffComputer';

‎src/vs/editor/test/node/diffing/fixtures/penalize-fragmentation/2.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
import { IDocumentDiffProviderOptions } from 'vs/editor/common/diff/documentDiffProvider';
2+
import { IChange } from 'vs/editor/common/diff/smartLinesDiffComputer';

‎src/vs/editor/test/node/diffing/fixtures/penalize-fragmentation/experimental.expected.diff.json

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
{
2+
"originalFileName": "./1.txt",
3+
"modifiedFileName": "./2.txt",
4+
"diffs": [
5+
{
6+
"originalRange": "[1,2)",
7+
"modifiedRange": "[1,3)",
8+
"innerChanges": [
9+
{
10+
"originalRange": "[1,11 -> 1,14]",
11+
"modifiedRange": "[1,11 -> 1,17]"
12+
},
13+
{
14+
"originalRange": "[1,15 -> 1,20]",
15+
"modifiedRange": "[1,18 -> 1,19]"
16+
},
17+
{
18+
"originalRange": "[1,24 -> 1,25]",
19+
"modifiedRange": "[1,23 -> 1,25]"
20+
},
21+
{
22+
"originalRange": "[1,26 -> 1,27]",
23+
"modifiedRange": "[1,26 -> 1,32]"
24+
},
25+
{
26+
"originalRange": "[1,28 -> 1,31]",
27+
"modifiedRange": "[1,33 -> 1,33]"
28+
},
29+
{
30+
"originalRange": "[1,35 -> 1,36]",
31+
"modifiedRange": "[1,37 -> 1,50]"
32+
},
33+
{
34+
"originalRange": "[1,37 -> 1,38]",
35+
"modifiedRange": "[1,51 -> 1,72]"
36+
},
37+
{
38+
"originalRange": "[1,39 -> 1,40]",
39+
"modifiedRange": "[1,73 -> 2,6]"
40+
},
41+
{
42+
"originalRange": "[1,42 -> 1,42]",
43+
"modifiedRange": "[2,8 -> 2,18]"
44+
},
45+
{
46+
"originalRange": "[1,72 -> 1,73]",
47+
"modifiedRange": "[2,48 -> 2,59]"
48+
}
49+
]
50+
}
51+
]
52+
}

‎src/vs/editor/test/node/diffing/fixtures/penalize-fragmentation/experimental.human.diff.json

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
{
2+
"originalFileName": "./1.txt",
3+
"modifiedFileName": "./2.txt",
4+
"diffs": [
5+
{
6+
"originalRange": "[1,2)",
7+
"modifiedRange": "[1,3)",
8+
"innerChanges": [
9+
{
10+
"originalRange": "[1,1 -> 1,1]",
11+
"modifiedRange": "[1,1 -> 1,91]"
12+
},
13+
{
14+
"originalRange": "[1,17 -> 1,41]",
15+
"modifiedRange": "[2,17 -> 2,17]"
16+
},
17+
{
18+
"originalRange": "[1,72 -> 1,73]",
19+
"modifiedRange": "[2,48 -> 2,59]"
20+
}
21+
]
22+
}
23+
]
24+
}

‎src/vs/editor/test/node/diffing/fixtures/penalize-fragmentation/smart.expected.diff.json

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
{
2+
"originalFileName": "./1.txt",
3+
"modifiedFileName": "./2.txt",
4+
"diffs": [
5+
{
6+
"originalRange": "[1,2)",
7+
"modifiedRange": "[1,3)",
8+
"innerChanges": [
9+
{
10+
"originalRange": "[1,11 -> 1,20]",
11+
"modifiedRange": "[1,11 -> 1,77]"
12+
},
13+
{
14+
"originalRange": "[1,24 -> 1,41]",
15+
"modifiedRange": "[1,81 -> 2,17]"
16+
},
17+
{
18+
"originalRange": "[1,72 -> 1,73]",
19+
"modifiedRange": "[2,48 -> 2,59]"
20+
}
21+
]
22+
}
23+
]
24+
}

‎src/vs/workbench/contrib/mergeEditor/browser/model/diffComputer.ts

Lines changed: 8 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@
55

66
import { assertFn, checkAdjacentItems } from 'vs/base/common/assert';
77
import { IReader, observableFromEvent } from 'vs/base/common/observable';
8-
import { isDefined } from 'vs/base/common/types';
9-
import { Range } from 'vs/editor/common/core/range';
108
import { LineRange as DiffLineRange, RangeMapping as DiffRangeMapping } from 'vs/editor/common/diff/linesDiffComputer';
119
import { ITextModel } from 'vs/editor/common/model';
1210
import { IEditorWorkerService } from 'vs/editor/common/services/editorWorker';
@@ -60,17 +58,18 @@ export class MergeDiffComputer implements IMergeDiffComputer {
6058
textModel1,
6159
toLineRange(c.modifiedRange),
6260
textModel2,
63-
c.innerChanges?.map(ic => normalizeRangeMapping(toRangeMapping(ic), textModel1, textModel2)).filter(isDefined)
61+
c.innerChanges?.map(ic => toRangeMapping(ic))
6462
)
6563
);
6664

6765
assertFn(() => {
68-
return checkAdjacentItems(changes,
69-
(m1, m2) => m2.inputRange.startLineNumber - m1.inputRange.endLineNumberExclusive === m2.outputRange.startLineNumber - m1.outputRange.endLineNumberExclusive &&
70-
// There has to be an unchanged line in between (otherwise both diffs should have been joined)
71-
m1.inputRange.endLineNumberExclusive < m2.inputRange.startLineNumber &&
72-
m1.outputRange.endLineNumberExclusive < m2.outputRange.startLineNumber,
73-
);
66+
return changes[0].inputRange.startLineNumber === changes[0].outputRange.startLineNumber &&
67+
checkAdjacentItems(changes,
68+
(m1, m2) => m2.inputRange.startLineNumber - m1.inputRange.endLineNumberExclusive === m2.outputRange.startLineNumber - m1.outputRange.endLineNumberExclusive &&
69+
// There has to be an unchanged line in between (otherwise both diffs should have been joined)
70+
m1.inputRange.endLineNumberExclusive < m2.inputRange.startLineNumber &&
71+
m1.outputRange.endLineNumberExclusive < m2.outputRange.startLineNumber,
72+
);
7473
});
7574

7675
return {
@@ -86,79 +85,3 @@ export function toLineRange(range: DiffLineRange): LineRange {
8685
export function toRangeMapping(mapping: DiffRangeMapping): RangeMapping {
8786
return new RangeMapping(mapping.originalRange, mapping.modifiedRange);
8887
}
89-
90-
export function normalizeRangeMapping(rangeMapping: RangeMapping, inputTextModel: ITextModel, outputTextModel: ITextModel): RangeMapping | undefined {
91-
const inputRangeEmpty = rangeMapping.inputRange.isEmpty();
92-
const outputRangeEmpty = rangeMapping.outputRange.isEmpty();
93-
94-
if (inputRangeEmpty && outputRangeEmpty) {
95-
return undefined;
96-
}
97-
98-
if (rangeMapping.inputRange.startLineNumber > inputTextModel.getLineCount()
99-
|| rangeMapping.outputRange.startLineNumber > outputTextModel.getLineCount()) {
100-
return rangeMapping;
101-
}
102-
103-
const originalStartsAtEndOfLine = isAtEndOfLine(rangeMapping.inputRange.startLineNumber, rangeMapping.inputRange.startColumn, inputTextModel);
104-
const modifiedStartsAtEndOfLine = isAtEndOfLine(rangeMapping.outputRange.startLineNumber, rangeMapping.outputRange.startColumn, outputTextModel);
105-
106-
if (!inputRangeEmpty && !outputRangeEmpty && originalStartsAtEndOfLine && modifiedStartsAtEndOfLine) {
107-
// a b c [\n] x y z \n
108-
// d e f [\n a] \n
109-
// ->
110-
// a b c \n [] x y z \n
111-
// d e f \n [a] \n
112-
113-
return new RangeMapping(
114-
rangeMapping.inputRange.setStartPosition(rangeMapping.inputRange.startLineNumber + 1, 1),
115-
116-
rangeMapping.outputRange.setStartPosition(rangeMapping.outputRange.startLineNumber + 1, 1),
117-
);
118-
}
119-
120-
if (
121-
modifiedStartsAtEndOfLine &&
122-
originalStartsAtEndOfLine &&
123-
((inputRangeEmpty && rangeEndsAtEndOfLine(rangeMapping.outputRange, outputTextModel)) ||
124-
(outputRangeEmpty && rangeEndsAtEndOfLine(rangeMapping.inputRange, inputTextModel)))
125-
) {
126-
// o: a b c [] \n x y z \n
127-
// m: d e f [\n a] \n
128-
// ->
129-
// o: a b c \n [] x y z \n
130-
// m: d e f \n [a \n]
131-
132-
// or
133-
134-
// a b c [\n x y z] \n
135-
// d e f [] \n a \n
136-
// ->
137-
// a b c \n [x y z \n]
138-
// d e f \n [] a \n
139-
140-
return new RangeMapping(
141-
moveRange(rangeMapping.inputRange),
142-
moveRange(rangeMapping.outputRange)
143-
);
144-
}
145-
146-
return rangeMapping;
147-
}
148-
149-
function isAtEndOfLine(lineNumber: number, column: number, model: ITextModel): boolean {
150-
return column >= model.getLineMaxColumn(lineNumber);
151-
}
152-
153-
function rangeEndsAtEndOfLine(range: Range, model: ITextModel,): boolean {
154-
return isAtEndOfLine(range.endLineNumber, range.endColumn, model);
155-
}
156-
157-
function moveRange(range: Range): Range {
158-
return new Range(
159-
range.startLineNumber + 1,
160-
1,
161-
range.endLineNumber + 1,
162-
1,
163-
);
164-
}

‎src/vs/workbench/contrib/mergeEditor/browser/view/lineAlignment.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ export function getAlignments(m: ModifiedBaseRange): LineAlignment[] {
5959
}
6060
}
6161

62-
result.push([m.input1Range.endLineNumberExclusive, m.baseRange.endLineNumberExclusive, m.input2Range.endLineNumberExclusive]);
62+
const finalLineAlignment: LineAlignment = [m.input1Range.endLineNumberExclusive, m.baseRange.endLineNumberExclusive, m.input2Range.endLineNumberExclusive];
63+
result = result.filter(r => r.every((v, idx) => v !== finalLineAlignment[idx]));
64+
result.push(finalLineAlignment);
6365

6466
assertFn(() => checkAdjacentItems(result.map(r => r[0]).filter(isDefined), (a, b) => a < b)
6567
&& checkAdjacentItems(result.map(r => r[1]).filter(isDefined), (a, b) => a <= b)

‎src/vs/workbench/contrib/mergeEditor/test/browser/model.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { EndOfLinePreference, ITextModel } from 'vs/editor/common/model';
1414
import { createModelServices, createTextModel } from 'vs/editor/test/common/testTextModel';
1515
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
1616
import { NullTelemetryService } from 'vs/platform/telemetry/common/telemetryUtils';
17-
import { IMergeDiffComputer, IMergeDiffComputerResult, normalizeRangeMapping, toLineRange, toRangeMapping } from 'vs/workbench/contrib/mergeEditor/browser/model/diffComputer';
17+
import { IMergeDiffComputer, IMergeDiffComputerResult, toLineRange, toRangeMapping } from 'vs/workbench/contrib/mergeEditor/browser/model/diffComputer';
1818
import { DetailedLineRangeMapping } from 'vs/workbench/contrib/mergeEditor/browser/model/mapping';
1919
import { MergeEditorModel } from 'vs/workbench/contrib/mergeEditor/browser/model/mergeEditorModel';
2020
import { MergeEditorTelemetry } from 'vs/workbench/contrib/mergeEditor/browser/telemetry';
@@ -275,7 +275,7 @@ class MergeModelInterface extends Disposable {
275275
textModel1,
276276
toLineRange(c.modifiedRange),
277277
textModel2,
278-
c.innerChanges?.map(ic => normalizeRangeMapping(toRangeMapping(ic), textModel1, textModel2)).filter(isDefined)
278+
c.innerChanges?.map(ic => toRangeMapping(ic)).filter(isDefined)
279279
)
280280
);
281281
return {

0 commit comments

Comments
 (0)