Fix failing tests on main branch #2
Conversation
1. Removed unsafe .Value call (line 455) - The original code accessed wf.Value before the match to check if wf was Some. This caused crashes when the workspace folder wasn't found. 2. Moved wfPathToUri into the Some wf, Some doc branch (line 510) - The calculation now only happens when we're certain wf is Some. 3. Added .cshtml extension check (line 460) - The Some wf, None branch now only processes files as Razor/cshtml if they actually have a .cshtml extension, instead of treating ALL unfound documents as cshtml files. 4. Safe handling of wf.Solution (lines 461-466) - Instead of the unsafe wf.Solution.Value, the code now pattern matches on wf.Solution to safely handle the case where the solution hasn't loaded yet. 5. Added retry logic for unfound documents (lines 464-465, 504-505) - When a document isn't found (due to timing or solution not being fully loaded), the code now posts PushDiagnosticsDocumentBacklogUpdate and PushDiagnosticsProcessPendingDocuments to rebuild the backlog and retry later. 6. Added continuation for unfound .cshtml documents (line 495) - When a .cshtml document isn't found as a Razor document, processing continues with the next document. All 67 tests pass, including the two previously failing tests: - testPushDiagnosticsWork - testDiagnoseCommandWorks
There was a problem hiding this comment.
Pull request overview
This PR fixes failing tests by improving error handling and adding retry logic for push diagnostics processing. The changes focus on safely handling cases where workspace folders, solutions, or documents are not yet loaded.
Changes:
- Removed unsafe
.Valueproperty access on optional types (workspace folders and solutions) - Added explicit
.cshtmlextension checking to avoid processing non-Razor files as Razor files - Implemented retry logic for documents that cannot be found due to timing or solution loading issues
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match semanticModelMaybe with | ||
| // Only try to process as a .cshtml file if it actually is one | ||
| match workspaceFolderUriToPath wf docUri with | ||
| | Some cshtmlPath when cshtmlPath.EndsWith(".cshtml", StringComparison.OrdinalIgnoreCase) -> |
There was a problem hiding this comment.
Inconsistent extension checking: The code uses cshtmlPath.EndsWith(".cshtml", StringComparison.OrdinalIgnoreCase) here, but elsewhere in the codebase (e.g., Workspace.fs:360, 392), the pattern is to use uri.EndsWith ".cshtml" without the case-insensitive comparison. Consider using a consistent approach throughout the codebase. Note that the check should ideally be on the URI (like line 360) before converting to a path, for consistency.
| match workspaceFolderUriToPath wf docUri with | ||
| | Some cshtmlPath when cshtmlPath.EndsWith(".cshtml", StringComparison.OrdinalIgnoreCase) -> |
There was a problem hiding this comment.
Inefficient ordering of checks: The code converts the URI to a path and then checks if it's a .cshtml file. It would be more efficient to check the URI extension first (like the pattern used in Workspace.fs:360) before attempting the path conversion, since the path conversion might be an expensive operation that could fail.
| | None -> | ||
| Error(Exception "could not GetSemanticModelAsync") | ||
| |> PushDiagnosticsDocumentDiagnosticsResolution | ||
| |> postSelf | ||
|
|
||
| | Some semanticModel -> | ||
| let diagnostics = | ||
| semanticModel.GetDiagnostics() | ||
| |> Seq.map (Diagnostic.fromRoslynDiagnostic (workspaceFolderPathToUri wf)) | ||
| |> Seq.filter (fun (_, uri) -> uri = docUri) | ||
| |> Seq.map fst | ||
| |> Array.ofSeq | ||
|
|
||
| Ok(docUri, None, diagnostics) | ||
| |> PushDiagnosticsDocumentDiagnosticsResolution | ||
| |> postSelf | ||
|
|
||
| | None -> | ||
| // could not find document for this enqueued uri | ||
| // Solution not loaded yet, rebuild backlog and try again later | ||
| postSelf PushDiagnosticsDocumentBacklogUpdate | ||
| postSelf PushDiagnosticsProcessPendingDocuments |
There was a problem hiding this comment.
Potential infinite retry loop: When the solution is not loaded, this code rebuilds the backlog and retries processing. However, if the solution never loads or takes a long time to load, this will cause the same document to be retried indefinitely. Consider adding a retry counter or timeout mechanism to prevent infinite retries. Additionally, since the document was already removed from the backlog at line 452, rebuilding the backlog will re-add it, which may not be the intended behavior.
| | _ -> | ||
| // Not a .cshtml file or couldn't convert URI | ||
| // This can happen if solution hasn't loaded yet - rebuild backlog for retry | ||
| logger.LogDebug( | ||
| "PushDiagnosticsProcessPendingDocuments: could not find document w/ uri \"{docUri}\"", | ||
| "PushDiagnosticsProcessPendingDocuments: could not find document w/ uri \"{docUri}\", will retry", | ||
| string docUri | ||
| ) | ||
|
|
||
| () | ||
| postSelf PushDiagnosticsDocumentBacklogUpdate | ||
| postSelf PushDiagnosticsProcessPendingDocuments |
There was a problem hiding this comment.
Potential infinite retry loop: Similar to the solution not loaded case, when a document is not found, this code rebuilds the entire backlog and retries processing. This can cause the same document to be retried indefinitely if it remains unfound (e.g., if the file was deleted or the URI is invalid). Consider adding a mechanism to track failed documents and avoid retrying them indefinitely, or at least add a maximum retry count.
All 67 tests pass, including the two previously failing tests: