Ignore DefaultTimeout in all but .NET Framework builds#5140
Ignore DefaultTimeout in all but .NET Framework builds#5140OsirisTerje merged 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes how DefaultTimeout is handled so it is only applied in .NET Framework builds, avoiding failures/behavior changes on modern .NET where thread-abort-based timeouts are unsupported (addressing #4658 and #5133).
Changes:
- Gate applying
FrameworkPackageSettings.DefaultTimeoutbehind#if NETFRAMEWORKin the framework runner. - Gate emitting
DefaultTimeoutinto NUnitLite run settings behind#if NETFRAMEWORKand show a warning in non-NETFRAMEWORK builds. - Update runner-setting parsing tests to only expect
DefaultTimeoutvalidation behavior on .NET Framework.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/NUnitFramework/tests/Api/TestAssemblyRunnerTests.cs | Adjusts which settings are validated as parseable/unparseable depending on NETFRAMEWORK. |
| src/NUnitFramework/nunitlite/TextUI.cs | Warns that DefaultTimeout is ignored outside .NET Framework builds. |
| src/NUnitFramework/nunitlite/TextRunner.cs | Prevents DefaultTimeout from being added to run settings outside .NET Framework builds. |
| src/NUnitFramework/framework/Api/NUnitTestAssemblyRunner.cs | Prevents DefaultTimeout from being applied to TestExecutionContext outside .NET Framework builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Apply package settings to the context | ||
| #if NETFRAMEWORK | ||
| if (Settings.TryGetValue(FrameworkPackageSettings.DefaultTimeout, out var timeout)) | ||
| context.TestCaseTimeout = ConvertSetting<int>(timeout); | ||
| #endif |
There was a problem hiding this comment.
With DefaultTimeout now ignored on non-NETFRAMEWORK, it would be good to add a regression test proving that providing the FrameworkPackageSettings.DefaultTimeout setting no longer causes runs to error out on platforms without THREAD_ABORT (the issue being fixed). Currently the updated tests only cover parsing/throwing on NETFRAMEWORK, but don’t assert the new non-framework behavior (i.e., the setting is safely ignored).
|
Awesome! I have also removed the same setting from the adapter for non-netframework, and updated docs for the adapter. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes #4658
Fixes #5133