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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add XUnitLogChecker to log libraries dumps #93906

Merged
merged 63 commits into from
Nov 15, 2023

Conversation

carlossanlop
Copy link
Member

No description provided.

@carlossanlop carlossanlop added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Oct 24, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 24, 2023
@ghost ghost assigned carlossanlop Oct 24, 2023
src/libraries/sendtohelixhelp.proj Outdated Show resolved Hide resolved
src/libraries/sendtohelixhelp.proj Outdated Show resolved Hide resolved
@carlossanlop carlossanlop changed the title [Draft] Test [Draft] Add XUnitLogChecker to log libraries dumps Oct 25, 2023
@ivdiazsa
Copy link
Member

The XUnitLogChecker enhancements work is going to be tracked here in issue #93988.

@carlossanlop carlossanlop removed the NO-REVIEW Experimental/testing PR, do NOT review it label Oct 25, 2023
@carlossanlop carlossanlop removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 2, 2023
@carlossanlop carlossanlop changed the title [Draft] Add XUnitLogChecker to log libraries dumps Add XUnitLogChecker to log libraries dumps Nov 2, 2023
eng/testing/RunnerTemplate.sh Outdated Show resolved Hide resolved
eng/testing/RunnerTemplate.cmd Outdated Show resolved Hide resolved
eng/testing/RunnerTemplate.cmd Outdated Show resolved Hide resolved
eng/testing/RunnerTemplate.sh Show resolved Hide resolved
@carlossanlop carlossanlop marked this pull request as ready for review November 3, 2023 01:32
@ivdiazsa
Copy link
Member

All the remaining observations I had have already been pointed out in the review. So, it looks good to me now.

@carlossanlop
Copy link
Member Author

@ViktorHofer I addressed your latest comment (remove unused envvar property defined outside of the condition). I only need a sign-off, and remove the temporary change that causes the crash and skips unrelated CIs, then I can merge if the CI is green in the final run.

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - minor q's


<!-- For enabling the use of XUnitLogChecker in coreclr and libraries test runs. -->
<IsXUnitLogCheckerSupported Condition="'$(IsXUnitLogCheckerSupported)' == ''">false</IsXUnitLogCheckerSupported>
<IsXUnitLogCheckerSupported Condition="'$(RuntimeFlavor)' == 'CoreCLR' and '$(TestNativeAot)' != 'true' and '$(TestRunNamePrefixSuffix)' != 'NativeAOT_Release' and '$(TargetOS)' != 'browser' and '$(TargetOS)' != 'wasi' and '$(TargetOS)' != 'ios' and '$(TargetOS)' != 'iossimulator' and '$(TargetOS)' != 'tvos' and '$(TargetOS)' != 'tvossimulator' and '$(TargetOS)' != 'maccatalyst' and '$(TargetOS)' != 'android'">true</IsXUnitLogCheckerSupported>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need both in that case? Also, runtime flavor should remove a lot of the TargetOS ones.

eng/testing/RunnerTemplate.sh Outdated Show resolved Hide resolved
eng/testing/RunnerTemplate.sh Outdated Show resolved Hide resolved
eng/testing/RunnerTemplate.sh Outdated Show resolved Hide resolved
src/libraries/sendtohelixhelp.proj Outdated Show resolved Hide resolved
@carlossanlop carlossanlop removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 15, 2023
@carlossanlop carlossanlop merged commit d561f39 into dotnet:main Nov 15, 2023
186 checks passed
@carlossanlop carlossanlop deleted the LibrariesXUnitLogChecker branch November 15, 2023 04:09
@jkotas
Copy link
Member

jkotas commented Nov 15, 2023

@carlossanlop This broke native AOT outer loop runs.

From https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_apis/build/builds/470488/logs/135

error : Correlation Payload 'D:\a_work\1\s\artifacts\bin\XUnitLogChecker' not found. [D:\a_work\1\s\src\libraries\sendtohelixhelp.proj]

Could you please take a look asap?

@carlossanlop
Copy link
Member Author

carlossanlop commented Nov 15, 2023

@jkotas apologies. I'll revert it, and then resubmit it but making sure I manually run outerloop too.

Here's the PR: #94807

carlossanlop added a commit that referenced this pull request Nov 15, 2023
carlossanlop added a commit that referenced this pull request Nov 15, 2023
carlossanlop added a commit to carlossanlop/runtime that referenced this pull request Nov 16, 2023
Libraries CI failures that generate dumps will now show the dump output in the console log itself.
carlossanlop added a commit to carlossanlop/runtime that referenced this pull request Nov 28, 2023
Libraries CI failures that generate dumps will now show the dump output in the console log itself.
carlossanlop added a commit that referenced this pull request Nov 29, 2023
…OT tests (#94868)

* Add XUnitLogChecker to log libraries dumps (#93906)

Libraries CI failures that generate dumps will now show the dump output in the console log itself.

* Change NativeAOT condition

* Pass the global property set to false in the post build step.

* Address suggestion:
- 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.

* Only add XUnitLogChecker as HelixPayload if its directory is found. Whether it was built or not has already been decided before, and the runner scripts know when to execute it.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants