[xharness] Add TestReporter implementation to detect crashes properly.#25150
[xharness] Add TestReporter implementation to detect crashes properly.#25150rolfbjarne merged 1 commit intomainfrom
Conversation
xharness regressed detecting crashing tests as crashing (dotnet/xharness@a6f9fe6), so clone the TestReporter class and modify it to properly detect crashes and report them as such. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: 2 pipeline(s) require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds a local ITestReporter implementation to tests/xharness to restore correct crash detection/reporting for test runs (addressing a regression in upstream xharness crash classification).
Changes:
- Introduce
TestReporterimplementation used to parse results, capture crash artifacts, and generate XML failure output. - Add
TestReporterFactoryto create the reporter with the required dependencies. - Wire both files into
tests/xharness/xharness.csproj.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| tests/xharness/xharness.csproj | Includes the new TestReporter + TestReporterFactory sources in the xharness build. |
| tests/xharness/TestReporterFactory.cs | Factory that constructs the new reporter implementation. |
| tests/xharness/TestReporter.cs | New reporter logic for determining success/crash/timeout and generating result artifacts. |
| if (!int.TryParse (pidstr, out pid)) | ||
| mainLog.WriteLine ("Could not parse pid: {0}", pidstr); | ||
| } else if (line.Contains ("Xamarin.Hosting: Launched ") && line.Contains (" with pid ")) { | ||
| var pidstr = line.Substring (line.LastIndexOf (' ')); |
There was a problem hiding this comment.
GetPidFromRunLog extracts the PID using line.Substring (line.LastIndexOf (' ')), which includes the leading space before the PID. int.TryParse will then fail for lines like "... with pid 123" and the PID will remain -1, breaking the later kill/crash-detection logic. Trim the substring or start after the last space (LastIndexOf (' ') + 1).
| var pidstr = line.Substring (line.LastIndexOf (' ')); | |
| var pidstr = line.Substring (line.LastIndexOf (' ') + 1); |
| while ((line = await logReader.ReadLineAsync ()) is not null) { | ||
| const string str = "was launched with pid '"; | ||
| var idx = line.IndexOf (str, StringComparison.Ordinal); | ||
| if (idx > 0) { |
There was a problem hiding this comment.
GetPidFromMainLog only parses when idx > 0, so it will miss cases where the marker string starts at index 0. Using idx >= 0 makes PID detection more robust across different log formatting.
| if (idx > 0) { | |
| if (idx >= 0) { |
| var crashed = false; | ||
| if (File.Exists (listener.TestLog.FullPath)) { | ||
| WrenchLog.WriteLine ("AddFile: {0}", listener.TestLog.FullPath); | ||
| (Success, crashed, result.ResultMessage) = await TestsSucceeded (appInfo, listener.TestLog.FullPath, timedout); |
There was a problem hiding this comment.
ParseResult sets Success from TestsSucceeded whenever a test log exists. This can override an earlier process-level failure detected in Collect*Result (e.g., app crashed after writing results) and end up reporting success. Consider merging the process outcome with the parsed test outcome instead of overwriting it.
| (Success, crashed, result.ResultMessage) = await TestsSucceeded (appInfo, listener.TestLog.FullPath, timedout); | |
| var previousSuccess = Success; | |
| var (parsedSuccess, parsedCrashed, parsedResultMessage) = await TestsSucceeded (appInfo, listener.TestLog.FullPath, timedout); | |
| Success = (previousSuccess ?? true) && parsedSuccess; | |
| crashed = parsedCrashed; | |
| result.ResultMessage = parsedResultMessage; |
| if (crashReason == "per-process-limit") { | ||
| result.ResultMessage = "Killed due to using too much memory (per-process-limit)."; | ||
| } else { | ||
| result.ResultMessage = $"Killed by the OS ({crashReason})"; | ||
| } |
There was a problem hiding this comment.
When a crashReason is found, ResultMessage is updated but the final ExecutingResult isn't recomputed. This can leave the run classified as Failed instead of Crashed even though an OS kill reason was detected. Set crashed = true (and adjust ExecutingResult) when crashReason is present.
| } | ||
|
|
||
| public void Dispose () | ||
| { |
There was a problem hiding this comment.
TestReporter.Dispose disposes crashLogs but not the CancellationTokenSource. CancellationTokenSource implements IDisposable and should be disposed to avoid leaking any underlying resources/timers if many reporters are created over time.
| { | |
| { | |
| cancellationTokenSource.Dispose (); |
| Console.WriteLine (node?.InnerXml); | ||
| Console.WriteLine (node?.SelectSingleNode ("reason")?.InnerText); |
There was a problem hiding this comment.
Avoid writing crash parsing details to stdout here (Console.WriteLine). This bypasses the harness log plumbing and can add noisy output in CI. Prefer mainLog.WriteLine (optionally behind verbosity) or remove.
| Console.WriteLine (node?.InnerXml); | |
| Console.WriteLine (node?.SelectSingleNode ("reason")?.InnerText); | |
| mainLog.WriteLine (node?.InnerXml); | |
| mainLog.WriteLine (node?.SelectSingleNode ("reason")?.InnerText); |
| Console.WriteLine (line); | ||
| resultLine = line; | ||
| break; | ||
| } else if (line.Contains ("[FAIL]")) { | ||
| Console.WriteLine (line); |
There was a problem hiding this comment.
Avoid writing raw test output lines to stdout (Console.WriteLine) during result scanning. This can spam CI logs for large test suites; route through mainLog (possibly at verbose level) instead.
| Console.WriteLine (line); | |
| resultLine = line; | |
| break; | |
| } else if (line.Contains ("[FAIL]")) { | |
| Console.WriteLine (line); | |
| mainLog.WriteLine (line); | |
| resultLine = line; | |
| break; | |
| } else if (line.Contains ("[FAIL]")) { | |
| mainLog.WriteLine (line); |
✅ [PR Build #c41f42e] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #c41f42e] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #c41f42e] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #c41f42e] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
xharness regressed detecting crashing tests as crashing (dotnet/xharness@a6f9fe6),
so clone the TestReporter class and modify it to properly detect crashes and report
them as such.