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.

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

Jump to bottom

fix: guard against undefined test results #354

Merged
merged 7 commits into from
Mar 28, 2024
Merged

fix: guard against undefined test results #354

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f78cfc7
fix: guard against undefined test results
peternhale Mar 21, 2024
a46dda4
chore: apply review suggestions and write test
peternhale Mar 22, 2024
86ba3c4
chore: remove possibility of returning undefined
peternhale Mar 26, 2024
f6d9c6a
chore: address review issues
peternhale Mar 27, 2024
b163380
chore: fix typo
peternhale Mar 27, 2024
ea3c21b
Merge branch 'main' of github.com:forcedotcom/salesforcedx-apex into …
peternhale Mar 28, 2024
6b7579e
chore: merge main
peternhale Mar 28, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
20 changes: 10 additions & 10 deletions src/streaming/streamingClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,16 +296,16 @@ export class StreamingClient {
value: result
});

for (let i = 0; i < result.records.length; i++) {
const item = result.records[i];
if (
item.Status === ApexTestQueueItemStatus.Queued ||
item.Status === ApexTestQueueItemStatus.Holding ||
item.Status === ApexTestQueueItemStatus.Preparing ||
item.Status === ApexTestQueueItemStatus.Processing
) {
return null;
}
if (
result.records.some(
(item) =>
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
}
return result;
}
Expand Down
77 changes: 36 additions & 41 deletions src/tests/asyncTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

'use strict';

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 +22,6 @@ import {
ApexTestResultData,
ApexTestResultOutcome,
ApexTestRunResult,
ApexTestRunResultRecord,
ApexTestRunResultStatus,
AsyncTestArrayConfiguration,
AsyncTestConfiguration,
Expand All @@ -32,7 +33,14 @@ import * as util from 'util';
import { QUERY_RECORD_LIMIT } from './constants';
import { CodeCoverage } from './codeCoverage';
import { HttpRequest } from 'jsforce';
import { elapsedTime } from '../utils/elapsedTime';

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

export class AsyncTests {
public readonly connection: Connection;
Expand Down Expand Up @@ -145,49 +153,34 @@ export class AsyncTests {
public async checkRunStatus(
testRunId: string,
progress?: Progress<ApexTestProgressValue>
): Promise<ApexTestRunResultRecord | undefined> {
): Promise<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;
try {
const testRunSummaryResults = (await this.connection.singleRecordQuery(
peternhale marked this conversation as resolved.
Show resolved Hide resolved
testRunSummaryQuery,
{
tooling: true
}
)) as ApexTestRunResult;

if (testRunSummaryResults.records.length === 0) {
if (finishedStatuses.includes(testRunSummaryResults.Status)) {
return 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 @@ -196,7 +189,7 @@ export class AsyncTests {
* @param asyncRunResult TestQueueItem and RunId for an async run
* @param commandStartTime start time for the async test run
* @param codeCoverage should report code coverages
* @param testRunSummary test run summary
* @param testRunSummary test run summary | undefined
Copy link

Choose a reason for hiding this comment

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

should this also be updated to not take undefined now?

* @param progress progress reporter
* @returns
*/
Expand All @@ -205,7 +198,7 @@ export class AsyncTests {
asyncRunResult: AsyncTestRun,
commandStartTime: number,
codeCoverage = false,
testRunSummary: ApexTestRunResultRecord,
testRunSummary: ApexTestRunResult | undefined,
progress?: Progress<ApexTestProgressValue>
): Promise<TestResult> {
const coveredApexClassIdSet = new Set<string>();
Expand All @@ -215,12 +208,12 @@ export class AsyncTests {
const { apexTestClassIdSet, testResults, globalTests } =
await this.buildAsyncTestResults(apexTestResults);

let outcome = testRunSummary.Status;
let outcome = testRunSummary?.Status ?? 'Unknown';
Copy link

Choose a reason for hiding this comment

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

with this change, the TestResult summary field will be 'Unknown' if the state is not one of Completed states? In that case what is the point of creating the TestResult?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe there is some unknown edge case where this function is being called with a undefined test summary, but has reportable results. By assigning defaults to test summary fields when test summary is undefined instead of throwing allows the user to at least see some results.

Another option would be to allow all output to be created, with the defaults and then throw.

Copy link

@diyer diyer Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this function is being called with a undefined test summary, but has reportable results

This seems to be a bug then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The undefined is one of the states that checkRunStatus can return. The anecdotal use case is that the tests are still in progress, but I have yet to reproduce that case. The streaming client subscribe function is the one that waits for test to complete and checkRunStatus reports on current state.

I found it more reasonable to allow a undefined test summary to be used in reporting results rather than throw an error and lose any aggregated results. One of these commands can run for hours, so a failure ensure the user repeating the command.

Copy link

Choose a reason for hiding this comment

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

what I am confused about though is what a summary result with Unknown and with results mean to the user?

Copy link

Choose a reason for hiding this comment

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

Have we seen cases other than the one mentioned in the issue (where the tests were still in "Processing"), where we have received Undefined and there were results?

if (globalTests.failed > 0) {
outcome = ApexTestRunResultStatus.Failed;
} else if (globalTests.passed === 0) {
outcome = ApexTestRunResultStatus.Skipped;
} else if (testRunSummary.Status === ApexTestRunResultStatus.Completed) {
} else if (testRunSummary?.Status === ApexTestRunResultStatus.Completed) {
mingxuanzhangsfdx marked this conversation as resolved.
Show resolved Hide resolved
outcome = ApexTestRunResultStatus.Passed;
}

Expand All @@ -235,15 +228,17 @@ export class AsyncTests {
passRate: calculatePercentage(globalTests.passed, testResults.length),
failRate: calculatePercentage(globalTests.failed, testResults.length),
skipRate: calculatePercentage(globalTests.skipped, testResults.length),
testStartTime: formatStartTime(testRunSummary.StartTime, 'ISO'),
testExecutionTimeInMs: testRunSummary.TestTime ?? 0,
testTotalTimeInMs: testRunSummary.TestTime ?? 0,
testStartTime: testRunSummary?.StartTime
? formatStartTime(testRunSummary.StartTime, 'ISO')
: '',
testExecutionTimeInMs: testRunSummary?.TestTime ?? 0,
testTotalTimeInMs: testRunSummary?.TestTime ?? 0,
commandTimeInMs: getCurrentTime() - commandStartTime,
hostname: this.connection.instanceUrl,
orgId: this.connection.getAuthInfoFields().orgId,
username: this.connection.getUsername(),
testRunId: asyncRunResult.runId,
userId: testRunSummary.UserId
userId: testRunSummary?.UserId ?? 'Unknown'
},
tests: testResults
};
Expand Down Expand Up @@ -394,6 +389,7 @@ export class AsyncTests {
/**
* Abort test run with test run id
* @param testRunId
* @param progress
*/
public async abortTestRun(
testRunId: string,
Expand Down Expand Up @@ -430,7 +426,7 @@ export class AsyncTests {
private getTestRunRequestAction(
options: AsyncTestConfiguration | AsyncTestArrayConfiguration
): () => Promise<string> {
const requestTestRun = async (): Promise<string> => {
return async (): Promise<string> => {
const url = `${this.connection.tooling._baseUrl()}/runTestsAsynchronous`;
const request: HttpRequest = {
method: 'POST',
Expand All @@ -448,6 +444,5 @@ export class AsyncTests {
return Promise.reject(e);
}
};
return requestTestRun;
}
}
10 changes: 4 additions & 6 deletions src/tests/testService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { AsyncTests } from './asyncTests';
import { SyncTests } from './syncTests';
import { formatTestErrors } from './diagnosticUtil';
import { QueryResult } from '../utils/types';
import { elapsedTime } from '../utils/elapsedTime';
import { elapsedTime } from '../utils';

export class TestService {
private readonly connection: Connection;
Expand Down Expand Up @@ -341,11 +341,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
8 changes: 1 addition & 7 deletions src/tests/types.ts
View file
Open in desktop
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 @@ -278,12 +278,6 @@ export type ApexTestRunResultRecord = {
UserId: string;
};

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

export const enum ApexTestQueueItemStatus {
Holding = 'Holding',
Queued = 'Queued',
Expand Down
Loading
Loading