-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix an error printed out when install_location file is missing #56327
Conversation
The app will still run, but we must not print out anything in that case.
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue DetailsThe app will still run, but we must not print out anything in that case. Fixes #56219
|
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.
Thanks!
defaultInstallLocation) | ||
.DotNetRoot(null) | ||
.Execute() | ||
.Should().NotHaveStdErrContaining("The install_location file"); |
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 think we should test the exact output, not just that it doesn't have install_location
, since the problem is that the app is creating extra output at all.
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.
True - but we don't test that anywhere else... so it would be a more "risky" change I guess.
It's definitely something we should test going forward though.
/backport to release/6.0-preview7 |
Started backporting to release/6.0-preview7: https://github.com/dotnet/runtime/actions/runs/1069045335 |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
28105b4
to
50faa41
Compare
Never mind, I didn't understand quite what the test was testing -- the install location here is gone and there's no global runtime installed, so there must be an error, it's just a question of whether the install location file name is included in the message. |
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.
LGTM
The app will still run, but we must not print out anything in that case.
Fixes #56219