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

fix: guard against undefined test results #354

Merged
merged 7 commits into from
Mar 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
1 change: 1 addition & 0 deletions src/execute/executeService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export class ExecuteService {
}
}
}
throw new Error(nls.localize('authForAnonymousApexFailed'));
peternhale marked this conversation as resolved.
Show resolved Hide resolved
}

@elapsedTime()
Expand Down
3 changes: 2 additions & 1 deletion src/i18n/i18n.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,6 @@ export const messages = {
covIdFormatErr: 'Cannot specify code coverage with a TestRunId result',
startHandshake: 'Attempting StreamingClient handshake',
finishHandshake: 'Finished StreamingClient handshake',
subscribeStarted: 'Subscribing to ApexLog events'
subscribeStarted: 'Subscribing to ApexLog events',
authForAnonymousApexFailed: 'The authentication for execute anonymous failed'
};
4 changes: 2 additions & 2 deletions src/i18n/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ import { Localization, Message } from './localization';

function loadMessageBundle(): Message {
try {
const layer = new Message(messages);
return layer;
return new Message(messages);
} catch (e) {
console.error('Cannot find messages in i18n module');
}
return undefined;
}

export const nls = new Localization(loadMessageBundle());
Expand Down
13 changes: 7 additions & 6 deletions src/streaming/streamingClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import { Client } from 'faye';
import { Connection, LoggerLevel } from '@salesforce/core';
import {
RetrieveResultsInterval,
StreamingErrors,
StreamMessage,
StreamingErrors,
TestResultMessage
} from './types';
import { Progress } from '../common';
Expand All @@ -19,7 +19,8 @@ import { elapsedTime, refreshAuth } from '../utils';
import {
ApexTestProgressValue,
ApexTestQueueItem,
ApexTestQueueItemRecord
ApexTestQueueItemRecord,
ApexTestQueueItemStatus
} from '../tests/types';

