Apply test output from timedout tests on non-threadabort platforms #4692
Conversation
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.
@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.
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.
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.
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.
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.
I moved the CopyOutputTo here to make it clear that this is the relevant location.
|
|
||
| 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.
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.
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.
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