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

Use unique IL assembly names instead of automatic renaming #85185

Merged
merged 13 commits into from
May 12, 2023

Conversation

markples
Copy link
Member

The main motivation for this is that I got stuck trying to write documentation for test merging and test suffixes. We have _il_d for IL and both _d and _cs_d for C# (and similar for _r and others - I'll use _d to denote all such tests here). Only the IL files needed to be separated into different merged groups, yet we split the C# as well.

This change renames the _il_d to just _d (unless there is already an existing _d, likely for a C# version of the test). It then replaces .assembly <name> with .assembly ASSEMBLY_NAME and adds per-configuration "prefix" IL source files with #define ASSEMBLY_NAME <name>_d. Then the rules are simple - tests have unique assembly names and no build-time renaming is required.

This lets me remove the 4 Regression merged groups with ~10 tests each in them. I rebalanced the merged groups where the number of _d tests were larger except HardwareIntrinsics (a unique directory) and Methodical (most of the tests are _d).

@ghost
Copy link

ghost commented Apr 21, 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

The main motivation for this is that I got stuck trying to write documentation for test merging and test suffixes. We have _il_d for IL and both _d and _cs_d for C# (and similar for _r and others - I'll use _d to denote all such tests here). Only the IL files needed to be separated into different merged groups, yet we split the C# as well.

This change renames the _il_d to just _d (unless there is already an existing _d, likely for a C# version of the test). It then replaces .assembly <name> with .assembly ASSEMBLY_NAME and adds per-configuration "prefix" IL source files with #define ASSEMBLY_NAME <name>_d. Then the rules are simple - tests have unique assembly names and no build-time renaming is required.

This lets me remove the 4 Regression merged groups with ~10 tests each in them. I rebalanced the merged groups where the number of _d tests were larger except HardwareIntrinsics (a unique directory) and Methodical (most of the tests are _d).

Author: markples
Assignees: markples
Labels:

area-System.Reflection.Metadata

Milestone: -

@markples
Copy link
Member Author

PTAL @trylek @jkoritzinsky
cc @dotnet/jit-contrib

The github view on this is pretty bad between the large number of files and missing the renames. The commits are logically separated:

@markples markples marked this pull request as ready for review April 21, 2023 22:58
@markples
Copy link
Member Author

A local pri 1 build/run showed that I didn't lose swaths of tests. I'll run outerloop, extraplatforms, and gcstress - the last after my jit64 changes are merged.

@markples
Copy link
Member Author

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

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, this indeed looks much cleaner than the hacky assembly renames in the scripts. It almost makes me wonder whether there might be a way to define the ASSEMBLY_NAME macro directly in the MSBuild script so that we don't need the auxiliary one-line IL files but I guess I'm probably asking too much, this is a nice improvement by itself already.

@markples
Copy link
Member Author

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

/azp run runtime-extra-platforms, gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

markples commented May 5, 2023

/azp run runtime-extra-platforms, gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

markples commented May 5, 2023

(just retesting because old runs were old and noisy, no new changes requiring re-review)

@markples
Copy link
Member Author

markples commented May 5, 2023

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member Author

MemorySsa is failing elsewhere

@markples markples merged commit c2d2416 into dotnet:main May 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants