-
Notifications
You must be signed in to change notification settings - Fork 728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Keep output writer when Timeout on modern .NET #4646
Conversation
@@ -121,8 +121,9 @@ private Task<TestResult> RunTestOnSeparateThread(TestExecutionContext context) | |||
{ | |||
var separateContext = new TestExecutionContext(context) | |||
{ | |||
CurrentResult = context.CurrentTest.MakeTestResult() | |||
//CurrentResult = context.CurrentTest.MakeTestResult() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resets the OutWriter
as the new test result will create it's own writer, which when assigned to separateContext.CurrentResult
will then apply it to the separateContext
's outwriter based on
nunit/src/NUnitFramework/framework/Internal/TestExecutionContext.cs
Lines 208 to 217 in 321db1a
public TestResult CurrentResult | |
{ | |
get => _currentResult; | |
set | |
{ | |
_currentResult = value; | |
if (value is not null) | |
OutWriter = value.OutWriter; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manfred-brands I'd like to pick up fixing this issue, but I'm looking for advice here. Do you recall why we create a new CurrentResult
in the TimeoutCommand
?
If we need it then I think the way to fix the issue here is to pass in context.OutWriter
when creating the new TestResult
in order to avoid creating new one on initialization. That's a bit involved to do in a non-breaking way so I wanted to double-check first about why the existing code does what it does
24dd177
to
83d9963
Compare
Having explored a bit more, I'm feeling fairly confident that we can delete the line, though I'd still welcome feedback on the approach. For now I'm going to proceed and try to get a test written for this |
Looking at this closer, I believe it is needed for the non-thread abort fallback to work. It prevents the result from being modified by the background test completing after the timeout, thus modifying the result after it's been returned. I'm going to close this now that I better understand the issue. I'll open a new PR once I have something finalized. |
Relates to #4598
Initial POC to try to get output printing when combining
Timeout
withTeardown