Merge pull request #3413 from github/koesie10/fix-tests-with-warnings

Show test results for tests with warnings

Author: koesie10

extensions/ql-vscode/src/codeql-cli/cli.ts

@@ -156,9 +156,17 @@ type ResolvedQueries = string[];
 type ResolvedTests = string[];
 
 /**
- * A compilation message for a test message (either an error or a warning)
+ * The severity of a compilation message for a test message.
  */
-interface CompilationMessage {
+export enum CompilationMessageSeverity {
+  Error = "ERROR",
+  Warning = "WARNING",
+}
+
+/**
+ * A compilation message for a test message (either an error or a warning).
+ */
+export interface CompilationMessage {
   /**
    * The text of the message
    */
@@ -170,7 +178,7 @@ interface CompilationMessage {
   /**
    * The severity of the message
    */
-  severity: number;
+  severity: CompilationMessageSeverity;
 }
 
 /**

extensions/ql-vscode/src/query-testing/test-manager.ts

@@ -21,7 +21,8 @@ import {
 } from "vscode";
 import { DisposableObject } from "../common/disposable-object";
 import { QLTestDiscovery } from "./qltest-discovery";
-import type { CodeQLCliServer } from "../codeql-cli/cli";
+import type { CodeQLCliServer, CompilationMessage } from "../codeql-cli/cli";
+import { CompilationMessageSeverity } from "../codeql-cli/cli";
 import { getErrorMessage } from "../common/helpers-pure";
 import type { BaseLogger, LogOptions } from "../common/logging";
 import type { TestRunner } from "./test-runner";
@@ -66,6 +67,23 @@ function changeExtension(p: string, ext: string): string {
   return p.slice(0, -extname(p).length) + ext;
 }
 
+function compilationMessageToTestMessage(
+  compilationMessage: CompilationMessage,
+): TestMessage {
+  const location = new Location(
+    Uri.file(compilationMessage.position.fileName),
+    new Range(
+      compilationMessage.position.line - 1,
+      compilationMessage.position.column - 1,
+      compilationMessage.position.endLine - 1,
+      compilationMessage.position.endColumn - 1,
+    ),
+  );
+  const testMessage = new TestMessage(compilationMessage.message);
+  testMessage.location = location;
+  return testMessage;
+}
+
 /**
  * Returns the complete text content of the specified file. If there is an error reading the file,
  * an error message is added to `testMessages` and this function returns undefined.
@@ -398,23 +416,15 @@ export class TestManager extends DisposableObject {
               );
             }
           }
-          if (event.messages?.length > 0) {
+          const errorMessages = event.messages.filter(
+            (m) => m.severity === CompilationMessageSeverity.Error,
+          );
+          if (errorMessages.length > 0) {
             // The test didn't make it far enough to produce results. Transform any error messages
             // into `TestMessage`s and report the test as "errored".
-            const testMessages = event.messages.map((m) => {
-              const location = new Location(
-                Uri.file(m.position.fileName),
-                new Range(
-                  m.position.line - 1,
-                  m.position.column - 1,
-                  m.position.endLine - 1,
-                  m.position.endColumn - 1,
-                ),
-              );
-              const testMessage = new TestMessage(m.message);
-              testMessage.location = location;
-              return testMessage;
-            });
+            const testMessages = event.messages.map(
+              compilationMessageToTestMessage,
+            );
             testRun.errored(testItem, testMessages, duration);
           } else {
             // Results didn't match expectations. Report the test as "failed".
@@ -423,6 +433,12 @@ export class TestManager extends DisposableObject {
               // here. Any failed test needs at least one message.
               testMessages.push(new TestMessage("Test failed"));
             }
+
+            // Add any warnings produced by the test to the test messages.
+            testMessages.push(
+              ...event.messages.map(compilationMessageToTestMessage),
+            );
+
             testRun.failed(testItem, testMessages, duration);
           }
         }

extensions/ql-vscode/test/vscode-tests/no-workspace/query-testing/test-adapter.test.ts

@@ -1,6 +1,7 @@
 import type { TestItem, TestItemCollection, TestRun } from "vscode";
 import {
   CancellationTokenSource,
+  Location,
   Range,
   TestRunRequest,
   Uri,
@@ -75,6 +76,11 @@ describe("test-adapter", () => {
         id: `test ${mockTestsInfo.hPath}`,
         uri: Uri.file(mockTestsInfo.hPath),
       } as TestItem,
+      {
+        children: { size: 0 } as TestItemCollection,
+        id: `test ${mockTestsInfo.kPath}`,
+        uri: Uri.file(mockTestsInfo.kPath),
+      } as TestItem,
     ];
     const childElements: IdTestItemPair[] = childItems.map((childItem) => [
       childItem.id,
@@ -87,15 +93,15 @@ describe("test-adapter", () => {
       id: `dir ${mockTestsInfo.testsPath}`,
       uri: Uri.file(mockTestsInfo.testsPath),
       children: {
-        size: 3,
+        size: 4,
         [Symbol.iterator]: childIteratorFunc,
       } as TestItemCollection,
     } as TestItem;
 
     const request = new TestRunRequest([rootItem]);
     await testManager.run(request, new CancellationTokenSource().token);
 
-    expect(enqueuedSpy).toHaveBeenCalledTimes(3);
+    expect(enqueuedSpy).toHaveBeenCalledTimes(4);
     expect(passedSpy).toHaveBeenCalledTimes(1);
     expect(passedSpy).toHaveBeenCalledWith(childItems[0], 3000);
     expect(erroredSpy).toHaveBeenCalledTimes(1);
@@ -112,6 +118,7 @@ describe("test-adapter", () => {
       ],
       4000,
     );
+    expect(failedSpy).toHaveBeenCalledTimes(2);
     expect(failedSpy).toHaveBeenCalledWith(
       childItems[2],
       [
@@ -121,7 +128,22 @@ describe("test-adapter", () => {
       ],
       11000,
     );
-    expect(failedSpy).toHaveBeenCalledTimes(1);
+    expect(failedSpy).toHaveBeenCalledWith(
+      childItems[3],
+      [
+        {
+          message: "Test failed",
+        },
+        {
+          message: "abc",
+          location: new Location(
+            Uri.file(mockTestsInfo.kPath),
+            new Range(0, 0, 1, 1),
+          ),
+        },
+      ],
+      15000,
+    );
     expect(endSpy).toHaveBeenCalledTimes(1);
   });
 });

extensions/ql-vscode/test/vscode-tests/no-workspace/query-testing/test-runner-helpers.ts

@@ -11,6 +11,7 @@ export const mockTestsInfo = {
   dPath: Uri.parse("file:/ab/c/d.ql").fsPath,
   gPath: Uri.parse("file:/ab/c/e/f/g.ql").fsPath,
   hPath: Uri.parse("file:/ab/c/e/f/h.ql").fsPath,
+  kPath: Uri.parse("file:/ab/c/e/f/k.ql").fsPath,
 };
 
 /**
@@ -89,6 +90,28 @@ function mockRunTests(): jest.Mock<any, any> {
         evaluationMs: 6000,
         messages: [],
       });
+      yield Promise.resolve({
+        test: mockTestsInfo.kPath,
+        pass: false,
+        diff: ["jkh", "tuv"],
+        failureStage: "RESULT",
+        compilationMs: 7000,
+        evaluationMs: 8000,
+        // a warning in an otherwise successful test
+        messages: [
+          {
+            position: {
+              fileName: mockTestsInfo.kPath,
+              line: 1,
+              column: 1,
+              endLine: 2,
+              endColumn: 2,
+            },
+            message: "abc",
+            severity: "WARNING",
+          },
+        ],
+      });
     })(),
   );
 

extensions/ql-vscode/test/vscode-tests/no-workspace/query-testing/test-runner.test.ts

@@ -94,7 +94,7 @@ describe("test-runner", () => {
       eventHandlerSpy,
     );
 
-    expect(eventHandlerSpy).toHaveBeenCalledTimes(3);
+    expect(eventHandlerSpy).toHaveBeenCalledTimes(4);
 
     expect(eventHandlerSpy).toHaveBeenNthCalledWith(1, {
       test: mockTestsInfo.dPath,
@@ -133,6 +133,27 @@ describe("test-runner", () => {
       failureStage: "RESULT",
       messages: [],
     });
+    expect(eventHandlerSpy).toHaveBeenNthCalledWith(4, {
+      test: mockTestsInfo.kPath,
+      pass: false,
+      compilationMs: 7000,
+      evaluationMs: 8000,
+      diff: ["jkh", "tuv"],
+      failureStage: "RESULT",
+      messages: [
+        {
+          position: {
+            fileName: mockTestsInfo.kPath,
+            line: 1,
+            column: 1,
+            endLine: 2,
+            endColumn: 2,
+          },
+          message: "abc",
+          severity: "WARNING",
+        },
+      ],
+    });
   });
 
   it("should reregister testproj databases around test run", async () => {