-
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: guard against undefined test results #354
Conversation
@W-15298950@ Account for a undefined ApexTestRunResult when formatting results
src/tests/asyncTests.ts
Outdated
@@ -215,12 +208,12 @@ export class AsyncTests { | |||
const { apexTestClassIdSet, testResults, globalTests } = | |||
await this.buildAsyncTestResults(apexTestResults); | |||
|
|||
let outcome = testRunSummary.Status; | |||
let outcome = testRunSummary?.Status ?? 'Unknown'; |
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.
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?
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 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.
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 function is being called with a undefined test summary, but has reportable results
This seems to be a bug then?
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.
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.
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 I am confused about though is what a summary result with Unknown and with results mean to 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.
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?
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.
Thanks for the change. I tested the workflow to run async tests and get async test results. I was not able to reproduce the undefined testRunSummary though. But given the unit test I do not see any problem.
src/tests/asyncTests.ts
Outdated
} | ||
|
||
/** | ||
* Format the results of a completed asynchronous test run | ||
* @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 |
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.
should this also be updated to not take undefined now?
src/tests/asyncTests.ts
Outdated
@@ -120,13 +120,13 @@ export class AsyncTests { | |||
await sClient.init(); | |||
await sClient.handshake(); | |||
let queueItem: ApexTestQueueItem; | |||
let testRunSummary = await this.checkRunStatus(testRunId); | |||
let renResult = await this.checkRunStatus(testRunId); |
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.
Typo here
return await this.formatAsyncResults( | ||
asyncRunResult, | ||
getCurrentTime(), | ||
codeCoverage, | ||
testRunSummary, | ||
runResult.testRunSummary, |
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.
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?
src/tests/asyncTests.ts
Outdated
: '', | ||
testExecutionTimeInMs: testRunSummary?.TestTime ?? 0, | ||
testTotalTimeInMs: testRunSummary?.TestTime ?? 0, | ||
testStartTime: formatStartTime(testRunSummary.StartTime, 'ISO'), |
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.
How do we make sure the fields StartTime, TestTime, UserId exist?
…phale/W-15298950-fix-undefined # Conflicts: # src/streaming/streamingClient.ts # src/tests/asyncTests.ts # src/tests/testService.ts # test/tests/asyncTests.test.ts
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 tested it again. run async test works.
* fix: guard against undefined test results @W-15298950@ Account for a undefined ApexTestRunResult when formatting results * chore: apply review suggestions and write test * chore: remove possibility of returning undefined * chore: address review issues * chore: fix typo * chore: merge main
Account for a undefined ApexTestRunResult when formatting results
@W-15298950@ #346
Under certain circumstances an undefined test summary record is passed to function formatAsyncResults. This function assumed that the parameter was always truthy, resulting in an unhandled exception for trying to access the Status (and others) in test summary.
This change implements guards that properly deal with an undefined test summary.