-
Notifications
You must be signed in to change notification settings - Fork 723
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
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
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.
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