Include console links for Azure DevOps test failures#17489
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: premun <7013027+premun@users.noreply.github.com>
Co-authored-by: premun <7013027+premun@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new AzDO-only failure reporting path can label a Helix job-details URL as “Console:” when the work-item console link is missing, which should be corrected for clarity/accuracy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| entry.Key.WorkItemName, | ||
| "Failed (AzDO tests)", | ||
| "see Azure DevOps test run results"); | ||
| GetConsoleOutputText(consoleOutputUri ?? job.DetailsUri)); |
There was a problem hiding this comment.
🟡 Changes recommended
A race described in existing comments can leave the final “Failed (AzDO tests)” console link stale because reconciliation doesn’t refresh the already-recorded failure entry when ConsoleOutputUri becomes available later.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| var key = (chainKey, wi.Name); | ||
| if (string.IsNullOrEmpty(wi.ConsoleOutputUri)) | ||
| { | ||
| _workItemConsoleOutputs.Remove(key); | ||
| } | ||
| else | ||
| { | ||
| _workItemConsoleOutputs[key] = wi.ConsoleOutputUri; | ||
| } | ||
| bool passed = !wi.IsFailed && !_failedTestWorkItems.Contains(key); |
There was a problem hiding this comment.
@copilot the test upload does not start before the work item is finished and finishing means it will already have the console log URI. So does this race still exist?
There was a problem hiding this comment.
The race can still exist, just not at the granularity you described. It's true that a work item can't upload until it's finished, and a finished work item's summary already carries ConsoleOutputUri. But TryRecordWorkItemOutcomes (which populates the _workItemConsoleOutputs cache) is only invoked once the whole Helix job is reconciled — and uploads/ObserveTestResults are triggered per work item as soon as that item is terminal, even while sibling work items in the same job are still running (see the "Upload terminal work items as soon as they are visible" comment in JobMonitorRunner). So for multi-work-item jobs, an item can finish, upload, and get recorded as an AzDO-test failure before the job as a whole (and thus the cache) is reconciled — leaving the entry with a stale/missing link even after the cache is later populated, since TryRecordWorkItemOutcomes never revisited it (wi.IsFailed was false and passed was false).
Fixed in 8be3a95: TryRecordWorkItemOutcomes now refreshes an already-recorded AzDO-test failure entry with the work item's console link once it becomes known during reconciliation. Added ObserveTestResults_BeforeJobReconciled_ConsoleLinkRefreshedOnReconcile to cover it.
Co-authored-by: premun <7013027+premun@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new console-URI cache behavior has a mismatch with the PR’s “latest non-empty” requirement and can associate stale console links across job incarnations in pre-reconcile scenarios.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Microsoft.DotNet.Helix/JobMonitor/MonitorState.cs:305
- This clears the cached console URI when the reconciled work item has an empty
ConsoleOutputUri, but the PR description calls for caching each work item’s latest non-empty console URI. Removing the entry here drops the last known good link and can regress the final failure output back to job details/no link even if a non-empty URI was previously observed.
if (string.IsNullOrEmpty(wi.ConsoleOutputUri))
{
_workItemConsoleOutputs.Remove(key);
}
else
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| _workItemConsoleOutputs.TryGetValue(key, out string consoleOutputUri); | ||
| _failedWorkItemConsoleInfo[key] = new FailedWorkItemConsoleInfo( | ||
| job.DisplayName, | ||
| entry.Key.WorkItemName, | ||
| "Failed (AzDO tests)", |
Azure DevOps-only test failures now retain and report the related Helix console output location alongside the existing test-results reference.
To double check:
Cache each reconciled work item’s latest non-empty console URI.
Fall back to Helix job details when no work-item console URI is available.
Cover final failure output containing both links.
The right tests are in and the right validation has happened. Guidance: https://github.com/dotnet/arcade/blob/main/Documentation/Validation.md