-
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
Conversation
@W-11622957@ fix apex test result query to set use actual result size
Fix broken tests
src/i18n/i18n.ts
Outdated
@@ -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', | |||
invalidCountQueryResult: 'Invalid query result for count query ("%s")' |
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?
src/tests/asyncTests.ts
Outdated
@@ -5,7 +5,7 @@ | |||
* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause | |||
*/ | |||
|
|||
import { ConfigAggregator, Connection } from '@salesforce/core'; | |||
import { Connection } from '@salesforce/core'; |
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.
newbie question: what is the ConfigAggregator? what is it different from using it vs not using it?
src/tests/utils.ts
Outdated
'invalidCountQueryResult', | ||
countQueries[index].substring( | ||
0, | ||
countQueries[index].indexOf('IN (') + 30 |
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.
what is number 30 here?
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.
I brings in the 30 characters following IN (
so the user will have a bit of the set if ides being fetched. With out some sort of clipping, the message can be very large, up to ~10.5K
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 method verifyCountQueries still needed?
src/tests/asyncTests.ts
Outdated
maxFetch: | ||
countQueryResult[index]?.expr0 ?? | ||
config.getPropertyValue(OrgConfigProperties.ORG_MAX_QUERY_LIMIT) | ||
maxFetch: countQueryResult[index].expr0 |
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 it possible to add some run time logging , for e.g. found total 'x' records, etc?
current version of plugin-apex is using 5.3.20
…phale/W-11622957-query-size # Conflicts: # package.json # src/tests/asyncTests.ts # yarn.lock
|
||
return { | ||
done: true, | ||
totalSize: allRecords.length, |
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.
can we log these details (totalSize) during the run?
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.
@diyer do you mean so they can be seen in telemetry or just in the typical local log files for the user?
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.
log files for the user
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.
Code changes all look good.
All test results and code coverage results are present when running with the CLI ✅
Pulled in the dependency into the extensions and behavior remains unchanged ✅
What does this PR do?
modify large result queries to set maxFetch to the size of number of expected records
What issues does this PR fix or reference?
@W-11622957@
@W-14458776@
This fix should prevent the loss of test and coverage result records.