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
Apply test output from timedout tests on non-threadabort platforms #4692
base: master
Are you sure you want to change the base?
Apply test output from timedout tests on non-threadabort platforms #4692
Conversation
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.
Not sure if this will work.
The problem is the test continuing to run and writing output to its context when at the same time the teardown is run.
src/NUnitFramework/framework/Internal/Commands/TimeoutCommand.cs
Outdated
Show resolved
Hide resolved
@manfred-brands I've been able to get back to coding this. I've pushed the naming change to adjust To verify the 3.x functionality I'm trying to restore here, I've put up a branch with the same tests on the 3.x branch. CI is still verifying but they pass as-is locally for me. I'm a little unclear on if there's further changes you'd like to see here, can you please take a look and let me know? |
@manfred-brands I know you've been busy between a few different PRs lately. Could I ask you for a bit of clarification on if you wanted further changes here? I'd been hoping to keep this PR focused to just restoring 3.x behavior but if it also starts discussion about other changes then I'm all for that discussion too. |
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.
@stevenaw So sorry for the delay, but I didn't get to it to pay it the attention needed.
Creating a separateContext with a new CurrentResult
does create a new OutWriter
, but that one will not be used as nothing has access to this from an NUnit tests.
Therefore this Output
will stay empty.
Then the overall result is set to the new (empty) result. This forces you to copy the CurrentContext
output to separateContext
I think the separateContext was created so any code executing after the test timed out won't affect status, but as it is not exposed it doesn't seem to do much.
|
||
context.CurrentResult.CopyOutputTo(separateContext.CurrentResult); |
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.
I still think this is the wrong way around: separateContext
is temporary, only scoped to this command. We want to copy the output from this temporary context to the context
passed in which is the context for the test, not the other way around.
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.
When putting a break point here, the separatedContext.CurrentResult.Output
is empty and the text is in the context.CurrentResult.Output
[SetUp] | ||
public void Setup() | ||
{ | ||
TestContext.WriteLine("setup"); |
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 writes to TestExecutionContext.CurrentContext.OutWriter
The separateContext
in TimedOutCommand
does not change that.
Unfortunately NUnit uses static
fields and sometimes parameters.
} | ||
else | ||
{ | ||
context.CurrentResult = testExecution.GetAwaiter().GetResult(); |
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 is the place where CurrentResult
gets overwritten and any output created by SetUp
output would get lost.
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.
I moved the CopyOutputTo
here to make it clear that this is the relevant location.
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.
The fact that we have both CurrentContext
and a context
parameter is confusing.
|
||
Assert.That(testMethod.Output, Does.Contain("setup")); | ||
Assert.That(testMethod.Output, Does.Contain("method output before pause")); | ||
Assert.That(testMethod.Output, Does.Not.Contain("method output after pause")); |
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.
Note that if you here put a Thread.Sleep(2000)
the test fails (on net core) as the still executing test would have added data to the output.
So the context is not isolated.
var testExecution = RunTestOnSeparateThread(context); | ||
if (Task.WaitAny(new Task[] { testExecution }, _timeout) != -1 | ||
|| _debugger.IsAttached) | ||
var separateContext = new TestExecutionContext(context) |
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.
I think we need to use IsolatedContext
which would already do the CopyOutput
part.
But that relies on setting the TestExecutionContect.CurrentContext
.
How does that work with the context parameter.
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.
Actually no matter what you do here will help. The test will use whatever the current context is at the time, which might be the context of the next test running.
So we can only solve the case where the test doesn't time out.
So either your fix or not creating a separate context at all.
Thanks for the prompt and detailed feedback @manfred-brands , appreciate it! |
Fixes #4598
@manfred-brands You mentioned in the source issue that you may also be looking into this on your own branch.
I've put up this PR as I was able to get this working in a relatively self-contained way for the existing code which may be able to merge either independent of your larger rework or could perhaps be PR'd into it as well. This PR could also be discarded if you've already gotten it working on your end too