-
Notifications
You must be signed in to change notification settings - Fork 25
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: modify large queries use query more to fetch all needed records #338
Changes from 3 commits
031be9e
13ad63e
0c8c356
31fe855
710bb90
6a5035f
506b36d
a7c0c14
5d2faab
5028f9f
8e96155
7dfb0fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,11 +28,16 @@ import { | |
TestResult, | ||
TestRunIdResult | ||
} from './types'; | ||
import { calculatePercentage, isValidTestRunID } from './utils'; | ||
import { | ||
calculatePercentage, | ||
isValidTestRunID, | ||
verifyCountQueries | ||
} from './utils'; | ||
import * as util from 'util'; | ||
import { QUERY_RECORD_LIMIT } from './constants'; | ||
import { CodeCoverage } from './codeCoverage'; | ||
import { HttpRequest } from 'jsforce'; | ||
import { OrgConfigProperties } from '@salesforce/core/lib/org/orgConfigProperties'; | ||
|
||
export class AsyncTests { | ||
public readonly connection: Connection; | ||
|
@@ -298,11 +303,13 @@ export class AsyncTests { | |
apexTestResultQuery += | ||
'ApexClass.Id, ApexClass.Name, ApexClass.NamespacePrefix '; | ||
apexTestResultQuery += 'FROM ApexTestResult WHERE QueueItemId IN (%s)'; | ||
|
||
const apexTestResultQueryCount = | ||
'Select count(id) from ApexTestResult where QueueItemId IN (%s)'; | ||
const apexResultIds = testQueueResult.records.map(record => record.Id); | ||
|
||
// iterate thru ids, create query with id, & compare query length to char limit | ||
const queries: string[] = []; | ||
const countQueries: string[] = []; | ||
for (let i = 0; i < apexResultIds.length; i += QUERY_RECORD_LIMIT) { | ||
const recordSet: string[] = apexResultIds | ||
.slice(i, i + QUERY_RECORD_LIMIT) | ||
|
@@ -311,12 +318,29 @@ export class AsyncTests { | |
apexTestResultQuery, | ||
recordSet.join(',') | ||
); | ||
const countQuery: string = util.format( | ||
apexTestResultQueryCount, | ||
recordSet.join(',') | ||
); | ||
countQueries.push(countQuery); | ||
queries.push(query); | ||
} | ||
|
||
const queryPromises = queries.map(query => { | ||
const countQueryPromises = countQueries.map(query => { | ||
return this.connection.singleRecordQuery<{ | ||
expr0: number; | ||
tooling: true; | ||
}>(query); | ||
}); | ||
|
||
const countQueryResult = await Promise.all(countQueryPromises); | ||
|
||
verifyCountQueries(countQueryResult, countQueries); | ||
|
||
const queryPromises = queries.map((query, index) => { | ||
return this.connection.tooling.query<ApexTestResultRecord>(query, { | ||
autoFetch: true | ||
autoFetch: true, | ||
maxFetch: countQueryResult[index].expr0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to add some run time logging , for e.g. found total 'x' records, etc? |
||
}); | ||
}); | ||
const apexTestResults = await Promise.all(queryPromises); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
import { Connection } from '@salesforce/core'; | ||
import { CLASS_ID_PREFIX, TEST_RUN_ID_PREFIX } from './constants'; | ||
import { NamespaceInfo } from './types'; | ||
import { nls } from '../i18n'; | ||
|
||
export function isValidTestRunID(testRunId: string): boolean { | ||
return ( | ||
|
@@ -54,3 +55,23 @@ export async function queryNamespaces( | |
|
||
return [...orgNamespaces, ...installedNamespaces]; | ||
} | ||
|
||
export function verifyCountQueries( | ||
countQueryResult: { expr0: number }[], | ||
countQueries: string[] | ||
) { | ||
// check count query results to ensure that there are results that can be used to set maxFetch | ||
countQueryResult.forEach((result, index) => { | ||
if (!result?.expr0 || result?.expr0 <= 0) { | ||
throw new Error( | ||
nls.localize( | ||
'invalidCountQueryResult', | ||
countQueries[index].substring( | ||
0, | ||
countQueries[index].indexOf('IN (') + 30 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is number 30 here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I brings in the 30 characters following There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this method verifyCountQueries still needed? |
||
) | ||
) | ||
); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a user facing error? what will the user do if one sees this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is customer facing, but unlikely to fail. I imagine the error would become part of an issue report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this message is not needed anymore I believe?