From ed1a928af2d74cd1ec7951159809df531aab7542 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 13 Sep 2017 12:35:49 -0700 Subject: [PATCH 1/2] Don't normalize whitespace in fourslash tests --- src/harness/fourslash.ts | 69 +++++++------------ src/harness/harness.ts | 3 +- src/harness/harnessLanguageService.ts | 2 +- src/server/client.ts | 3 +- src/server/protocol.ts | 1 + src/server/session.ts | 2 +- src/services/services.ts | 2 +- src/services/textChanges.ts | 6 +- .../fourslash/codeFixAddMissingMember5.ts | 3 +- .../fourslash/codeFixAddMissingMember7.ts | 3 +- .../cases/fourslash/codeFixSuperAfterThis.ts | 5 +- tests/cases/fourslash/codeFixSuperCall.ts | 3 +- tests/cases/fourslash/extract-method14.ts | 2 +- .../fourslash/formatConflictDiff3Marker1.ts | 18 ++--- .../cases/fourslash/formatConflictMarker1.ts | 14 ++-- .../fourslash/importNameCodeFixReExport.ts | 5 +- 16 files changed, 62 insertions(+), 79 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index d89a4677ef90a..cc459e6bfc3c0 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -22,10 +22,6 @@ namespace FourSlash { ts.disableIncrementalParsing = false; - function normalizeNewLines(s: string) { - return s.replace(/\r\n/g, "\n"); - } - // Represents a parsed source file with metadata export interface FourSlashFile { // The contents of the file (with markers, etc stripped out) @@ -364,7 +360,7 @@ namespace FourSlash { baseIndentSize: 0, indentSize: 4, tabSize: 4, - newLineCharacter: Harness.IO.newLine(), + newLineCharacter: "\n", convertTabsToSpaces: true, indentStyle: ts.IndentStyle.Smart, insertSpaceAfterCommaDelimiter: true, @@ -1603,7 +1599,7 @@ namespace FourSlash { } } - public printCurrentFileState(makeWhitespaceVisible: boolean, makeCaretVisible: boolean) { + public printCurrentFileState(showWhitespace: boolean, makeCaretVisible: boolean) { for (const file of this.testData.files) { const active = (this.activeFile === file); Harness.IO.log(`=== Script (${file.fileName}) ${(active ? "(active, cursor at |)" : "")} ===`); @@ -1611,8 +1607,8 @@ namespace FourSlash { if (active) { content = content.substr(0, this.currentCaretPosition) + (makeCaretVisible ? "|" : "") + content.substr(this.currentCaretPosition); } - if (makeWhitespaceVisible) { - content = TestState.makeWhitespaceVisible(content); + if (showWhitespace) { + content = makeWhitespaceVisible(content); } Harness.IO.log(content); } @@ -2128,10 +2124,10 @@ namespace FourSlash { public verifyCurrentFileContent(text: string) { const actual = this.getFileContent(this.activeFile.fileName); - if (normalizeNewLines(actual) !== normalizeNewLines(text)) { + if (actual !== text) { throw new Error("verifyCurrentFileContent\n" + - "\tExpected: \"" + TestState.makeWhitespaceVisible(text) + "\"\n" + - "\t Actual: \"" + TestState.makeWhitespaceVisible(actual) + "\""); + "\tExpected: \"" + makeWhitespaceVisible(text) + "\"\n" + + "\t Actual: \"" + makeWhitespaceVisible(actual) + "\""); } } @@ -2305,11 +2301,11 @@ namespace FourSlash { const actualText = this.rangeText(ranges[0]); const result = includeWhiteSpace - ? normalizeNewLines(actualText) === normalizeNewLines(expectedText) + ? actualText === expectedText : this.removeWhitespace(actualText) === this.removeWhitespace(expectedText); if (!result) { - this.raiseError(`Actual text doesn't match expected text. Actual:\n'${actualText}'\nExpected:\n'${expectedText}'`); + this.raiseError(`Actual range text doesn't match expected text. Actual:\n'${makeWhitespaceVisible(actualText)}'\nExpected:\n'${makeWhitespaceVisible(expectedText)}'`); } } @@ -2403,14 +2399,17 @@ namespace FourSlash { const originalContent = scriptInfo.content; for (const codeFix of codeFixes) { this.applyEdits(codeFix.changes[0].fileName, codeFix.changes[0].textChanges, /*isFormattingEdit*/ false); - actualTextArray.push(this.normalizeNewlines(this.rangeText(ranges[0]))); + let text = this.rangeText(ranges[0]); + // TODO:GH#18445 (remove this line to see errors in many `importNameCodeFix` tests) + text = text.replace(/\r\n/g, "\n"); + actualTextArray.push(text); scriptInfo.updateContent(originalContent); } - const sortedExpectedArray = ts.map(expectedTextArray, str => this.normalizeNewlines(str)).sort(); + const sortedExpectedArray = expectedTextArray.sort(); const sortedActualArray = actualTextArray.sort(); if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) { this.raiseError( - `Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.join("\n\n")}'\n---\nExpected: \n'${sortedExpectedArray.join("\n\n")}'`); + `Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.map(makeWhitespaceVisible).join("\n\n")}'\n---\nExpected: \n'${sortedExpectedArray.map(makeWhitespaceVisible).join("\n\n")}'`); } } @@ -2431,7 +2430,7 @@ namespace FourSlash { } if (actual.newText !== expected.newText) { - this.raiseError(`${name} failed - expected insertion:\n"${this.clarifyNewlines(expected.newText)}"\nactual insertion:\n"${this.clarifyNewlines(actual.newText)}"`); + this.raiseError(`${name} failed - expected insertion:\n"${makeWhitespaceVisible(expected.newText)}"\nactual insertion:\n"${makeWhitespaceVisible(actual.newText)}"`); } if (actual.caretOffset !== expected.caretOffset) { @@ -2440,17 +2439,6 @@ namespace FourSlash { } } - private clarifyNewlines(str: string) { - return str.replace(/\r?\n/g, lineEnding => { - const representation = lineEnding === "\r\n" ? "CRLF" : "LF"; - return "# - " + representation + lineEnding; - }); - } - - private normalizeNewlines(str: string) { - return str.replace(/\r?\n/g, "\n"); - } - public verifyBraceCompletionAtPosition(negative: boolean, openingBrace: string) { const openBraceMap = ts.createMapFromTemplate({ @@ -2878,8 +2866,8 @@ namespace FourSlash { } const actualContent = this.getFileContent(this.activeFile.fileName); - if (this.normalizeNewlines(actualContent) !== this.normalizeNewlines(expectedContent)) { - this.raiseError(`verifyFileAfterApplyingRefactors failed: expected:\n${expectedContent}\nactual:\n${actualContent}`); + if (actualContent !== expectedContent) { + this.raiseError(`verifyFileAfterApplyingRefactors failed: expected:\n${makeWhitespaceVisible(expectedContent)}\nactual:\n${makeWhitespaceVisible(actualContent)}`); } } @@ -3014,10 +3002,6 @@ namespace FourSlash { } } - private static makeWhitespaceVisible(text: string) { - return text.replace(/ /g, "\u00B7").replace(/\r/g, "\u00B6").replace(/\n/g, "\u2193\n").replace(/\t/g, "\u2192\ "); - } - public setCancelled(numberOfCalls: number): void { this.cancellationToken.setCancelled(numberOfCalls); } @@ -3319,12 +3303,7 @@ ${code} let column = 1; const flush = (lastSafeCharIndex: number) => { - if (lastSafeCharIndex === undefined) { - output = output + content.substr(lastNormalCharPosition); - } - else { - output = output + content.substr(lastNormalCharPosition, lastSafeCharIndex - lastNormalCharPosition); - } + output = output + content.substr(lastNormalCharPosition, lastSafeCharIndex === undefined ? undefined : lastSafeCharIndex - lastNormalCharPosition); }; if (content.length > 0) { @@ -3511,6 +3490,10 @@ ${code} function toArray(x: T | T[]): T[] { return ts.isArray(x) ? x : [x]; } + + function makeWhitespaceVisible(text: string) { + return text.replace(/ /g, "\u00B7").replace(/\r/g, "\u00B6").replace(/\n/g, "\u2193\n").replace(/\t/g, "\u2192\ "); + } } namespace FourSlashInterface { @@ -4143,15 +4126,15 @@ namespace FourSlashInterface { } public printCurrentFileState() { - this.state.printCurrentFileState(/*makeWhitespaceVisible*/ false, /*makeCaretVisible*/ true); + this.state.printCurrentFileState(/*showWhitespace*/ false, /*makeCaretVisible*/ true); } public printCurrentFileStateWithWhitespace() { - this.state.printCurrentFileState(/*makeWhitespaceVisible*/ true, /*makeCaretVisible*/ true); + this.state.printCurrentFileState(/*showWhitespace*/ true, /*makeCaretVisible*/ true); } public printCurrentFileStateWithoutCaret() { - this.state.printCurrentFileState(/*makeWhitespaceVisible*/ false, /*makeCaretVisible*/ false); + this.state.printCurrentFileState(/*showWhitespace*/ false, /*makeCaretVisible*/ false); } public printCurrentQuickInfo() { diff --git a/src/harness/harness.ts b/src/harness/harness.ts index 2fc1aac2d8328..8344be36753ed 100644 --- a/src/harness/harness.ts +++ b/src/harness/harness.ts @@ -500,7 +500,8 @@ namespace Harness { export let IO: IO; // harness always uses one kind of new line - const harnessNewLine = "\r\n"; + // But note that `parseTestData` in `fourslash.ts` uses "\n" + export const harnessNewLine = "\r\n"; // Root for file paths that are stored in a virtual file system export const virtualFileSystemRoot = "/"; diff --git a/src/harness/harnessLanguageService.ts b/src/harness/harnessLanguageService.ts index 78da05570d82e..23bc08108c764 100644 --- a/src/harness/harnessLanguageService.ts +++ b/src/harness/harnessLanguageService.ts @@ -130,7 +130,7 @@ namespace Harness.LanguageService { } public getNewLine(): string { - return "\r\n"; + return harnessNewLine; } public getFilenames(): string[] { diff --git a/src/server/client.ts b/src/server/client.ts index 0ffac42dae4d4..9e472a83e2b7a 100644 --- a/src/server/client.ts +++ b/src/server/client.ts @@ -573,7 +573,7 @@ namespace ts.server { getEditsForRefactor( fileName: string, - _formatOptions: FormatCodeSettings, + formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string): RefactorEditInfo { @@ -581,6 +581,7 @@ namespace ts.server { const args = this.createFileLocationOrRangeRequestArgs(positionOrRange, fileName) as protocol.GetEditsForRefactorRequestArgs; args.refactor = refactorName; args.action = actionName; + args.formatOptions = formatOptions; const request = this.processRequest(CommandNames.GetEditsForRefactor, args); const response = this.processResponse(request); diff --git a/src/server/protocol.ts b/src/server/protocol.ts index 3fdbd8fd7f7d8..1740f8ae0ba1d 100644 --- a/src/server/protocol.ts +++ b/src/server/protocol.ts @@ -494,6 +494,7 @@ namespace ts.server.protocol { refactor: string; /* The 'name' property from the refactoring action */ action: string; + formatOptions: FormatCodeSettings, }; diff --git a/src/server/session.ts b/src/server/session.ts index 0b3038a408d87..2d773d6f467da 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -1488,7 +1488,7 @@ namespace ts.server { const result = project.getLanguageService().getEditsForRefactor( file, - this.projectService.getFormatCodeOptions(), + convertFormatOptions(args.formatOptions), position || textRange, args.refactor, args.action diff --git a/src/services/services.ts b/src/services/services.ts index 197f76b607f32..bdd0eb41e4fd8 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2019,7 +2019,7 @@ namespace ts { startPosition, endPosition, program: getProgram(), - newLineCharacter: host.getNewLine(), + newLineCharacter: formatOptions ? formatOptions.newLineCharacter : host.getNewLine(), rulesProvider: getRuleProvider(formatOptions), cancellationToken }; diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 42c1d1e9a4fd0..f3ad7df16076e 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -184,16 +184,12 @@ namespace ts.textChanges { return s; } - function getNewlineKind(context: { newLineCharacter: string }) { - return context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed; - } - export class ChangeTracker { private changes: Change[] = []; private readonly newLineCharacter: string; public static fromContext(context: RefactorContext | CodeFixContext) { - return new ChangeTracker(getNewlineKind(context), context.rulesProvider); + return new ChangeTracker(context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed, context.rulesProvider); } constructor( diff --git a/tests/cases/fourslash/codeFixAddMissingMember5.ts b/tests/cases/fourslash/codeFixAddMissingMember5.ts index db893cb61d3fe..44b11c5141b21 100644 --- a/tests/cases/fourslash/codeFixAddMissingMember5.ts +++ b/tests/cases/fourslash/codeFixAddMissingMember5.ts @@ -17,5 +17,4 @@ verify.currentFileContentIs(`class C { ()=>{ this.foo === 10 }; } } -C.foo = undefined; -`); \ No newline at end of file +C.foo = undefined;` + "\r\n"); // TODO: GH#18445 diff --git a/tests/cases/fourslash/codeFixAddMissingMember7.ts b/tests/cases/fourslash/codeFixAddMissingMember7.ts index 8ac7f2b5aff0d..7690023815cf9 100644 --- a/tests/cases/fourslash/codeFixAddMissingMember7.ts +++ b/tests/cases/fourslash/codeFixAddMissingMember7.ts @@ -13,5 +13,4 @@ verify.getAndApplyCodeFix(/*errorCode*/ undefined, /*index*/ 2) verify.currentFileContentIs(`class C { static p = ()=>{ this.foo === 10 }; } -C.foo = undefined; -`); +C.foo = undefined;` + "\r\n"); // TODO: GH#18445 diff --git a/tests/cases/fourslash/codeFixSuperAfterThis.ts b/tests/cases/fourslash/codeFixSuperAfterThis.ts index 55b44b0788136..a4db2c909fdcc 100644 --- a/tests/cases/fourslash/codeFixSuperAfterThis.ts +++ b/tests/cases/fourslash/codeFixSuperAfterThis.ts @@ -9,7 +9,8 @@ //// super(); //// |]} ////} +// TODO: GH#18445 verify.rangeAfterCodeFix(` - super(); + super();\r this.a = 12; - `, /*includeWhiteSpace*/ true); \ No newline at end of file + `, /*includeWhiteSpace*/ true); diff --git a/tests/cases/fourslash/codeFixSuperCall.ts b/tests/cases/fourslash/codeFixSuperCall.ts index f7584050a153d..28f7d34a2bd4c 100644 --- a/tests/cases/fourslash/codeFixSuperCall.ts +++ b/tests/cases/fourslash/codeFixSuperCall.ts @@ -6,6 +6,7 @@ //// constructor() {[| //// |]} ////} +// TODO: GH#18445 verify.rangeAfterCodeFix(` - super(); + super();\r `, /*includeWhitespace*/ true); diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts index 27a561743c65e..e2b58a36450c8 100644 --- a/tests/cases/fourslash/extract-method14.ts +++ b/tests/cases/fourslash/extract-method14.ts @@ -19,7 +19,7 @@ edit.applyRefactor({ `function foo() { var i = 10; var __return: any; - ({ __return, i } = n/*RENAME*/ewFunction(i)); + ({ __return, i } = /*RENAME*/newFunction(i)); return __return; } function newFunction(i) { diff --git a/tests/cases/fourslash/formatConflictDiff3Marker1.ts b/tests/cases/fourslash/formatConflictDiff3Marker1.ts index f6492a6f60c51..188fe5b0dd205 100644 --- a/tests/cases/fourslash/formatConflictDiff3Marker1.ts +++ b/tests/cases/fourslash/formatConflictDiff3Marker1.ts @@ -11,12 +11,12 @@ ////} format.document(); -verify.currentFileContentIs("class C {\r\n\ -<<<<<<< HEAD\r\n\ - v = 1;\r\n\ -||||||| merged common ancestors\r\n\ -v = 3;\r\n\ -=======\r\n\ -v = 2;\r\n\ ->>>>>>> Branch - a\r\n\ -}"); \ No newline at end of file +verify.currentFileContentIs(`class C { +<<<<<<< HEAD + v = 1; +||||||| merged common ancestors +v = 3; +======= +v = 2; +>>>>>>> Branch - a +}`); diff --git a/tests/cases/fourslash/formatConflictMarker1.ts b/tests/cases/fourslash/formatConflictMarker1.ts index c0e2dbc4f69dc..413206dc893ef 100644 --- a/tests/cases/fourslash/formatConflictMarker1.ts +++ b/tests/cases/fourslash/formatConflictMarker1.ts @@ -9,10 +9,10 @@ ////} format.document(); -verify.currentFileContentIs("class C {\r\n\ -<<<<<<< HEAD\r\n\ - v = 1;\r\n\ -=======\r\n\ -v = 2;\r\n\ ->>>>>>> Branch - a\r\n\ -}"); \ No newline at end of file +verify.currentFileContentIs(`class C { +<<<<<<< HEAD + v = 1; +======= +v = 2; +>>>>>>> Branch - a +}`); diff --git a/tests/cases/fourslash/importNameCodeFixReExport.ts b/tests/cases/fourslash/importNameCodeFixReExport.ts index c77d32cc45828..eb1c1a9134343 100644 --- a/tests/cases/fourslash/importNameCodeFixReExport.ts +++ b/tests/cases/fourslash/importNameCodeFixReExport.ts @@ -10,7 +10,8 @@ ////x;|] goTo.file("/b.ts"); -verify.rangeAfterCodeFix(`import { x } from "./a"; - +// TODO:GH#18445 +verify.rangeAfterCodeFix(`import { x } from "./a";\r +\r export { x } from "./a"; x;`, /*includeWhiteSpace*/ true); From cec25d944af25aa674f751cc4c4b1447b9860c95 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 14 Sep 2017 07:43:17 -0700 Subject: [PATCH 2/2] Only render whitespace when the diff is text-only --- src/harness/fourslash.ts | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index cc459e6bfc3c0..fb5e9e0354e72 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2125,9 +2125,7 @@ namespace FourSlash { public verifyCurrentFileContent(text: string) { const actual = this.getFileContent(this.activeFile.fileName); if (actual !== text) { - throw new Error("verifyCurrentFileContent\n" + - "\tExpected: \"" + makeWhitespaceVisible(text) + "\"\n" + - "\t Actual: \"" + makeWhitespaceVisible(actual) + "\""); + throw new Error(`verifyCurrentFileContent failed:\n${showTextDiff(text, actual)}`); } } @@ -2305,7 +2303,7 @@ namespace FourSlash { : this.removeWhitespace(actualText) === this.removeWhitespace(expectedText); if (!result) { - this.raiseError(`Actual range text doesn't match expected text. Actual:\n'${makeWhitespaceVisible(actualText)}'\nExpected:\n'${makeWhitespaceVisible(expectedText)}'`); + this.raiseError(`Actual range text doesn't match expected text.\n${showTextDiff(expectedText, actualText)}`); } } @@ -2407,10 +2405,11 @@ namespace FourSlash { } const sortedExpectedArray = expectedTextArray.sort(); const sortedActualArray = actualTextArray.sort(); - if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) { - this.raiseError( - `Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.map(makeWhitespaceVisible).join("\n\n")}'\n---\nExpected: \n'${sortedExpectedArray.map(makeWhitespaceVisible).join("\n\n")}'`); - } + ts.zipWith(sortedExpectedArray, sortedActualArray, (expected, actual, index) => { + if (expected !== actual) { + this.raiseError(`Import fix at index ${index} doesn't match.\n${showTextDiff(expected, actual)}`); + } + }); } public verifyDocCommentTemplate(expected: ts.TextInsertion | undefined) { @@ -2430,7 +2429,7 @@ namespace FourSlash { } if (actual.newText !== expected.newText) { - this.raiseError(`${name} failed - expected insertion:\n"${makeWhitespaceVisible(expected.newText)}"\nactual insertion:\n"${makeWhitespaceVisible(actual.newText)}"`); + this.raiseError(`${name} failed for expected insertion.\n${showTextDiff(expected.newText, actual.newText)}`); } if (actual.caretOffset !== expected.caretOffset) { @@ -2867,7 +2866,7 @@ namespace FourSlash { const actualContent = this.getFileContent(this.activeFile.fileName); if (actualContent !== expectedContent) { - this.raiseError(`verifyFileAfterApplyingRefactors failed: expected:\n${makeWhitespaceVisible(expectedContent)}\nactual:\n${makeWhitespaceVisible(actualContent)}`); + this.raiseError(`verifyFileAfterApplyingRefactors failed:\n${showTextDiff(expectedContent, actualContent)}`); } } @@ -3494,6 +3493,23 @@ ${code} function makeWhitespaceVisible(text: string) { return text.replace(/ /g, "\u00B7").replace(/\r/g, "\u00B6").replace(/\n/g, "\u2193\n").replace(/\t/g, "\u2192\ "); } + + function showTextDiff(expected: string, actual: string): string { + // Only show whitespace if the difference is whitespace-only. + if (differOnlyByWhitespace(expected, actual)) { + expected = makeWhitespaceVisible(expected); + actual = makeWhitespaceVisible(actual); + } + return `Expected:\n${expected}\nActual:${actual}`; + } + + function differOnlyByWhitespace(a: string, b: string) { + return stripWhitespace(a) === stripWhitespace(b); + } + + function stripWhitespace(s: string): string { + return s.replace(/\s/g, ""); + } } namespace FourSlashInterface {