-
Notifications
You must be signed in to change notification settings - Fork 107
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
Some QoL improvements for diag tests #1204
Conversation
asklar
commented
Jun 15, 2022
•
edited
Loading
edited
- Fixes DiagnosticTests! There were a few non-passing tests so I debugged into it and saw that we're failing because in some codepaths we didn't have an assembly name or version. Passing these requires a context options object.
- DiagnosticTests can now pass a context with options, so we can remove special casing we had for cswinrt generator when running inside a test host
- When tests failed, the inner exception was not being captured, so the error just said "...But found:" and no additional information about the exception in the generator was provided
- Setting ExitCode upon error, which should cause builds to fail when the generator fails to produce. Fixes Build succeeds even when generator fails #1202
- Fixes some typos
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 @asklar for these changes.
@@ -154,6 +152,7 @@ public void Generate() | |||
{ | |||
Logger.Log("Exiting early -- found errors in authored runtime component."); | |||
Logger.Close(); | |||
Environment.ExitCode = -1; |
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.
Didn't realize we can do this, does this make the build fail out and how does it look like when it does?
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.
Looks good to me, just needs some feedback on the comments
7b13929
to
e98230b
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |