Delete ILCompiler.Compiler.Tests unit test - #133474
Merged
MichalStrehovsky merged 2 commits intoSep 9, 2026
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The migrated tests introduce build-breaking issues (warnings-as-errors from unused usings, plus an ambiguous MetadataType reference) that must be fixed before merge.
Pull request overview
This PR removes the ILCompiler.Compiler.Tests unit test project and migrates the Swift lowering coverage into ILCompiler.TypeSystem.Tests, updating the associated test-data assembly and build wiring accordingly.
Changes:
- Delete
ILCompiler.Compiler.Tests(and its assets) and remove it from the AOT solution and toolstest subset. - Add
SwiftLoweringTeststoILCompiler.TypeSystem.Tests, including the shared JIT-interface sources needed for Swift lowering verification. - Move/retarget Swift lowering test data into
CoreTestAssemblyunder aTypeSystemTests.TestData.*namespace, and add missing test-data sources.
File summaries
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/SwiftLoweringTests.cs | Moves Swift lowering test to TypeSystem tests and switches to TestTypeSystemContext/CoreTestAssembly-based discovery. |
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/ILCompiler.TypeSystem.Tests.csproj | Adds linked JIT-interface/Pgo sources and includes SwiftLoweringTests.cs. |
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/SwiftTypesSupport.cs | Renames Swift test-data namespace to TypeSystemTests.TestData.SwiftTypes. |
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/SwiftTypes.cs | Renames Swift test-data namespace to TypeSystemTests.TestData.SwiftTypes. |
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/Platform.cs | Adds InlineArrayAttribute stub needed for compiling inline-array test data in the custom corelib. |
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/GvmVariantInterface.cs | Moves the variant-GVM test data into CoreTestAssembly (used by existing TypeSystem tests). |
| src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/CoreTestAssembly.csproj | Removes link to the old ILCompiler.Compiler.Tests asset path (now local file exists). |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/ILCompiler.Compiler.Tests.csproj | Deleted test project. |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/ILCompiler.Compiler.Tests.Assets/ILCompiler.Compiler.Tests.Assets.csproj | Deleted assets project. |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/ILCompiler.Compiler.Tests.Assets/Devirtualization.cs | Deleted devirtualization asset source. |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/ILCompiler.Compiler.Tests.Assets/DependencyGraph.cs | Deleted dependency-graph asset source. |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/DevirtualizationTests.cs | Deleted unit tests. |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/DependencyGraphTests.cs | Deleted unit tests. |
| src/coreclr/tools/aot/ilc.slnx | Removes the deleted test project from the solution. |
| eng/Subsets.props | Removes the deleted test project from clr.toolstests. |
Review details
Suppressed comments (4)
src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/SwiftLoweringTests.cs:35
MetadataTypeis ambiguous here because bothSystem.Reflection.MetadataandInternal.TypeSystemdefine aMetadataType. With the currentusing System.Reflection.Metadata;, this should fail to compile with CS0104 unless the type is qualified or aliased.
src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/SwiftTypesSupport.cs:7using System.Runtime.InteropServices;is unused in this file, and the repo builds withTreatWarningsAsErrors=true, so this will fail the build (CS8019).
src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/SwiftTypes.cs:8using System;is unused in this file, and the repo builds withTreatWarningsAsErrors=true, so this will fail the build (CS8019).
src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/SwiftLoweringTests.cs:39- Offsets parsing is currently checking the wrong level:
NamedArguments.FirstOrDefault(...).Valueis aCustomAttributeTypedArgument<TypeDesc>, so the pattern match toImmutableArray<CustomAttributeTypedArgument<TypeDesc>>will never succeed. This makesoffsetsalways null and can silently ignore explicitOffsets = [...]data in the test attribute.
- Files reviewed: 14/15 changed files
- Comments generated: 0
- Review effort level: Lite
jkotas
approved these changes
Sep 9, 2026
agocke
approved these changes
Sep 9, 2026
agocke
left a comment
Member
There was a problem hiding this comment.
Strong agree with: I've never seen these tests fail due to a bug that wasn't caught elswehere.
MichalStrehovsky
enabled auto-merge (squash)
September 9, 2026 22:24
maximmenshikov
added a commit
to maximmenshikov/runtime
that referenced
this pull request
Sep 22, 2026
Fourteen helpers - {FLT,DBL}{ADD,SUB,MUL,DIV}, {FLT,DBL}CMP_{LE,GE}, FLT2DBL and
DBL2FLT - bound by ilc to the toolchain's compiler-rt builtins (__adddf3,
__ledf2, ...), the same way CORINFO_HELP_DBLREM is already bound to fmod. No
floating-point arithmetic is implemented in the runtime or the libraries.
The target is selected explicitly as TargetAbi.NativeAotRiscV64SoftFloat,
spelled riscv64-lp64 on the ilc command line, following the armel precedent. It
drives the JIT flag, the RISC-V ELF float-ABI field in e_flags (which is what
lets the linker reject a mix of lp64 and lp64d objects), the instruction-set
defaults and a two-way validation: lp64d requires F and D, lp64 must not have
them. crossgen2 rejects the target - the helpers have no ReadyToRun encoding.
Value numbering models the helpers as the operations they implement, as
CORINFO_HELP_LMUL is modelled on 32-bit targets; only the two three-way
compares need VNFuncs of their own.
Note: the RiscV64ObjectWriterTests addition was dropped - ILCompiler.Compiler.Tests
was deleted upstream in dotnet#133474 and needs a new home.
Signed-off-by: Maxim Menshikov <maksim.menshikov@nethermind.io>
maximmenshikov
added a commit
to maximmenshikov/runtime
that referenced
this pull request
Sep 23, 2026
Fourteen helpers - {FLT,DBL}{ADD,SUB,MUL,DIV}, {FLT,DBL}CMP_{LE,GE}, FLT2DBL and
DBL2FLT - bound by ilc to the toolchain's compiler-rt builtins (__adddf3,
__ledf2, ...), the same way CORINFO_HELP_DBLREM is already bound to fmod. No
floating-point arithmetic is implemented in the runtime or the libraries.
The target is selected explicitly as TargetAbi.NativeAotRiscV64SoftFloat,
spelled riscv64-lp64 on the ilc command line, following the armel precedent. It
drives the JIT flag, the RISC-V ELF float-ABI field in e_flags (which is what
lets the linker reject a mix of lp64 and lp64d objects), the instruction-set
defaults and a two-way validation: lp64d requires F and D, lp64 must not have
them. crossgen2 rejects the target - the helpers have no ReadyToRun encoding.
Value numbering models the helpers as the operations they implement, as
CORINFO_HELP_LMUL is modelled on 32-bit targets; only the two three-way
compares need VNFuncs of their own.
Note: the RiscV64ObjectWriterTests addition was dropped - ILCompiler.Compiler.Tests
was deleted upstream in dotnet#133474 and needs a new home.
Signed-off-by: Maxim Menshikov <maksim.menshikov@nethermind.io>
maximmenshikov
added a commit
to maximmenshikov/runtime
that referenced
this pull request
Sep 25, 2026
Fourteen helpers - {FLT,DBL}{ADD,SUB,MUL,DIV}, {FLT,DBL}CMP_{LE,GE}, FLT2DBL and
DBL2FLT - bound by ilc to the toolchain's compiler-rt builtins (__adddf3,
__ledf2, ...), the same way CORINFO_HELP_DBLREM is already bound to fmod. No
floating-point arithmetic is implemented in the runtime or the libraries.
The target is selected explicitly as TargetAbi.NativeAotRiscV64SoftFloat,
spelled riscv64-lp64 on the ilc command line, following the armel precedent. It
drives the JIT flag, the RISC-V ELF float-ABI field in e_flags (which is what
lets the linker reject a mix of lp64 and lp64d objects), the instruction-set
defaults and a two-way validation: lp64d requires F and D, lp64 must not have
them. crossgen2 rejects the target - the helpers have no ReadyToRun encoding.
Value numbering models the helpers as the operations they implement, as
CORINFO_HELP_LMUL is modelled on 32-bit targets; only the two three-way
compares need VNFuncs of their own.
Note: the RiscV64ObjectWriterTests addition was dropped - ILCompiler.Compiler.Tests
was deleted upstream in dotnet#133474 and needs a new home.
Signed-off-by: Maxim Menshikov <maksim.menshikov@nethermind.io>
maximmenshikov
added a commit
to maximmenshikov/runtime
that referenced
this pull request
Sep 25, 2026
Fourteen helpers - {FLT,DBL}{ADD,SUB,MUL,DIV}, {FLT,DBL}CMP_{LE,GE}, FLT2DBL and
DBL2FLT - bound by ilc to the toolchain's compiler-rt builtins (__adddf3,
__ledf2, ...), the same way CORINFO_HELP_DBLREM is already bound to fmod. No
floating-point arithmetic is implemented in the runtime or the libraries.
The target is selected explicitly as TargetAbi.NativeAotRiscV64SoftFloat,
spelled riscv64-lp64 on the ilc command line, following the armel precedent. It
drives the JIT flag, the RISC-V ELF float-ABI field in e_flags (which is what
lets the linker reject a mix of lp64 and lp64d objects), the instruction-set
defaults and a two-way validation: lp64d requires F and D, lp64 must not have
them. crossgen2 rejects the target - the helpers have no ReadyToRun encoding.
Value numbering models the helpers as the operations they implement, as
CORINFO_HELP_LMUL is modelled on 32-bit targets; only the two three-way
compares need VNFuncs of their own.
Note: the RiscV64ObjectWriterTests addition was dropped - ILCompiler.Compiler.Tests
was deleted upstream in dotnet#133474 and needs a new home.
Signed-off-by: Maxim Menshikov <maksim.menshikov@nethermind.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I expect this to stir up controversy so I'll preface this that I do understand the value of unit testing and it was me who added this testing in the first place (dotnet/corert#4285).
Over the years, we have developed a sense for when adding a test to ILCompiler.Compiler.Tests is appropriate: never. In 9 years we accumulated testing for 3 things in here:
The tests never failed for me because they found a bug in my code. They only failed because they couldn't compile or had to be adapted to new behaviors.
The problem is the AI is defaulting to adding tests here. The tests that the AI adds here are two kinds:
We could write AI guidance to never add tests here. But then why do we have these tests? So I'm deleting them. AI should follow the testing strategy that we have been successful with for 9 years and add E2E testing in src/tests.
Testing in src/tests is much higher in value because it runs for all our runtimes and AOT compilers, and executes with all our stress modes and optimization levels. The ILCompiler.Compiler.Tests does none of that and if we make it do that, then we're just replicating src/tests.
I'm moving the Swift lowering testing to type system tests because it's much more related to that than anything else. But if we think it's redundant with the Swift lowing testing that we have in src/tests, we could delete it too. I don't have strong opinion on that one.