Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show test results for tests with warnings #3413

Merged
merged 2 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions extensions/ql-vscode/src/codeql-cli/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -170,7 +178,7 @@ interface CompilationMessage {
/**
* The severity of the message
*/
severity: number;
severity: CompilationMessageSeverity;
}

/**
Expand Down
48 changes: 32 additions & 16 deletions extensions/ql-vscode/src/query-testing/test-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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".
Expand All @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { TestItem, TestItemCollection, TestRun } from "vscode";
import {
CancellationTokenSource,
Location,
Range,
TestRunRequest,
Uri,
Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand All @@ -112,6 +118,7 @@ describe("test-adapter", () => {
],
4000,
);
expect(failedSpy).toHaveBeenCalledTimes(2);
expect(failedSpy).toHaveBeenCalledWith(
childItems[2],
[
Expand All @@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

/**
Expand Down Expand Up @@ -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",
},
],
});
})(),
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ describe("test-runner", () => {
eventHandlerSpy,
);

expect(eventHandlerSpy).toHaveBeenCalledTimes(3);
expect(eventHandlerSpy).toHaveBeenCalledTimes(4);

expect(eventHandlerSpy).toHaveBeenNthCalledWith(1, {
test: mockTestsInfo.dPath,
Expand Down Expand Up @@ -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 () => {
Expand Down
Loading