Skip to content

Fix handling of test results with undefined status. #15348

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.

Sign up for GitHub

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

Closed
ericsnowcurrently wants to merge 4 commits into from
Closed

Fix handling of test results with undefined status. #15348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ac8acd
Do not create diagnostics for messages with undefined status.
ericsnowcurrently Feb 9, 2021
0bc6380
Do not emit messages for tests with undefined status.
ericsnowcurrently Feb 9, 2021
280c2ee
Split out IPythonTestPassMessage and IPythonTestFailMessage.
ericsnowcurrently Feb 9, 2021
8015cf5
Do not use "Ok" in test diagnostic messages.
ericsnowcurrently Feb 9, 2021
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
25 changes: 13 additions & 12 deletions src/client/testing/common/managers/baseTestManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { IServiceContainer } from '../../../ioc/types';
import { EventName } from '../../../telemetry/constants';
import { sendTelemetryEvent } from '../../../telemetry/index';
import { TestDiscoveryTelemetry, TestRunTelemetry } from '../../../telemetry/types';
import { IPythonTestMessage, ITestDiagnosticService, WorkspaceTestStatus } from '../../types';
import { IPythonTestFailMessage, IPythonTestMessage, ITestDiagnosticService, WorkspaceTestStatus } from '../../types';
import { copyDesiredTestResults } from '../testUtils';
import { CANCELLATION_REASON, CommandSource, TEST_OUTPUT_CHANNEL } from './../constants';
import {
Expand Down Expand Up @@ -415,7 +415,7 @@ export abstract class BaseTestManager implements ITestManager {
fs.arePathsSame(fileUri.fsPath, Uri.file(msg.testFilePath).fsPath) &&
msg.status !== TestStatus.Pass
) {
const diagnostic = this.createDiagnostics(msg);
const diagnostic = this.createDiagnostics(msg as IPythonTestFailMessage);
newDiagnostics.push(diagnostic);
}
}
Expand Down Expand Up @@ -491,24 +491,25 @@ export abstract class BaseTestManager implements ITestManager {
});
}

