Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent modal popups during managed and native test runs by enforcing “test mode” assertion behavior, and by ensuring assertion/exception details are visible in console output so CI fails fast and diagnosably.
Changes:
- Add
FW_TEST_MODE=1to managed/native test runners and runsettings to bypass assertion dialog behavior (including registry overrides). - Introduce NUnit assembly-level attributes to suppress assert dialogs and log last-chance exceptions to console for projects that don’t use the shared bootstrap.
- Update native test project includes to build with the DebugProcs dependency and disable assertion dialogs in
testGeneric.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test.ps1 | Sets FW_TEST_MODE for managed test runs outside .runsettings. |
| Test.runsettings | Adds FW_TEST_MODE to test environment variables. |
| Src/InstallValidator/InstallerArtifactsTests/TestAssemblyInfo.cs | Adds assembly-level unhandled-exception logging for installer artifact tests. |
| Src/InstallValidator/InstallerArtifactsTests/InstallerArtifactsTests.csproj | Links AppForTests.config and compiles the logging attribute for tests opting out of shared bootstrap. |
| Src/InstallValidator/InstallValidatorTests/TestAssemblyInfo.cs | Adds assembly-level unhandled-exception logging for installer validator tests. |
| Src/InstallValidator/InstallValidatorTests/InstallValidatorTests.csproj | Compiles the logging attribute for tests opting out of shared bootstrap. |
| Src/Generic/Test/testGeneric.cpp | Disables native assert dialogs during global test setup. |
| Src/Generic/Test/TestGeneric.vcxproj | Fixes include paths for DebugProcs dependency. |
| Src/DebugProcs/DebugProcs.cpp | Adds FW_TEST_MODE override for assert-dialog behavior; mirrors asserts to stderr. |
| Src/Common/FwUtils/FwUtilsTests/Attributes/SuppressAssertDialogsAttribute.cs | New NUnit attribute to suppress assert UI and mirror trace/assert output to console. |
| Src/Common/FwUtils/FwUtilsTests/Attributes/LogUnhandledExceptionsAttribute.cs | New NUnit attribute to log last-chance exceptions to console. |
| Src/Common/FwUtils/FwUtilsTests/Attributes/HandleApplicationThreadExceptionAttribute.cs | Writes WinForms thread exceptions to console before rethrowing. |
| Src/AssemblyInfoForUiIndependentTests.cs | Enables last-chance exception logging for UI-independent test assemblies. |
| Src/AssemblyInfoForTests.cs | Enables last-chance exception logging and assert-dialog suppression for most test assemblies. |
| Src/AppForTests.config | Adds ConsoleTraceListener to mirror trace output to console. |
| Build/scripts/Invoke-CppTest.ps1 | Sets FW_TEST_MODE for native test runner execution. |
| m_listener = new ConsoleErrorTraceListener(throwOnFail: !hasEnvVarListener); | ||
| Trace.Listeners.Insert(0, m_listener); | ||
| } |
There was a problem hiding this comment.
BeforeTest unconditionally inserts a new ConsoleErrorTraceListener into Trace.Listeners, which can result in multiple listeners (duplicate output and extra overhead) if BeforeTest runs more than once per process or if another console listener is already configured (e.g., via AppForTests.config). Add a guard to avoid inserting a duplicate listener (or reuse a static singleton) and consider skipping when an existing console/EnvVar listener already provides the desired output.
| public override void Write(string message) | ||
| { | ||
| Console.Error.Write(message); | ||
| Console.Error.Flush(); | ||
| } | ||
|
|
||
| public override void WriteLine(string message) | ||
| { | ||
| Console.Error.WriteLine(message); | ||
| Console.Error.Flush(); | ||
| } |
There was a problem hiding this comment.
ConsoleErrorTraceListener mirrors all Trace output to Console.Error and flushes on every write. In large test runs this can significantly slow execution and flood CI logs, even when there are no assertion failures. Consider limiting console output to Fail/assert paths (or buffering/conditional flushing) so only actionable failure details are emitted by default.
| // Escalate instead of allowing the exception to be quietly ignored at finalization time. | ||
| throw new UnobservedTaskExceptionLoggedException(e.Exception); |
There was a problem hiding this comment.
The XML comment says this attribute is “logging-only”, but OnUnobservedTaskException throws a new exception from the TaskScheduler.UnobservedTaskException event handler. Exceptions thrown on the finalizer thread can terminate the process unpredictably and may not surface as a clean test failure. Consider logging and then using a deterministic fail-fast mechanism (or mark the exception observed and report it through the test framework in a controlled way) rather than throwing from this event.
| // Escalate instead of allowing the exception to be quietly ignored at finalization time. | |
| throw new UnobservedTaskExceptionLoggedException(e.Exception); | |
| // Mark the exception as observed so it is not rethrown on the finalizer thread. | |
| e.SetObserved(); |
| // Always mirror assertion text to the process error stream so test runners, | ||
| // humans, and automation can see the failure details without a debugger. | ||
| fprintf(stderr, "%s\n", assertbuf); | ||
| fflush(stderr); | ||
|
|
There was a problem hiding this comment.
SilAssert now always writes the full assertion text to stderr. This affects all consumers of DebugProcs.dll (not just tests) and may create noisy output/perf overhead for interactive apps. If the intent is test-only visibility, consider gating the fprintf/fflush on FW_TEST_MODE (or a similar opt-in env var) so normal runs aren’t impacted.
| <trace autoflush="false" indentsize="4"> | ||
| <listeners> | ||
| <clear/> | ||
| <add name="ConsoleTraceListener" type="System.Diagnostics.ConsoleTraceListener"/> |
There was a problem hiding this comment.
Adding ConsoleTraceListener will mirror all System.Diagnostics.Trace output to stdout for every test using this config, which can significantly increase log volume. Given this PR also installs a console trace listener via SuppressAssertDialogsAttribute, consider whether you need both (and whether you can restrict console output to assertion/failure details only).
| <add name="ConsoleTraceListener" type="System.Diagnostics.ConsoleTraceListener"/> |
| // Force environment variables that control native (DebugProcs.dll) and | ||
| // managed (EnvVarTraceListener) assertion behavior. | ||
| Environment.SetEnvironmentVariable("AssertUiEnabled", "false"); | ||
| Environment.SetEnvironmentVariable("AssertExceptionEnabled", "true"); | ||
| Environment.SetEnvironmentVariable("FW_TEST_MODE", "1"); | ||
|
|
There was a problem hiding this comment.
SuppressAssertDialogsAttribute sets process-wide environment variables in BeforeTest but never restores the previous values. This can leak into subsequent test assemblies that run in the same testhost process and make failures harder to diagnose. Consider capturing the prior values (AssertUiEnabled/AssertExceptionEnabled/FW_TEST_MODE) and restoring them in AfterTest.
Summary
Validation
./build.ps1./test.ps1./test.ps1 -NativeNotes
testGenericLib.exeandTestViews.exeThis change is