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

Standardize test script naming to CLRTestBa{tc,s}hP{re,ost}Commands #76483

Merged
merged 30 commits into from
Nov 4, 2022

Conversation

markples
Copy link
Member

@markples markples commented Oct 1, 2022

Main change is BashCLRTestPreCommands -> CLRTestBashPreCommands. As part of this I noticed that wasm tests overwrite the Bash precommands, and fixing that breaks them so it isn't touched here. Also, there are several non-Commands variables that start with Ba{tc,s}hTest that aren't touched here.

Remove check on RunCrossGen2 in disasm checks as that is intended as a shell environment variable, not an msbuild variable.

Convert a few more tests to the new environment variable format.

@ghost ghost assigned markples Oct 1, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 1, 2022
@ghost
Copy link

ghost commented Oct 1, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Also converts a few stragglers to the new environment variable format.

NOTE: Includes #76458. Don't review until rebased.

Author: markples
Assignees: markples
Labels:

area-CodeGen-coreclr

Milestone: -

@markples
Copy link
Member Author

Runtime_76219 looks like #69092

@markples markples changed the title Standardize test script naming to CLRTestBa{tc,s}h{Pre,Post}Commands Standardize test script naming to CLRTestBa{tc,s}hP{re,ost}Commands Nov 2, 2022
@markples
Copy link
Member Author

markples commented Nov 2, 2022

Runtime_76219 looks like #69092

This seems to have been caused by converting the environment variables to the new format, which means they aren't discarded by CLRTest.Execute.Bash.targets - undoing that change from here.

@markples
Copy link
Member Author

markples commented Nov 2, 2022

@trylek PTAL - this includes the naming cleanup that I promised back when you reviewed the disasm checks PR

cc @dotnet/jit-contrib - variable rename in tests

@markples markples marked this pull request as ready for review November 2, 2022 09:50
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for cleaning this up! I also apologize for not having responded to your last e-mail regarding test merging, I'll make sure to reply before EOD today.

</PropertyGroup>

<ItemGroup>
<CLRTestBashEnvironmentVariable Include="COMPlus_JitFuncInfoLogFile" Value="SIMD.log" />
Copy link
Member

Choose a reason for hiding this comment

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

Why did you revert this from DOTNET_ to COMPlus_?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this. I think what happened is (1) I started this change before the COMPlus_ rename, (2) it has been open a long time due to it not being my highest priority and test failures (some real, some not), (3) I missed it when I merged the rename, and (4) my re-checks for envvar, complus_, precommands were limited to *proj and missed this file. I checked again and also found some envvars in the new InstrumentedTiers.csproj (which I have less excuse for since I had already renamed the PreCommands variable in it...)

@markples markples merged commit be87e80 into dotnet:main Nov 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2022
@markples markples deleted the test/naming branch December 9, 2022 23:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants