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

Mz/error handling #355

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Mz/error handling #355

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff776dc
chore: add error handling in the bottlenecks
mingxuanzhangsfdx Mar 14, 2024
6dd4ce5
chore: catch exception in runAsyncTest workflow
mingxuanzhangsfdx Mar 21, 2024
6f60eb9
chore: polish error handling
mingxuanzhangsfdx Mar 21, 2024
f3c10e6
chore: remove unnecessary catches
mingxuanzhangsfdx Mar 22, 2024
4b35861
chore: add unit tests and change scope for tested methods
mingxuanzhangsfdx Mar 25, 2024
f069842
chore: stub fs.createWriteStream
mingxuanzhangsfdx Mar 25, 2024
b0b55ff
chore: delete hello
mingxuanzhangsfdx Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions hello
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
world
2 changes: 2 additions & 0 deletions src/i18n/i18n.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ export const messages = {
'Must provide a suite name or suite id to retrieve test classes in suite',
missingSuiteErr: 'Suite does not exist',
missingTestClassErr: 'Apex class %s does not exist in the org',
jsonStringifyErr: `The test result in the format of %s is too large to be stringified. Please try to run fewer tests at a time. Error: %s`,
largeTestResultErr: `The test result in the format of %s is too large to be stored in the heap. Please try to run fewer tests at a time. Error: %s`,
testSuiteMsg: 'Apex test class %s already exists in Apex test suite %s',
classSuiteMsg: `Added Apex class %s to your Apex test suite %s`,
error_no_default_username:
Expand Down
24 changes: 17 additions & 7 deletions src/streaming/streamingClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,16 +276,26 @@ export class StreamingClient {
return null;
}

private async getCompletedTestRun(
public async getCompletedTestRun(
testRunId: string
): Promise<ApexTestQueueItem> {
const queryApexTestQueueItem = `SELECT Id, Status, ApexClassId, TestRunResultId FROM ApexTestQueueItem WHERE ParentJobId = '${testRunId}'`;
const result = await this.conn.tooling.query<ApexTestQueueItemRecord>(
queryApexTestQueueItem,
{
autoFetch: true
}
);
let result;
try {
result = await this.conn.tooling.query<ApexTestQueueItemRecord>(
queryApexTestQueueItem,
{
autoFetch: true
}
);
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', [
'ApexTestQueueItem',
error?.message
])
);
}

if (result.records.length === 0) {
throw new Error(nls.localize('noTestQueueResults', testRunId));
Expand Down
143 changes: 84 additions & 59 deletions src/tests/asyncTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,22 @@ export class AsyncTests {
message: nls.localize('retrievingTestRunSummary')
});

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

Choose a reason for hiding this comment

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

@mingxuanzhangsfdx a couple of issues I see with the strategy of adding try catch on queries.

First, assuming all errors thrown by the query are due to out of memory exception will present the user will a false cause. Maybe the user's network fails some how causing a network exception or some other unexpected issue.

Second, out of memory errors cannot be handled by any means.

Please research approaches to dealing with out of heap space errors and then we can talk about what can be done.

Copy link

@diyer diyer Mar 25, 2024 •
edited

Choose a reason for hiding this comment

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

Agree with Pete. A not so very good approach is to look for specific message in the returned error message but it is ver brittle and can break if the underlying error message returned changes

testRunSummaryResults = (await this.connection.tooling.query(
testRunSummaryQuery,
{
autoFetch: true
}
)) as ApexTestRunResult;
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', [
'ApexTestRunResult',
error?.message
])
);
}

if (testRunSummaryResults.records.length === 0) {
throw new Error(nls.localize('noTestResultSummary', testRunId));
Expand Down Expand Up @@ -314,8 +324,14 @@ export class AsyncTests {
const queryPromises = queries.map((query) => {
return queryAll(this.connection, query, true);
});
const apexTestResults = await Promise.all(queryPromises);
return apexTestResults as ApexTestResult[];
try {
const apexTestResults = await Promise.all(queryPromises);
return apexTestResults as ApexTestResult[];
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', ['ApexTestResult[]', error?.message])
);
}
}

@elapsedTime()
Expand All @@ -336,59 +352,68 @@ export class AsyncTests {
let skipped = 0;

// Iterate over test results, format and add them as results.tests
const testResults: ApexTestResultData[] = [];
for (const result of apexTestResults) {
result.records.forEach((item) => {
switch (item.Outcome) {
case ApexTestResultOutcome.Pass:
passed++;
break;
case ApexTestResultOutcome.Fail:
case ApexTestResultOutcome.CompileFail:
failed++;
break;
case ApexTestResultOutcome.Skip:
skipped++;
break;
}

apexTestClassIdSet.add(item.ApexClass.Id);
// Can only query the FullName field if a single record is returned, so manually build the field
item.ApexClass.FullName = item.ApexClass.NamespacePrefix
? `${item.ApexClass.NamespacePrefix}.${item.ApexClass.Name}`
: item.ApexClass.Name;

const diagnostic =
item.Message || item.StackTrace ? getAsyncDiagnostic(item) : null;

testResults.push({
id: item.Id,
queueItemId: item.QueueItemId,
stackTrace: item.StackTrace,
message: item.Message,
asyncApexJobId: item.AsyncApexJobId,
methodName: item.MethodName,
outcome: item.Outcome,
apexLogId: item.ApexLogId,
apexClass: {
id: item.ApexClass.Id,
name: item.ApexClass.Name,
namespacePrefix: item.ApexClass.NamespacePrefix,
fullName: item.ApexClass.FullName
},
runTime: item.RunTime ?? 0,
testTimestamp: item.TestTimestamp, // TODO: convert timestamp
fullName: `${item.ApexClass.FullName}.${item.MethodName}`,
...(diagnostic ? { diagnostic } : {})
try {
const testResults: ApexTestResultData[] = [];
for (const result of apexTestResults) {
result.records.forEach((item) => {
switch (item.Outcome) {
case ApexTestResultOutcome.Pass:
passed++;
break;
case ApexTestResultOutcome.Fail:
case ApexTestResultOutcome.CompileFail:
failed++;
break;
case ApexTestResultOutcome.Skip:
skipped++;
break;
}

apexTestClassIdSet.add(item.ApexClass.Id);
// Can only query the FullName field if a single record is returned, so manually build the field
item.ApexClass.FullName = item.ApexClass.NamespacePrefix
? `${item.ApexClass.NamespacePrefix}.${item.ApexClass.Name}`
: item.ApexClass.Name;

const diagnostic =
item.Message || item.StackTrace ? getAsyncDiagnostic(item) : null;

testResults.push({
id: item.Id,
queueItemId: item.QueueItemId,
stackTrace: item.StackTrace,
message: item.Message,
asyncApexJobId: item.AsyncApexJobId,
methodName: item.MethodName,
outcome: item.Outcome,
apexLogId: item.ApexLogId,
apexClass: {
id: item.ApexClass.Id,
name: item.ApexClass.Name,
namespacePrefix: item.ApexClass.NamespacePrefix,
fullName: item.ApexClass.FullName
},
runTime: item.RunTime ?? 0,
testTimestamp: item.TestTimestamp, // TODO: convert timestamp
fullName: `${item.ApexClass.FullName}.${item.MethodName}`,
...(diagnostic ? { diagnostic } : {})
});
});
});
}
}

return {
apexTestClassIdSet,
testResults,
globalTests: { passed, failed, skipped }
};
return {
apexTestClassIdSet,
testResults,
globalTests: { passed, failed, skipped }
};
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', [
'ApexTestResultData[]',
error?.message
])
);
}
}

