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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

.Net: Remove use of Console.SetOut as it impacts other tests #5627

Conversation

markwallace-microsoft
Copy link
Member

Motivation and Context

Calls to Console.SetOut(this._testOutputHelper); impact tests that are using Console during tear down.
Removing these calls as the console output is already being captured in RedirectOutput.

Description

Contribution Checklist

@markwallace-microsoft markwallace-microsoft requested a review from a team as a code owner March 25, 2024 12:32
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Mar 25, 2024
@github-actions github-actions bot changed the title Remove use of Console.SetOut as it impacts other tests .Net: Remove use of Console.SetOut as it impacts other tests Mar 25, 2024
@stephentoub
Copy link
Member

Removing these calls as the console output is already being captured in RedirectOutput.

Maybe I'm misunderstanding, but isn't RedirectOutput just a tee that forwards to both stdout and the ITestOutputHelper? If nothing writes to that RedirectOutput, then it's not actually doing anything, and it seems like the purpose of the Console.SetOut was to ensure that Console.Write calls would end up in both places.

@markwallace-microsoft
Copy link
Member Author

markwallace-microsoft commented Mar 25, 2024

Removing these calls as the console output is already being captured in RedirectOutput.

Maybe I'm misunderstanding, but isn't RedirectOutput just a tee that forwards to both stdout and the ITestOutputHelper? If nothing writes to that RedirectOutput, then it's not actually doing anything, and it seems like the purpose of the Console.SetOut was to ensure that Console.Write calls would end up in both places.

@stephentoub Yes, that is correct. In the tests where I removed the Console.SetOut they weren't testing for the existence of specific values in console output. The tests also weren't resetting the Console.Out at the end of the test which was causing an issue. The initial thought was to fix that problem but resetting the Console.Out but during stand-up we discussed this and decided to remove the unneeded the Console.SetOut calls because this pattern was likely being repeated when not strictly needed. There may be more code I can delete if there are places where the RedirectOutput isn't needed at all, I'll create a follow up PR to this one to fix that.

@stephentoub
Copy link
Member

but during stand-up we discussed this and decided to remove the unneeded the Console.SetOut calls because this pattern was likely being repeated when not strictly needed. There may be more code I can delete if there are places where the RedirectOutput isn't needed at all, I'll create a follow up PR to this one to fix that.

Ok

@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Mar 25, 2024
Merged via the queue into microsoft:main with commit 73b0376 Mar 25, 2024
18 checks passed
@markwallace-microsoft markwallace-microsoft deleted the users/markwallace/remove_console_setout branch March 25, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants