-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add arch info in missing framework error #57150
Conversation
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue DetailsAdds the architecture information for missing framework errors.
|
string expectedUrlQuery; | ||
string expectedUrlParameter = null; | ||
string expectedStdErr = null; | ||
int expectedErrorCode = 0; |
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.
Any reason this variable moved down?
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.
There was no particular reason for moving the declaration order I can change it back. The variable was declared but nothing in the test was checking that the actual exit code was the one expected.
src/installer/tests/HostActivation.Tests/PortableAppActivation.cs
Outdated
Show resolved
Hide resolved
.Should().Fail() | ||
.And.HaveStdErrContaining($"- https://aka.ms/dotnet-core-applaunch?{expectedUrlQuery}") | ||
.And.HaveStdErrContaining(expectedStdErr) | ||
.And.ExitWith(expectedErrorCode); |
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 don't think this will work properly on some (Unix) systems where the exit codes are 0-255
string expectedUrlQuery; | ||
string expectedUrlParameter = null; | ||
string expectedStdErr = null; |
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.
Can you also change the name of this test ErrorReportedInDialog
-> ErrorReportedInStdErr
?
While we're here, can we also include the version in the download URL? Something like |
We already put the framework version on the URL, is this something different? A URL example would be: |
Nope that should be good enough. I just wanted to make sure we had some information that the site could use to narrow down the needed runtime. Do we have that for the case where there is no runtime, or only when there's a missing framework? |
Yes. For missing runtime we put the arch, the rid and the apphost version. E.g., |
Adds the architecture information for missing framework errors.