/**
Expand Down
100 changes: 63 additions & 37 deletions src/tests/codeCoverage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import * as util from 'util';
import { calculatePercentage, queryAll } from './utils';
import { QUERY_RECORD_LIMIT } from './constants';
import { elapsedTime } from '../utils/elapsedTime';
import { nls } from '../i18n';

export class CodeCoverage {
public readonly connection: Connection;
Expand Down Expand Up @@ -54,43 +55,60 @@ export class CodeCoverage {
if (apexTestClassSet.size === 0) {
return new Map();
}

const perClassCodeCovResults =
await this.queryPerClassCodeCov(apexTestClassSet);
let perClassCodeCovResults;
try {
perClassCodeCovResults =
await this.queryPerClassCodeCov(apexTestClassSet);
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', [
'ApexCodeCoverage[]',
error?.message
])
);
}

const perClassCoverageMap = new Map<string, PerClassCoverage[]>();

perClassCodeCovResults.forEach((chunk) => {
chunk.records.forEach((item) => {
const totalLines = item.NumLinesCovered + item.NumLinesUncovered;
const percentage = calculatePercentage(
item.NumLinesCovered,
totalLines
);

const value = {
apexClassOrTriggerName: item.ApexClassOrTrigger.Name,
apexClassOrTriggerId: item.ApexClassOrTrigger.Id,
apexTestClassId: item.ApexTestClassId,
apexTestMethodName: item.TestMethodName,
numLinesCovered: item.NumLinesCovered,
numLinesUncovered: item.NumLinesUncovered,
percentage,
...(item.Coverage ? { coverage: item.Coverage } : {})
};
const key = `${item.ApexTestClassId}-${item.TestMethodName}`;
if (perClassCoverageMap.get(key)) {
perClassCoverageMap.get(key).push(value);
} else {
perClassCoverageMap.set(
`${item.ApexTestClassId}-${item.TestMethodName}`,
[value]
try {
perClassCodeCovResults.forEach((chunk) => {
chunk.records.forEach((item) => {
const totalLines = item.NumLinesCovered + item.NumLinesUncovered;
const percentage = calculatePercentage(
item.NumLinesCovered,
totalLines
);
}
});
});

return perClassCoverageMap;
const value = {
apexClassOrTriggerName: item.ApexClassOrTrigger.Name,
apexClassOrTriggerId: item.ApexClassOrTrigger.Id,
apexTestClassId: item.ApexTestClassId,
apexTestMethodName: item.TestMethodName,
numLinesCovered: item.NumLinesCovered,
numLinesUncovered: item.NumLinesUncovered,
percentage,
...(item.Coverage ? { coverage: item.Coverage } : {})
};
const key = `${item.ApexTestClassId}-${item.TestMethodName}`;
if (perClassCoverageMap.get(key)) {
perClassCoverageMap.get(key).push(value);
} else {
perClassCoverageMap.set(
`${item.ApexTestClassId}-${item.TestMethodName}`,
[value]
);
}
});
});
return perClassCoverageMap;
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', [
'Map<string, PerClassCoverage[]>',
error?.message
])
);
}
}

/**
Expand All @@ -107,9 +125,17 @@ export class CodeCoverage {
if (apexClassIdSet.size === 0) {
return { codeCoverageResults: [], totalLines: 0, coveredLines: 0 };
}

const codeCoverageAggregates =
await this.queryAggregateCodeCov(apexClassIdSet);
let codeCoverageAggregates;
try {
codeCoverageAggregates = await this.queryAggregateCodeCov(apexClassIdSet);
} catch (error) {
throw new Error(
nls.localize('largeTestResultErr', [
'ApexCodeCoverageAggregate[]',
error?.message
])
);
}

let totalLinesCovered = 0;
let totalLinesUncovered = 0;
Expand Down Expand Up @@ -153,7 +179,7 @@ export class CodeCoverage {
}

@elapsedTime()
private async queryPerClassCodeCov(
public async queryPerClassCodeCov(
Copy link
Collaborator

Choose a reason for hiding this comment

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

What change to public?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the function is stubbed here in the unit test in order to mock the exception.

apexTestClassSet: Set<string>
): Promise<ApexCodeCoverage[]> {
const perClassCodeCovQuery =
Expand All @@ -162,7 +188,7 @@ export class CodeCoverage {
}

@elapsedTime()
private async queryAggregateCodeCov(
public async queryAggregateCodeCov(
apexClassIdSet: Set<string>
): Promise<ApexCodeCoverageAggregate[]> {
const codeCoverageQuery =
Expand Down