[Touch.Unit] Preserve console output without retaining Console.Out - #26646
rolfbjarne wants to merge 3 commits into
Conversation
Resolve Console.Out at write time so app-bundle output continues through the platform-visible console writer while NUnit capture is active. This avoids retaining the original synchronized writer, preserving the lock ordering fix for concurrent NUnit and callback output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved writer-retention and duplicate-output paths remain, and focused regression coverage is missing.
Pull request overview
Updates Touch.Unit console routing to avoid retaining the original Console.Out while preserving platform-visible output.
Changes:
- Removes the persistent standard-output writer.
- Adds dynamic console forwarding.
- Adjusts runner output handling.
File summaries
| File | Summary |
|---|---|
tests/common/Touch.Unit/Touch.Client/Runner/TouchRunner.cs |
Replaces retained console forwarding with dynamic forwarding; XML fallback and HTTP duplicate-output paths still require fixes, along with regression coverage. |
Review details
Suppressed comments (3)
tests/common/Touch.Unit/Touch.Client/Runner/TouchRunner.cs:418
⚠️ Testing — This changes the synchronization-sensitive writer used for every test-run message, but there is no regression test for the fix. Please add a focused test that redirectsConsole.Out, writes through the runner writer, changes the redirect, and verifies the later output reaches the new writer without retaining the original; this would catch a future reintroduction of the deadlock/output-loss behavior.
writers.Add (new ConsoleTextWriter ());
tests/common/Touch.Unit/Touch.Client/Runner/TouchRunner.cs:418
- ❌ bug — This removes the captured writer only for the standalone console branch. If XML reporting is enabled and network setup fails but execution continues,
defaultWriterremains null, soNUnitOutputTextWriterabove falls back toConsole.Outand retains the original synchronized writer (and later closes it). That path can still reintroduce the lock cycle and close the platform console; passdefaultWriter ?? new ConsoleTextWriter ()to that writer as well.
writers.Add (new ConsoleTextWriter ());
tests/common/Touch.Unit/Touch.Client/Runner/TouchRunner.cs:418
- ❌ bug — With the HTTP transport and XML disabled,
defaultWriterisHttpTextWriter, whoseWrite/WriteLinemethods already echo every message toConsole.Out(HttpTextWriter.cs:107-124).MultiplexedTextWriternow sends the same message to that writer and toConsoleTextWriter, so runner output is emitted twice (the old independent standard-output stream did not share this path). Avoid adding a second console sink for this transport, or remove the HTTP writer's console echo, and cover the combination with a regression test.
writers.Add (new ConsoleTextWriter ());
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
Use the dynamic console proxy for XML fallback output so NUnit never retains or closes the original Console.Out writer. Remove HttpTextWriter's implicit console echo because the runner now provides the console sink explicitly, preventing duplicated HTTP transport output. Add focused coverage that redirects Console.Out twice and verifies the proxy routes each write to the current writer rather than retaining the first one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eea9570a-646e-4aac-9a5f-b06062484381
There was a problem hiding this comment.
🟡 Changes recommended
The XML-only fallback can duplicate runner output, and targeted coverage for the affected routing paths is still missing.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
tests/common/Touch.Unit/Touch.Client/Runner/TouchRunner.cs:404
- ❌ Bug — When XML is enabled but no transport writer is created (for example, after TCP host selection fails), this proxy is used as
NUnitOutputTextWriter.BaseWriterwhile anotherConsoleTextWriteris added to the multiplexed list at line 418.NUnitOutputTextWriterbuffers the same runner lines inextra_dataand writes them toBaseWriterduringClose, so the console sink emits each line once live and once again inside the XML comment/wrapper. Avoid using the same console sink for both paths, or suppress the live sink for this XML-only fallback.
writers.Add (new NUnitOutputTextWriter (
this, defaultWriter ?? new ConsoleTextWriter (), formatter, options.XmlMode));
tests/common/Touch.Unit/Touch.Client/Runner/TouchRunner.cs:404
⚠️ test_coverage — The new regression only testsConsoleTextWriter.WriteLinein isolation. It does not exercise this XML fallback throughNUnitOutputTextWriter.Close(), which callsBaseWriter.Close()and is the path that previously retained/closed the capturedConsole.Out; a regression in thisdefaultWriter ?? ...wiring would still pass. Add a test for the no-default-writer XML path and verify that closing it leaves the installed console writer usable.
this, defaultWriter ?? new ConsoleTextWriter (), formatter, options.XmlMode));
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
✅ 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 #a433c07] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 2 tests failed, 262 tests passed. Failures❌ monotouch tests (iOS)2 tests failed, 23 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Sonoma (14): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Summary
Restores runner console output on Mac Catalyst app-bundle launches while keeping the lock-order deadlock fix from #26580.
#26580 avoided a deadlock by writing runner output through a
StreamWriteroverConsole.OpenStandardOutput (). That worked around the lock cycle, but in Mac Catalyst app-bundle launches it bypassed the platform-visibleConsole.Outsink, soConsole.WriteLineandConsole.Error.WriteLineoutput disappeared entirely.This PR reverts that implementation and replaces it with
ConsoleTextWriter, a proxy that resolvesConsole.Outon every write. Because it never stores a reference to the writer, output stays visible in the app bundle without retaining the original synchronized writer that participates in NUnit'sConsole.SetOutlock-order cycle.Changes
ConsoleTextWriter: a proxyTextWriterthat looks upConsole.Outat write time instead of capturing it.ConsoleTextWriterwhen there is no network or default writer, soNUnitOutputTextWriterneither retains nor closes the originalConsole.Out.HttpTextWriter: no longer independently echoes toConsole.Out.TouchRunnersupplies the console sink instead, which prevents duplicate output when HTTP transport is enabled without XML.ConsoleTextWriterTests.UsesCurrentConsoleWriter, which redirectsConsole.Outtwice and verifies that each write lands in the writer installed at that moment.Final diff: 6 files, 73 insertions, 12 deletions.
Validation
make clean build run-bare -C tests/introspection/dotnet/MacCatalystcompleted the test run and restored visible output: 1 runner header, 39 PASS lines, and 4 FAIL lines, matching the known baseline. The command exited nonzero only because of two unrelated known introspection failures, which were explicitly ignored for this investigation.Review feedback
The previous Copilot review asked for fixes in three areas — XML fallback retaining
Console.Out, duplicate output over HTTP, and focused regression coverage. All three are addressed above.🤖 Pull request created by Copilot