Fix some of our higher hitting crashes (#2719)

microsoft · Sep 9, 2022 · 7bd5fd7 · 7bd5fd7
1 parent 0618e92
commit 7bd5fd7
Show file tree

Hide file tree

Showing 7 changed files with 194 additions and 33 deletions.
diff --git a/src/ctest.ts b/src/ctest.ts
@@ -135,7 +135,7 @@ function cleanupResultsXml(messy: MessyResults): CTestResults {
         site: {
             $: messy.Site.$,
             testing: {
-                testList: testingHead.TestList.map(l => l.Test[0]),
+                testList: testingHead.TestList[0].Test,
                 test: testingHead.Test.map((test): Test => ({
                     fullName: test.FullName[0],
                     fullCommandLine: test.FullCommandLine[0],
@@ -150,11 +150,15 @@ function cleanupResultsXml(messy: MessyResults): CTestResults {
     };
 }
 
-export async function readTestResultsFile(testXml: string) {
-    const content = (await fs.readFile(testXml)).toString();
-    const data = await parseXmlString(content) as MessyResults;
-    const clean = cleanupResultsXml(data);
-    return clean;
+export async function readTestResultsFile(testXml: string): Promise<CTestResults | undefined> {
+    try {
+        const content = (await fs.readFile(testXml)).toString();
+        const data = await parseXmlString(content) as MessyResults;
+        const clean = cleanupResultsXml(data);
+        return clean;
+    } catch {
+        return undefined;
+    }
 }
 
 export function parseCatchTestOutput(output: string): FailingTestDecoration[] {
@@ -252,20 +256,16 @@ export class DecorationManager {
         const fails: vscode.DecorationOptions[] = [];
         const editorFile = util.lightNormalizePath(editor.document.fileName);
         for (const decor of this.failingTestDecorations) {
-            const decoratedFile = util.lightNormalizePath(
-                path.isAbsolute(decor.fileName) ? decor.fileName : path.join(this.binaryDir, decor.fileName));
+            const decoratedFile = util.lightNormalizePath(path.isAbsolute(decor.fileName) ? decor.fileName : path.join(this.binaryDir, decor.fileName));
             if (editorFile !== decoratedFile) {
                 continue;
             }
-            const fileLine = editor.document.lineAt(decor.lineNumber);
-            const range = new vscode.Range(decor.lineNumber,
-                fileLine.firstNonWhitespaceCharacterIndex,
-                decor.lineNumber,
-                fileLine.range.end.character);
-            fails.push({
-                hoverMessage: decor.hoverMessage,
-                range
-            });
+            try {
+                const fileLine = editor.document.lineAt(decor.lineNumber);
+                const range = new vscode.Range(decor.lineNumber, fileLine.firstNonWhitespaceCharacterIndex, decor.lineNumber, fileLine.range.end.character);
+                fails.push({ hoverMessage: decor.hoverMessage, range });
+            } catch {
+            }
         }
         editor.setDecorations(this.failingTestDecorationType, fails);
     }
@@ -343,11 +343,11 @@ export class CTestDriver implements vscode.Disposable {
     private readonly testsChangedEmitter = new vscode.EventEmitter<api.Test[]>();
     readonly onTestsChanged = this.testsChangedEmitter.event;
 
-    private _testResults: CTestResults | null = null;
-    get testResults(): CTestResults | null {
+    private _testResults?: CTestResults;
+    get testResults(): CTestResults | undefined {
         return this._testResults;
     }
-    set testResults(v: CTestResults | null) {
+    set testResults(v: CTestResults | undefined) {
         this._testResults = v;
         if (v) {
             const total = v.site.testing.test.length;
@@ -462,15 +462,15 @@ export class CTestDriver implements vscode.Disposable {
             console.assert(tagDir);
             await this.reloadTestResults(resultsFile);
         } else {
-            this.testResults = null;
+            this.testResults = undefined;
         }
 
         return tests;
     }
 
     private async reloadTestResults(testXml: string): Promise<void> {
         this.testResults = await readTestResultsFile(testXml);
-        const failing = this.testResults.site.testing.test.filter(t => t.status === 'failed');
+        const failing = this.testResults?.site.testing.test.filter(t => t.status === 'failed') || [];
         this.decorationManager.clearFailingTestDecorations();
         const newDecors = [] as FailingTestDecoration[];
         for (const t of failing) {
@@ -487,7 +487,7 @@ export class CTestDriver implements vscode.Disposable {
         if (!currentTestResults) {
             return;
         }
-        for (const cTestRes of currentTestResults.site.testing.test) {
+        for (const cTestRes of currentTestResults.site.testing.test || []) {
             cTestRes.status = 'notrun';
         }
         this.testResults = currentTestResults;

diff --git a/src/proc.ts b/src/proc.ts
@@ -176,6 +176,15 @@ export function execute(command: string, args?: string[], outputConsumer?: Outpu
         }
 
         const encoding = options.outputEncoding && iconv.encodingExists(options.outputEncoding) ? options.outputEncoding : 'utf8';
+        const accumulate = (str1: string, str2: string) => {
+            try {
+                return str1 + str2;
+            } catch {
+                // If the resulting string is longer than can be represented by `string.length`, an exception will be thrown.
+                // Don't accumulate any more content at this point.
+                return str1;
+            }
+        };
 
         result = new Promise<ExecutionResult>(resolve => {
             let stdout_acc = '';
@@ -195,7 +204,7 @@ export function execute(command: string, args?: string[], outputConsumer?: Outpu
                     const str = iconv.decode(Buffer.from(data), encoding);
                     const lines = str.split('\n').map(l => l.endsWith('\r') ? l.substr(0, l.length - 1) : l);
                     while (lines.length > 1) {
-                        line_acc += lines[0];
+                        line_acc = accumulate(line_acc, lines[0]);
                         if (outputConsumer) {
                             outputConsumer.output(line_acc);
                         } else if (util.isTestMode()) {
@@ -206,16 +215,16 @@ export function execute(command: string, args?: string[], outputConsumer?: Outpu
                         lines.splice(0, 1);
                     }
                     console.assert(lines.length, 'Invalid lines', JSON.stringify(lines));
-                    line_acc += lines[0];
-                    stdout_acc += str;
+                    line_acc = accumulate(line_acc, lines[0]);
+                    stdout_acc = accumulate(stdout_acc, str);
                 });
             });
             child?.stderr?.on('data', (data: Uint8Array) => {
                 rollbar.invoke(localize('processing.data.event.stderr', 'Processing {0} event from proc stderr', "\"data\""), { data, command, args }, () => {
                     const str = iconv.decode(Buffer.from(data), encoding);
                     const lines = str.split('\n').map(l => l.endsWith('\r') ? l.substr(0, l.length - 1) : l);
                     while (lines.length > 1) {
-                        stderr_line_acc += lines[0];
+                        stderr_line_acc = accumulate(stderr_line_acc, lines[0]);
                         if (outputConsumer) {
                             outputConsumer.error(stderr_line_acc);
                         } else if (util.isTestMode() && stderr_line_acc) {
@@ -226,8 +235,8 @@ export function execute(command: string, args?: string[], outputConsumer?: Outpu
                         lines.splice(0, 1);
                     }
                     console.assert(lines.length, 'Invalid lines', JSON.stringify(lines));
-                    stderr_line_acc += lines[0];
-                    stderr_acc += str;
+                    stderr_line_acc = accumulate(stderr_line_acc, lines[0]);
+                    stderr_acc = accumulate(stderr_acc, str);
                 });
             });
             // The 'close' event is emitted after a process has ended and the stdio streams of a child process have been closed.

diff --git a/test/unit-tests/TestResults.xml b/test/unit-tests/TestResults.xml
@@ -0,0 +1,93 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Site BuildName="(empty)"
+	BuildStamp="20220830-1901-Experimental"
+	Name="(empty)"
+	Generator="ctest-3.23.2"
+	CompilerName=""
+	CompilerVersion=""
+	OSName="Windows"
+	Hostname="localhost"
+	OSRelease=" Professional"
+	OSVersion=" (Build 19044)"
+	OSPlatform="AMD64"
+	Is64Bits="1"
+	VendorString="GenuineIntel"
+	VendorID="Intel Corporation"
+	FamilyID="6"
+	ModelID="15"
+	ProcessorCacheSize="-1"
+	NumberOfLogicalCPU="8"
+	NumberOfPhysicalCPU="4"
+	TotalVirtualMemory="24492"
+	TotalPhysicalMemory="16300"
+	LogicalProcessorsPerPhysical="2"
+	ProcessorClockFrequency="3491"
+	>
+	<Testing>
+		<StartDateTime>Aug 30 12:01 Pacific Daylight Time</StartDateTime>
+		<StartTestTime>1661886092</StartTestTime>
+		<TestList>
+			<Test>./test1</Test>
+			<Test>./test2</Test>
+		</TestList>
+		<Test Status="passed">
+			<Name>test1</Name>
+			<Path>.</Path>
+			<FullName>./test1</FullName>
+			<FullCommandLine>D:\cmaketest\build\Debug\mytest.exe</FullCommandLine>
+			<Results>
+				<NamedMeasurement type="numeric/double" name="Execution Time">
+					<Value>5.22339</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="numeric/double" name="Processors">
+					<Value>1</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Completion Status">
+					<Value>Completed</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Command Line">
+					<Value>D:\cmaketest\build\Debug\mytest.exe</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Environment">
+					<Value>#CTEST_RESOURCE_GROUP_COUNT=</Value>
+				</NamedMeasurement>
+				<Measurement>
+					<Value>D:/cmaketest/build/Debug/mytest.exe
+debug
+</Value>
+				</Measurement>
+			</Results>
+		</Test>
+		<Test Status="failed">
+			<Name>test2</Name>
+			<Path>.</Path>
+			<FullName>./test2</FullName>
+			<FullCommandLine>D:\cmaketest\build\Debug\mytest.exe</FullCommandLine>
+			<Results>
+				<NamedMeasurement type="numeric/double" name="Execution Time">
+					<Value>5.02486</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="numeric/double" name="Processors">
+					<Value>1</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Completion Status">
+					<Value>Completed</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Command Line">
+					<Value>D:\cmaketest\build\Debug\mytest.exe</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Environment">
+					<Value>#CTEST_RESOURCE_GROUP_COUNT=</Value>
+				</NamedMeasurement>
+				<Measurement>
+					<Value>D:/cmaketest/build/Debug/mytest.exe
+debug
+</Value>
+				</Measurement>
+			</Results>
+		</Test>
+		<EndDateTime>Aug 30 12:01 Pacific Daylight Time</EndDateTime>
+		<EndTestTime>1661886103</EndTestTime>
+		<ElapsedMinutes>0</ElapsedMinutes>
+	</Testing>
+</Site>
diff --git a/test/unit-tests/TestResults2.xml b/test/unit-tests/TestResults2.xml
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Site BuildName="(empty)"
+	BuildStamp="20220830-1901-Experimental"
+	Name="(empty)"
+	Generator="ctest-3.23.2"
+	CompilerName=""
+	CompilerVersion=""
+	OSName="Windows"
+	Hostname="localhost"
+	OSRelease=" Professional"
+	OSVersion=" (Build 19044)"
+	OSPlatform="AMD64"
+	Is64Bits="1"
+	VendorString="GenuineIntel"
+	VendorID="Intel Corporation"
+	FamilyID="6"
+	ModelID="15"
+	ProcessorCacheSize="-1"
+	NumberOfLogicalCPU="8"
+	NumberOfPhysicalCPU="4"
+	TotalVirtualMemory="24492"
+	TotalPhysicalMemory="16300"
+	LogicalProcessorsPerPhysical="2"
+	ProcessorClockFrequency="3491"
+	>
+	<Testing>
+		<StartDateTime>Aug 30 16:26 Pacific Daylight Time</StartDateTime>
+		<StartTestTime>1661902010</StartTestTime>
+		<TestList/>
+		<EndDateTime>Aug 30 16:26 Pacific Daylight Time</EndDateTime>
+		<EndTestTime>1661902010</EndTestTime>
+		<ElapsedMinutes>0</ElapsedMinutes>
+	</Testing>
+</Site>
diff --git a/test/unit-tests/cpptools.test.ts b/test/unit-tests/cpptools.test.ts
@@ -1,6 +1,6 @@
 /* eslint-disable no-unused-expressions */
 import { parseCompileFlags, getIntelliSenseMode, CppConfigurationProvider } from '@cmt/cpptools';
-import { expect } from '@test/util';
+import { expect, getTestResourceFilePath } from '@test/util';
 import { CMakeCache } from '@cmt/cache';
 import * as path from 'path';
 import * as codeModel from '@cmt/drivers/codeModel';
@@ -9,9 +9,6 @@ import { Version } from 'vscode-cpptools';
 import * as util from '@cmt/util';
 
 const here = __dirname;
-function getTestResourceFilePath(filename: string): string {
-    return path.normalize(path.join(here, '../../../test/unit-tests', filename));
-}
 
 suite('CppTools tests', () => {
     test('Parse some compiler flags', () => {

diff --git a/test/unit-tests/ctest.test.ts b/test/unit-tests/ctest.test.ts
@@ -0,0 +1,24 @@
+import { readTestResultsFile } from "@cmt/ctest";
+import { expect, getTestResourceFilePath } from "@test/util";
+
+suite('CTest test', () => {
+    test('Parse XML test results', async () => {
+        const result = await readTestResultsFile(getTestResourceFilePath('TestResults.xml'));
+        expect(result!.site.testing.testList.length).to.eq(2);
+        expect(result!.site.testing.test[0].name).to.eq('test1');
+        expect(result!.site.testing.test[0].status).to.eq('passed');
+        expect(result!.site.testing.test[1].name).to.eq('test2');
+        expect(result!.site.testing.test[1].status).to.eq('failed');
+    });
+
+    test('CTest enabled, but no tests', async () => {
+        const result = await readTestResultsFile(getTestResourceFilePath('TestResults2.xml'));
+        expect(result!.site.testing.testList.length).to.eq(0);
+        expect(result!.site.testing.test.length).to.eq(0);
+    });
+
+    test('Bad test results file', async () => {
+        const result = await readTestResultsFile(getTestResourceFilePath('TestCMakeCache.txt'));
+        expect(result).to.eq(undefined);
+    });
+});
diff --git a/test/util.ts b/test/util.ts
@@ -15,6 +15,10 @@ chai.use(chaiAsPromised);
 
 export { expect };
 
+export function getTestResourceFilePath(filename: string): string {
+    return path.normalize(path.join(__dirname, '../../test/unit-tests', filename));
+}
+
 export async function clearExistingKitConfigurationFile() {
     await fs.writeFile(path.join(paths.dataDir, 'cmake-kits.json'), '[]');
 }