const TEST_RESULT_CHANNEL = '/systemTopic/TestResult';
Expand Down Expand Up @@ -298,10 +299,10 @@ export class StreamingClient {
if (
result.records.some(
(item) =>
item.Status === 'Queued' /* ApexTestQueueItemStatus.Queued */ ||
item.Status === 'Holding' /* ApexTestQueueItemStatus.Holding */ ||
item.Status === 'Preparing' /* ApexTestQueueItemStatus.Preparing */ ||
item.Status === 'Processing' /* ApexTestQueueItemStatus.Processing */
item.Status === ApexTestQueueItemStatus.Queued ||
item.Status === ApexTestQueueItemStatus.Holding ||
item.Status === ApexTestQueueItemStatus.Preparing ||
item.Status === ApexTestQueueItemStatus.Processing
)
) {
return null;
peternhale marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
75 changes: 34 additions & 41 deletions src/tests/asyncTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Connection } from '@salesforce/core';
import { CancellationToken, Progress } from '../common';
import { nls } from '../i18n';
import { AsyncTestRun, StreamingClient } from '../streaming';
import { formatStartTime, getCurrentTime } from '../utils';
import { elapsedTime, formatStartTime, getCurrentTime } from '../utils';
import { formatTestErrors, getAsyncDiagnostic } from './diagnosticUtil';
import {
ApexTestProgressValue,
Expand All @@ -20,7 +20,6 @@ import {
ApexTestResultData,
ApexTestResultOutcome,
ApexTestRunResult,
ApexTestRunResultRecord,
ApexTestRunResultStatus,
AsyncTestArrayConfiguration,
AsyncTestConfiguration,
Expand All @@ -32,9 +31,16 @@ import * as util from 'util';
import { QUERY_RECORD_LIMIT } from './constants';
import { CodeCoverage } from './codeCoverage';
import { HttpRequest } from 'jsforce';
import { elapsedTime } from '../utils/elapsedTime';
import { isValidTestRunID } from '../narrowing';

const finishedStatuses = [
ApexTestRunResultStatus.Aborted,
ApexTestRunResultStatus.Failed,
ApexTestRunResultStatus.Completed,
ApexTestRunResultStatus.Passed,
ApexTestRunResultStatus.Skipped
];

export class AsyncTests {
public readonly connection: Connection;
private readonly codecoverage: CodeCoverage;
Expand Down Expand Up @@ -83,12 +89,12 @@ export class AsyncTests {
}

const asyncRunResult = await sClient.subscribe(undefined, testRunId);
const testRunSummary = await this.checkRunStatus(asyncRunResult.runId);
const runResult = await this.checkRunStatus(asyncRunResult.runId);
return await this.formatAsyncResults(
asyncRunResult,
getCurrentTime(),
codeCoverage,
testRunSummary,
runResult.testRunSummary,
Copy link
Member

Choose a reason for hiding this comment

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

by making the change here, testRunSummary will return the result even if the finishedStatuses is Queued and Processing. But what does the result look like?

progress
);
} catch (e) {
Expand All @@ -113,13 +119,13 @@ export class AsyncTests {
await sClient.init();
await sClient.handshake();
let queueItem: ApexTestQueueItem;
let testRunSummary = await this.checkRunStatus(testRunId);
let runResult = await this.checkRunStatus(testRunId);

if (testRunSummary !== undefined) {
if (runResult.testsComplete) {
queueItem = await sClient.handler(undefined, testRunId);
} else {
queueItem = (await sClient.subscribe(undefined, testRunId)).queueItem;
testRunSummary = await this.checkRunStatus(testRunId);
runResult = await this.checkRunStatus(testRunId);
}

token &&
Expand All @@ -135,7 +141,7 @@ export class AsyncTests {
{ queueItem, runId: testRunId },
getCurrentTime(),
codeCoverage,
testRunSummary
runResult.testRunSummary
);
} catch (e) {
throw formatTestErrors(e);
Expand All @@ -146,50 +152,37 @@ export class AsyncTests {
public async checkRunStatus(
testRunId: string,
progress?: Progress<ApexTestProgressValue>
): Promise<ApexTestRunResultRecord | undefined> {
): Promise<{
testsComplete: boolean;
testRunSummary: ApexTestRunResult;
}> {
if (!isValidTestRunID(testRunId)) {
throw new Error(nls.localize('invalidTestRunIdErr', testRunId));
}

let testRunSummaryQuery =
'SELECT AsyncApexJobId, Status, ClassesCompleted, ClassesEnqueued, ';
testRunSummaryQuery +=
'MethodsEnqueued, StartTime, EndTime, TestTime, UserId ';
testRunSummaryQuery += `FROM ApexTestRunResult WHERE AsyncApexJobId = '${testRunId}'`;
const testRunSummaryQuery = `SELECT AsyncApexJobId, Status, ClassesCompleted, ClassesEnqueued, MethodsEnqueued, StartTime, EndTime, TestTime, UserId FROM ApexTestRunResult WHERE AsyncApexJobId = '${testRunId}'`;

progress?.report({
type: 'FormatTestResultProgress',
value: 'retrievingTestRunSummary',
message: nls.localize('retrievingTestRunSummary')
});

const testRunSummaryResults = (await this.connection.tooling.query(
testRunSummaryQuery,
{
autoFetch: true
}
)) as ApexTestRunResult;

if (testRunSummaryResults.records.length === 0) {
try {
const testRunSummaryResults =
await this.connection.singleRecordQuery<ApexTestRunResult>(
testRunSummaryQuery,
{
tooling: true
}
);
return {
testsComplete: finishedStatuses.includes(testRunSummaryResults.Status),
testRunSummary: testRunSummaryResults
};
} catch (e) {
throw new Error(nls.localize('noTestResultSummary', testRunId));
}

if (
testRunSummaryResults.records[0].Status ===
ApexTestRunResultStatus.Aborted ||
testRunSummaryResults.records[0].Status ===
ApexTestRunResultStatus.Failed ||
testRunSummaryResults.records[0].Status ===
ApexTestRunResultStatus.Completed ||
testRunSummaryResults.records[0].Status ===
ApexTestRunResultStatus.Passed ||
testRunSummaryResults.records[0].Status ===
ApexTestRunResultStatus.Skipped
) {
return testRunSummaryResults.records[0];
}

return undefined;
}

/**
Expand All @@ -206,7 +199,7 @@ export class AsyncTests {
asyncRunResult: AsyncTestRun,
commandStartTime: number,
codeCoverage = false,
testRunSummary: ApexTestRunResultRecord,
testRunSummary: ApexTestRunResult,
progress?: Progress<ApexTestProgressValue>
): Promise<TestResult> {
const coveredApexClassIdSet = new Set<string>();
Expand Down
8 changes: 3 additions & 5 deletions src/tests/testService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,11 +378,9 @@ export class TestService {
try {
if (tests) {
const payload = await this.buildTestPayload(tests);
const classes = payload.tests?.map((testItem) => {
if (testItem.className) {
return testItem.className;
}
});
const classes = payload.tests
?.filter((testItem) => testItem.className)
.map((testItem) => testItem.className);
if (new Set(classes).size !== 1) {
throw new Error(nls.localize('syncClassErr'));
}
Expand Down
12 changes: 3 additions & 9 deletions src/tests/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ export const enum ApexTestRunResultStatus {
Skipped = 'Skipped'
}

export type ApexTestRunResultRecord = {
export type ApexTestRunResult = {
/**
* The parent Apex job ID for the result
*/
Expand All @@ -267,23 +267,17 @@ export type ApexTestRunResultRecord = {
/**
* The time at which the test run started.
*/
StartTime: string;
StartTime: string | undefined;
/**
* The time it took the test to run, in seconds.
*/
TestTime: number;
TestTime: number | undefined;
/**
* The user who ran the test run
*/
UserId: string;
};

export type ApexTestRunResult = {
done: boolean;
totalSize: number;
records: ApexTestRunResultRecord[];
};

export const enum ApexTestQueueItemStatus {
Holding = 'Holding',
Queued = 'Queued',
Expand Down
6 changes: 5 additions & 1 deletion src/utils/dateUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ export function getCurrentTime(): number {
* @returns formatted date and time
*/
export function formatStartTime(
startTime: string | number,
startTime: string | number | undefined,
format: 'ISO' | 'locale' = 'locale'
): string {
if (!startTime) {
return '';
}

const date = new Date(startTime);
if (format === 'ISO') {
return date.toISOString();
Expand Down
3 changes: 1 addition & 2 deletions src/utils/elapsedTime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@

/* eslint-disable @typescript-eslint/no-explicit-any,@typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-return */

import { Logger, LoggerLevel } from '@salesforce/core';
import { LoggerLevelValue } from '@salesforce/core/lib/logger/logger';
import { Logger, LoggerLevel, LoggerLevelValue } from '@salesforce/core';

const log = (
level: LoggerLevelValue,
Expand Down
Loading
Loading