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
|
Thanks for the prompt and detailed feedback @manfred-brands , appreciate it! |
|
@manfred-brands I've read through your feedback. I'm not 100% sure, but were there further changes you wanted to see here? Reminder that, when combined with your own PR #4673, this code will primarily only serve .NET6 which will be an increasingly small base over time |
|
@stevenaw I think we can have leave it as is. It solves the problem when test with a timeout succeed in time. Regarding #4673 I have to sort out the deadlock in the CI build under linux, but have been to busy with work to dive into this, hopefully this weekend. |
|
Thanks for the confirmation @manfred-brands ! Appreciate your time and review here.
I completely understand, I'm also pretty busy in this realm at the moment too. |
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