private createDiagnostics(message: IPythonTestMessage): Diagnostic {
const stackStart = message.locationStack![0];
const diagPrefix = this.unitTestDiagnosticService.getMessagePrefix(message.status!);
private createDiagnostics(message: IPythonTestFailMessage): Diagnostic {
const stackStart = message.locationStack[0];
const diagMsg = this.getDiagnosticMessage(message);
const severity = this.unitTestDiagnosticService.getSeverity(message.severity)!;
const diagMsg = message.message ? message.message.split('\n')[0] : '';
const diagnostic = new Diagnostic(
stackStart.location.range,
`${diagPrefix ? `${diagPrefix}: ` : 'Ok'}${diagMsg}`,
severity,
);
const diagnostic = new Diagnostic(stackStart.location.range, diagMsg, severity);
diagnostic.code = message.code;
diagnostic.source = message.provider;
const relatedInfoArr: DiagnosticRelatedInformation[] = [];
for (const frameDetails of message.locationStack!) {
for (const frameDetails of message.locationStack) {
const relatedInfo = new DiagnosticRelatedInformation(frameDetails.location, frameDetails.lineText);
relatedInfoArr.push(relatedInfo);
}
diagnostic.relatedInformation = relatedInfoArr;
return diagnostic;
}

private getDiagnosticMessage(message: IPythonTestFailMessage): string {
const diagPrefix = this.unitTestDiagnosticService.getMessagePrefix(message.status)!;
const diagMsg = message.message ? message.message.split('\n')[0] : '';
return `${diagPrefix}: ${diagMsg}`;
Copy link
Copy Markdown

@kimadeline kimadeline Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this change have no impact on the issue that caused us to introduce the "Ok" string in the diagnostic message? #14067

}
}
18 changes: 14 additions & 4 deletions src/client/testing/pytest/services/testMessageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ import { IFileSystem } from '../../../common/platform/types';
import { Product } from '../../../common/types';
import { IServiceContainer } from '../../../ioc/types';
import { FlattenedTestFunction, ITestMessageService, Tests, TestStatus } from '../../common/types';
import { ILocationStackFrameDetails, IPythonTestMessage, PythonTestMessageSeverity } from '../../types';
import {
ILocationStackFrameDetails,
IPythonTestFailMessage,
IPythonTestMessage,
IPythonTestPassMessage,
PythonTestMessageSeverity,
} from '../../types';

@injectable()
export class TestMessageService implements ITestMessageService {
Expand All @@ -33,12 +39,16 @@ export class TestMessageService implements ITestMessageService {
}, []);
const messages: IPythonTestMessage[] = [];
for (const tf of testFuncs) {
if (tf.testFunction.status === undefined) {
// The test results were not updated after the test run.
continue;
}
const nameToRun = tf.testFunction.nameToRun;
const provider = ProductNames.get(Product.pytest)!;
const status = tf.testFunction.status!;
const status = tf.testFunction.status;
if (status === TestStatus.Pass) {
// If the test passed, there's not much to do with it.
const msg: IPythonTestMessage = {
const msg: IPythonTestPassMessage = {
code: nameToRun,
severity: PythonTestMessageSeverity.Pass,
provider: provider,
Expand All @@ -59,7 +69,7 @@ export class TestMessageService implements ITestMessageService {
severity = PythonTestMessageSeverity.Skip;
}

const msg: IPythonTestMessage = {
const msg: IPythonTestFailMessage = {
code: nameToRun,
message: message,
severity: severity,
Expand Down
26 changes: 18 additions & 8 deletions src/client/testing/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,17 +169,27 @@ export interface ITestDiagnosticService {
getSeverity(unitTestSeverity: PythonTestMessageSeverity): DiagnosticSeverity | undefined;
}

export interface IPythonTestMessage {
code: string | undefined;
message?: string;
interface IPythonTestMessageCommon {
code: string;
testFilePath: string;
status: TestStatus;
severity: PythonTestMessageSeverity;
provider: string | undefined;
traceback?: string;
testTime: number;
status?: TestStatus;
locationStack?: ILocationStackFrameDetails[];
testFilePath: string;
provider: string;
}
export interface IPythonTestPassMessage extends IPythonTestMessageCommon {
status: TestStatus.Pass;
severity: PythonTestMessageSeverity.Pass;
}
export interface IPythonTestFailMessage extends IPythonTestMessageCommon {
status: Exclude<TestStatus, TestStatus.Pass>;
severity: PythonTestMessageSeverity.Error | PythonTestMessageSeverity.Skip;
Copy link
Copy Markdown

@kimadeline kimadeline Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we handling PythonTestMessageSeverity.Failure?

Copy link
Copy Markdown
Author

@ericsnowcurrently ericsnowcurrently Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll fix that (properly).

// The following are failure-specific.
message?: string;
traceback?: string;
locationStack: ILocationStackFrameDetails[];
}
export type IPythonTestMessage = IPythonTestPassMessage | IPythonTestFailMessage;
export enum PythonTestMessageSeverity {
Error,
Failure,
Expand Down
33 changes: 18 additions & 15 deletions src/test/testing/pytest/pytest.testMessageService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { XUnitParser } from '../../../client/testing/common/xUnitParser';
import { TestMessageService } from '../../../client/testing/pytest/services/testMessageService';
import {
ILocationStackFrameDetails,
IPythonTestFailMessage,
IPythonTestMessage,
PythonTestMessageSeverity,
} from '../../../client/testing/types';
Expand Down Expand Up @@ -54,45 +55,47 @@ async function testMessageProperties(
imported: boolean = false,
status: TestStatus,
) {
const failure = message as IPythonTestFailMessage;
const expectedFailure = expectedMessage as IPythonTestFailMessage;
assert.equal(message.code, expectedMessage.code, 'IPythonTestMessage code');
assert.equal(message.message, expectedMessage.message, 'IPythonTestMessage message');
assert.equal(failure.message, expectedFailure.message, 'IPythonTestMessage message');
assert.equal(message.severity, expectedMessage.severity, 'IPythonTestMessage severity');
assert.equal(message.provider, expectedMessage.provider, 'IPythonTestMessage provider');
assert.isNumber(message.testTime, 'IPythonTestMessage testTime');
assert.equal(message.status, expectedMessage.status, 'IPythonTestMessage status');
assert.equal(message.testFilePath, expectedMessage.testFilePath, 'IPythonTestMessage testFilePath');
if (status !== TestStatus.Pass) {
assert.equal(
message.locationStack![0].lineText,
expectedMessage.locationStack![0].lineText,
failure.locationStack[0].lineText,
expectedFailure.locationStack[0].lineText,
'IPythonTestMessage line text',
);
assert.equal(
message.locationStack![0].location.uri.fsPath,
expectedMessage.locationStack![0].location.uri.fsPath,
failure.locationStack[0].location.uri.fsPath,
expectedFailure.locationStack[0].location.uri.fsPath,
'IPythonTestMessage locationStack fsPath',
);
if (status !== TestStatus.Skipped) {
assert.equal(
message.locationStack![1].lineText,
expectedMessage.locationStack![1].lineText,
failure.locationStack[1].lineText,
expectedFailure.locationStack[1].lineText,
'IPythonTestMessage line text',
);
assert.equal(
message.locationStack![1].location.uri.fsPath,
expectedMessage.locationStack![1].location.uri.fsPath,
failure.locationStack[1].location.uri.fsPath,
expectedFailure.locationStack[1].location.uri.fsPath,
'IPythonTestMessage locationStack fsPath',
);
}
if (imported) {
assert.equal(
message.locationStack![2].lineText,
expectedMessage.locationStack![2].lineText,
failure.locationStack[2].lineText,
expectedFailure.locationStack[2].lineText,
'IPythonTestMessage imported line text',
);
assert.equal(
message.locationStack![2].location.uri.fsPath,
expectedMessage.locationStack![2].location.uri.fsPath,
failure.locationStack[2].location.uri.fsPath,
expectedFailure.locationStack[2].location.uri.fsPath,
'IPythonTestMessage imported location fsPath',
);
}
Expand Down Expand Up @@ -223,14 +226,14 @@ suite('Unit Tests - PyTest - TestMessageService', () => {
const expectedLocationStack = await getExpectedLocationStackFromTestDetails(td);
expectedMessage = {
code: td.nameToRun,
message: td.message,
severity: expectedSeverity,
provider: ProductNames.get(Product.pytest)!,
testTime: 0,
status: td.status,
message: td.message,
locationStack: expectedLocationStack,
testFilePath: path.join(UNITTEST_TEST_FILES_PATH, td.fileName),
};
} as IPythonTestFailMessage;
testMessage = testMessages.find((tm) => tm.code === td.nameToRun)!;
});
test('Message', async () => {
Expand Down