Skip to content
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

Ensure Console.Out is not disposed and is restored if it was replaced during the test run #4317

Merged
merged 1 commit into from Mar 31, 2023

Conversation

normj
Copy link
Contributor

@normj normj commented Mar 23, 2023

I'm on the AWS .NET team. We got a report from a user that used NUnitLite and got an object disposed exception when using this library in their AWS Lambda functions. Lambda is AWS's serverless compute offering.

In Lambda we redirect Console.Out to our own StreamWriter for logging. NUnitLite is wrapping Console.Out with its own ColorConsoleWriter and when it is done it disposes ColorConsoleWriter and disposes its inner TextWriter which would normally be the console stdout that ignores the dispose call but instead calls our TextWriter to dispose. With our TextWriter disposed any calls to Console.Out now fail.

We have fixed Lambda to ignore the dispose call but I though this PR would be helpful for others to ensure NUnitLite doesn't accidently dispose of somebody else's custom Console.Out.

I also made sure if Console.SetOut or Console.SetError are used to be sure to restore to the original TextWriters after the run is complete.

Fixes #4319

@OsirisTerje
Copy link
Member

  1. @normj Would be nice to see a test around this.
  2. @stevenaw @normj Should this also be backported to 3.13 ?

@stevenaw
Copy link
Member

I think this could be helpful for 3.13. Redirecting or splitting these outputs has been a bit of a common request in issues and discussions the last few months.

@OsirisTerje OsirisTerje added the requires:backport Requires backportint to the v3 branch label Mar 26, 2023
@OsirisTerje
Copy link
Member

@normj Would you mind creating another PR for merging into the https://github.com/nunit/nunit/tree/v3.13-dev branch?

@normj normj force-pushed the normj/fix-console-dispose branch from 9f43045 to 3ed73fa Compare March 26, 2023 19:15
@normj
Copy link
Contributor Author

normj commented Mar 26, 2023

🤦‍♂️ Not adding tests on a PR for a test framework

@OsirisTerje I added a test to cover the ColorConsoleWriter. I made sure the test failed without this PRs code changes for ColorConsoleWriter. I'm not sure how to add a test for the change in the Execute method of TestRunner since that is the code executing the tests. Appreciate any suggestions there.

I'm happy to make a make a backporting PR to the v3.13-dev branch once I know I got this PR right.

Copy link
Member

@OsirisTerje OsirisTerje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like everything is in order. Thanks @normj for the contribution!

@OsirisTerje OsirisTerje merged commit 24ad51a into nunit:master Mar 31, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires:backport Requires backportint to the v3 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextRunner accidentally disposes System.Out
3 participants