Skip to content

Update (remove most) most of CLRTestKind, GenerateRunScript, RestorePackages#89696

Merged
trylek merged 4 commits intodotnet:mainfrom
markples:prop_update
Aug 22, 2023
Merged

Update (remove most) most of CLRTestKind, GenerateRunScript, RestorePackages#89696
trylek merged 4 commits intodotnet:mainfrom
markples:prop_update

Conversation

@markples
Copy link
Contributor

@markples markples commented Jul 30, 2023 •
edited
Loading

I came across these while looking at the set of properties in order to generate some error checking for merged groups. As best as I can tell, most of these are very old uses that likely have had the test system changed from under them. They cause confusion when encountered.

  • RestorePackages - I think has been replaced by the logic that bulk restores.
  • CLRTestKind - A (somewhat) more recent change was Switch over Loader/classloader/TypeGeneratorTests to use merged wrappers #61942, which changed the default for CLRTestKind from "BuildAndRun" to "BuildAndRun if exe, else BuildOnly". However, BuildOnly appears to be intended for exes - build the exe but not test infrastructure to execute it as another project will handle that. I think SharedLibrary is what was intended here. Many OutputType=library tests specify BuildOnly or SharedLibrary, which seem incorrect and (now) unnecessary.
  • GenerateRunScript - This is unconditionally set (overriding individual project files) in src/tests/Directory.Build.targets.

@ghost ghost assigned markples Jul 30, 2023
@ghost
Copy link

ghost commented Jul 30, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: markples
Assignees: markples
Labels:

area-System.Reflection.Metadata
Milestone: -

@markples markples changed the title redo props Update (remove most) most of CLRTestKind, GenerateRunScript, RestorePackages Jul 31, 2023
@markples
Copy link
Contributor Author

markples commented Jul 31, 2023

@trylek @jkoritzinsky This is fallout from staring at msbuild properties too long for merged groups.

cc @dotnet/jit-contrib @Maoni0 @cshung @mrsharm

I'm a bit concerned that the checkin gate doesn't provide complete coverage so would welcome additional tests to run.

@markples markples marked this pull request as ready for review July 31, 2023 07:27
@markples
Copy link
Contributor Author

markples commented Jul 31, 2023

/azp run runtime-coreclr outerloop, runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 31, 2023

Azure Pipelines successfully started running 2 pipeline(s).

@markples
Copy link
Contributor Author

markples commented Jul 31, 2023

One little static check on the contents of the diff:

C:\r\runtime4\src\tests>git diff dotnet/main prop_update | findstr "^+ ^-" | findstr /v "^+++ ^---" | sort | uniq
-
-    <CLRTestKind Condition="'$(CLRTestKind)' == ''">BuildOnly</CLRTestKind>
-    <CLRTestKind>BuildAndRun</CLRTestKind>
-    <CLRTestKind>BuildOnly</CLRTestKind>
-    <CLRTestKind>SharedLibrary</CLRTestKind>
-    <GenerateRunScript>false</GenerateRunScript>
-    <RestorePackages>false</RestorePackages>
-    <RestorePackages>true</RestorePackages>
-  </PropertyGroup>
-  <PropertyGroup>
+    <CLRTestKind Condition="'$(CLRTestKind)' == ''">SharedLibrary</CLRTestKind>

The conditional updates are the change to the default in Directory.Build.targets. The PropertyGroup removals are intended (not unchecked) removals of (now) empty groups.

This was referenced Jul 31, 2023
Closed
Closed
Open
Closed
Closed
Closed
Closed
@buyaa-n buyaa-n added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Reflection.Metadata labels Jul 31, 2023
@ghost
Copy link

ghost commented Jul 31, 2023

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

Issue Details

I came across these while looking at the set of properties in order to generate some error checking for merged groups. As best as I can tell, most of these are very old uses that likely have had the test system changed from under them. They cause confusion when encountered.

  • RestorePackages - I think has been replaced by the logic that bulk restores.
  • CLRTestKind - A (somewhat) more recent change was Switch over Loader/classloader/TypeGeneratorTests to use merged wrappers #61942, which changed the default for CLRTestKind from "BuildAndRun" to "BuildAndRun if exe, else BuildOnly". However, BuildOnly appears to be intended for exes - build the exe but not test infrastructure to execute it as another project will handle that. I think SharedLibrary is what was intended here. Many OutputType=library tests specify BuildOnly or SharedLibrary, which seem incorrect and (now) unnecessary.
  • GenerateRunScript - This is unconditionally set (overriding individual project files) in src/tests/Directory.Build.targets.
Author: markples
Assignees: markples
Labels:

area-CodeGen-coreclr
Milestone: -

trylek
trylek approved these changes Jul 31, 2023
View reviewed changes
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.

Thanks Mark for cleaning this up!

@markples markples assigned trylek and unassigned markples Aug 8, 2023
@trylek
Copy link
Member

trylek commented Aug 21, 2023

Based on offline discussion with Mark I have rebased this change against latest main and I plan to retest it and merge it in once it passes as the start of my quality week activities as Mark has fully switched over to GC work.

@trylek
Copy link
Member

trylek commented Aug 21, 2023

/azp run runtime-extra-platforms

@trylek
Copy link
Member

trylek commented Aug 21, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 21, 2023

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

azure-pipelines bot commented Aug 21, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@trylek
Copy link
Member

trylek commented Aug 22, 2023

Runtime and outerloop runs are clean and for the runtime-extra-platforms I haven't found any issues different from a previous run, merging in.

@trylek trylek merged commit 5fea131 into dotnet:main Aug 22, 2023
This was referenced Aug 22, 2023
Closed
Closed
Closed
@MichalStrehovsky MichalStrehovsky mentioned this pull request Aug 30, 2023
Merged
@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Aug 30, 2023

We stopped running the SharedLibrary test after this change.

I'm putting back the ClrTestKind for this test in #91356 but I do wonder if there's any other test we stopped running by dropping ClrTestKind.

@markples
Copy link
Contributor Author

markples commented Aug 31, 2023

This one happened because the test was marked OutputType==library and ClrTestKind=BuildAndRun. However, it looks like it depends on a native entry point (and CLRTestBatchPreCommands, etc.), so the managed part of the test is "library" but the overall project is an executable. I just did a scan of all proj files in that commit that removed BuildAndRun, and SharedLibrary is the only one of the ~30 that doesn't have OutputType==exe.

@markples markples deleted the prop_update branch September 28, 2023 23:34
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@trylek trylek trylek approved these changes

Assignees

@trylek trylek

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markples @trylek @MichalStrehovsky @jkoritzinsky @buyaa-n