-
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
Add XUnitLogChecker to log libraries dumps, do not enable for NativeAOT tests #94868
Add XUnitLogChecker to log libraries dumps, do not enable for NativeAOT tests #94868
Conversation
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsSecond attempt of #93906 . That change had to be reverted because NativeAOT tests were broken in outerloop. I added an extra commit that changes the NativeAOT condition per @MichalStrehovsky 's suggestion in this comment.
|
/azp run |
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
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 as long as OuterLoop passes
It failed again, but I see what's happening:
I'm thinking that one option would be to pass the |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
@MichalStrehovsky @hoyosjs The latest commit seems to have worked: The NativeAOT runs hit unrelated test failures, but they are printing the expected message, are not running XUnitLogChecker, and the final returned exit code is the one from the test run itself:
|
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.
This seems likely to break user workflows, to me. Now test scripts have to be passed an additional flag, where they didn't before.
I don't think it's a good idea to introduce a new system to pass extra information about the product to the test. We already do this via customizing the RunScript today. https://github.com/dotnet/runtime/compare/main...agocke:runtime:xunitlogchecker?expand=1 shows an example of how we could encode the environment variable into the script. I think that's simpler and more reliable than the additions in this PR.
Is the issue only that we don't have a workable dotnet.exe? Instead of piping through a flag, could we detect this when we try to run it and emit message to the log that we tried, but we couldn't run XUnitLogChecker? |
Libraries CI failures that generate dumps will now show the dump output in the console log itself.
- Avoid using yml extraHelixArguments. - Add the new embedded SetCommands optional section in runner scripts. - Set __IsXUnitLogCheckerSupported in tests.targets as SetScriptCommand items instead of directly in sendtohelixhelp.proj.
9eda395
to
a147fd5
Compare
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.
This looks directionally right to me, I think there may just be some parameter confusion around the xunitwrapper binaries.
And no YAML changes! Great!
…hether it was built or not has already been decided before, and the runner scripts know when to execute it.
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
I manually triggered runtime-nativeaot-outerloop against my latest commit and it surfaced a build failure that consistently showed up in all the jobs. I don't think my change caused it. Unfortunately, the failure is preventing me from validating my changes, since a failure would show up after building. @dotnet/ilc-contrib any ideas on what the root cause could be? Oh and I opened an issue to see if it could catch other hits besides the ones in my PR: #95367 |
Thanks for reporting it! It's a JIT bug - I think I have a fix in #95383, but it's in the JIT codebase and I'm not a JIT dev so... we'll see. |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks @MichalStrehovsky for the quick fix! The newly triggered run did not show the build error and was able to execute tests. The failures are unrelated to this PR and prove that my change worked and is not blocking nativeaot outerloop anymore: |
Second attempt of #93906 . That change had to be reverted because NativeAOT tests were broken in outerloop.
I added an extra commit that changes the NativeAOT condition per @MichalStrehovsky 's suggestion in this comment.