[TrimmableTypeMap] Add CoreCLRTrimmable Mono.Android.NET-Tests lane#11091
[TrimmableTypeMap] Add CoreCLRTrimmable Mono.Android.NET-Tests lane#11091simonrozsival wants to merge 5 commits intomainfrom
Conversation
f8106b0 to
a437455
Compare
d5aefaa to
3a8d509
Compare
1814a6d to
d0e30f6
Compare
17b123b to
e2c70fd
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e2c70fd to
87c531f
Compare
There was a problem hiding this comment.
Pull request overview
Adds CI/test-lane plumbing to run Mono.Android.NET-Tests on CoreCLR with the trimmable typemap implementation, keeping currently-unsupported test buckets explicitly excluded to maintain green coverage while follow-up runtime/generator work lands separately.
Changes:
- Adds a new
CoreCLRTrimmableAPK instrumentation lane and pins the existingCoreCLRlane tollvm-ir. - Updates
Mono.Android.NET-Tests.csprojto default to CoreCLR + exclude trimmable-specific buckets when_AndroidTypeMapImplementation=trimmable. - Centralizes additional trimmable-mode runtime test exclusions in
NUnitInstrumentation, and adjusts a few Java.Interop tests/projects to avoid unsupported coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.RuntimeTests/NUnitInstrumentation.cs | Adds runtime-switch-driven exclusions for categories and specific Java.Interop test names under trimmable typemap. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj | Sets defaults/exclusions for trimmable typemap runs (incl. CoreCLRTrimmable flavor). |
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JnienvTest.cs | Marks specific tests as TrimmableIgnore. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs | Marks a specific test as TrimmableIgnore (and normalizes file header). |
| tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj | Removes JniTypeUtf8Test.cs from compilation due to unsupported APIs in the product assembly. |
| build-tools/automation/yaml-templates/stage-package-tests.yaml | Updates CoreCLR lane args and adds the new CoreCLRTrimmable lane. |
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict: ✅ LGTM
Found 1 suggestion:
- 💡 Code organization — the trimmable category filtering is now split between
Mono.Android.NET-Tests.csprojandNUnitInstrumentation.cs(NUnitInstrumentation.cs:29). Keeping one source of truth would make the temporary exclusions easier to retire.
The lane wiring itself looks good, and I like that the unsupported cases stay explicit instead of being hidden behind runtime fallback behavior.
Review generated by android-reviewer from review guidelines.
02780bb to
c3a7ebc
Compare
619991d to
6a0644f
Compare
19ec2fa to
e084238
Compare
e02ec3c to
4abdc1e
Compare
|
Splitting this PR into focused, reviewable pieces so each fix gets the attention it deserves. This PR will stay open and become a thin, clean addition to CI configuration once all the children land on Child PRs
Once the children merge, this PR will be rebased to include only:
Follow-ups (issues)
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the trimmable typemap PR focused on the CoreCLRTrimmable lane and its targeted test-side plumbing. Move the generic NUnit runner and logcat auditing work to PR #11162, and drop the leftover Preserve-related noise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
Nice split between the trimmable typemap/product work here and the generic harness changes in #11162 — that makes this PR much easier to reason about. I left one inline suggestion on the new regression tests.
CI note: the public checks look good, but the internal Xamarin.Android-PR pipeline is still pending, so this is not merge-ready quite yet.
## Summary Improve the Android NUnit harness so trimmable and test-lane investigations can audit discovery and exclusions reliably. ## Changes - fix final Passed/Failed/Skipped summary counts to match NUnit aggregate results - add a discovery-only dry-run mode - add noexclusions=true to bypass built-in exclusions for auditing - surface excluded tests as ignored results with explicit reasons - overwrite logcat output per run so stale logs do not masquerade as current crashes ## Why These generic test-runner improvements were useful while working on #11091. I had problems identifying which tests were discovered, but did not run, as passed + skipped + failed < total. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
|
failing tests are unrelated |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM — well-structured change
Summary: Adds the CoreCLRTrimmable Mono.Android.NET-Tests CI lane and fixes several trimmable typemap issues exposed by running Release + CoreCLR + trimmable builds. Clean and well-organized PR.
What works well
PropagateCannotRegisterToDescendants— correct bidirectional propagation (up to base classes, then down to descendants) ensures the complete transitive closure. Algorithm is simple and appropriate for the small number of types involved.CollectApplicationRegistrationTypes— smart filtering that keeps only the two framework roots plus app/runtime types, avoiding legacyandroid.test.*descendants that don't exist on modern API surfaces. Well-tested.AdditionalResolvedAssembliesonGenerateNativeApplicationConfigSources— clean separation of counting (for native config) from assembly store inclusion (handled by_AddTrimmableTypeMapAssembliesToStore). Fixes the incremental build issue where the old approach inside_GenerateJavaStubscould be skipped.- Test coverage — new unit tests, build-level regression tests for assembly store mappings, and incremental build verification.
- TrimmerRoots.xml — targeted preservation of only the types needed by the test suite via reflection.
Issues found
| Severity | Count |
|---|---|
| 💡 Suggestion | 5 |
No bugs or correctness issues found. All suggestions are minor formatting/style items.
CI Status
Public checks (license/cla, dotnet-android) are ✅ passing. Internal Azure DevOps build status should be verified separately.
Generated by Android PR Reviewer for issue #11091 · ● 9.1M
| skipped on incremental builds, which would leave the item group empty. | ||
| --> | ||
| <Target Name="_PrepareTrimmableNativeConfigAssemblies" | ||
| BeforeTargets="_GeneratePackageManagerJava"> |
There was a problem hiding this comment.
🤖 💡 MSBuild targets — _PrepareTrimmableNativeConfigAssemblies uses BeforeTargets="_GeneratePackageManagerJava", but $(_GeneratePackageManagerJavaDependsOn) exists and is the preferred extension point. Since this file is imported after Common.targets, you could append to the property:
<PropertyGroup>
<_GeneratePackageManagerJavaDependsOn>
_PrepareTrimmableNativeConfigAssemblies;
$(_GeneratePackageManagerJavaDependsOn)
</_GeneratePackageManagerJavaDependsOn>
</PropertyGroup>That said, BeforeTargets is simpler here and the comment explains the rationale well, so this is a "consider" not a "must fix."
Rule: Avoid BeforeTargets/AfterTargets (Postmortem #33)
| } | ||
|
|
||
| var environmentText = File.ReadAllText (environmentFile); | ||
| var runtimeDataMatch = Regex.Match (environmentText, @"assembly_store_bundled_assemblies.*\[(\d+)\s+x"); |
There was a problem hiding this comment.
🤖 💡 Performance — The two Regex patterns inside the foreach loop are recompiled on every iteration. For a test this isn't critical, but consider extracting them to static readonly Regex fields (or using [GeneratedRegex]) for cleanliness:
static readonly Regex AssemblyStoreCountRegex = new (@"assembly_store_bundled_assemblies.*\[(\d+)\s+x", RegexOptions.Compiled);
static readonly Regex MappingIndexRegex = new (@"\bmi:(\d+)\b", RegexOptions.Compiled);Rule: Avoid unnecessary allocations
|
@copilot implement the suggestions in review comments |
Implemented in 36b783e. Tests in our project that are trimmable-incompatible now use
Addressed in 36b783e — tests in this project that are trimmable-incompatible now carry |
… run Fix several trimmable typemap generator and runtime bugs that prevented Mono.Android.NET-Tests from passing in Release+CoreCLR+trimmable mode: Generator: - Use (managedName, assemblyName) tuple as scanner dictionary key to prevent duplicate-type crashes when two assemblies define the same managed type name (e.g. Java.Lang.Throwable in both Java.Interop and Mono.Android) - Add MergeCrossAssemblyAliases to propagate aliases across assembly boundaries before splitting peers into per-assembly typemap universes (Release only) - Emit TypeMapAssociationAttribute for all entries with proxies so the runtime proxy type map is populated correctly (fixes CreatePeer for interface types) - ForceUnconditionalEntries=true workaround for dotnet/runtime#127004 (trimmer strips TypeMapAssociation attributes when TypeMap references the same type) - Fix invalid ParameterHandle (was default/row-0); use valid 1-based row index - Skip inner per-RID builds in _GenerateTrimmableTypeMap (they lack the full assembly set needed for correct deferred-registration propagation) Runtime: - Add JavaPeerProxy.ShouldSkipActivation(IntPtr) to detect existing managed peers and prevent duplicate peer creation during UCO constructor callbacks - Use ShouldSkipActivation in UCO nctor callbacks instead of WithinNewObjectScope Build: - Add _PrepareTrimmableNativeConfigAssemblies target to populate native config with typemap DLL paths (must run unconditionally, not inside _GenerateJavaStubs) - Add typemap DLLs to _ShrunkAssemblies to keep _RemoveRegisterAttribute counts in sync with _ResolvedAssemblies Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Configure Mono.Android.NET-Tests to build and run successfully with _AndroidTypeMapImplementation=trimmable and UseMonoRuntime=false. Changes: - Root test and runner assemblies in TrimmerRoots.xml so NUnit can discover and execute tests after trimming - Exclude 48 tests that are incompatible with the trimmable typemap (tracked in #11170): - 5 tests that require net.dot.jni.test.*/net.dot.jni.internal.* Java classes - 24 tests relying on JavaProxyObject/JavaProxyThrowable (not in APK) - 4 proxy resolution / trimmer gaps - 4 JavaCast proxy resolution failures - 4 open generic type / registration failures - 4 JNI method remapping not supported - 3 other - Exclusions are guarded by RuntimeFeature.TrimmableTypeMap so they only apply to trimmable builds - Add TODO comments to tests with known trimmable-specific failures (issue #11170) so they are visible to contributors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a new CI lane that runs Mono.Android.NET-Tests with the trimmable typemap implementation on CoreCLR: _AndroidTypeMapImplementation=trimmable UseMonoRuntime=false The lane runs after the existing CoreCLR lane and uses the same APK artifact from the CoreCLRTrimmable build flavor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fd05865 to
2d750b0
Compare
In non-trimmed (Debug) builds, _AndroidEnableObjectReferenceLogging defaults to true, so RuntimeFeature.ObjectReferenceLogging returns true. The existing Meter test already uses the same #if DEBUG pattern; apply it here too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The feature switch (ObjectReferenceLoggingEnabledByDefault) is the source of truth; its default value varies by build configuration (true in non-trimmed Debug builds, false in trimmed Release builds). Hardcoding an expected value in the test is fragile. Remove the test case entirely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Add the
CoreCLRTrimmableMono.Android.NET-TestsCI lane and fix the Release regressions exposed by the trimmable typemap path.This PR includes
[TrimmableTypeMap] Fix bugs exposed by Release+CoreCLR+trimmable test run
Generator fixes:
(managedName, assemblyName)tuple as scanner dictionary key — prevents duplicate-type crashes when two assemblies define the same managed name (e.g.Java.Lang.Throwablein bothJava.InteropandMono.Android)MergeCrossAssemblyAliasesto propagate aliases across assembly boundaries before splitting into per-assembly typemap universes (Release only)TypeMapAssociationAttributefor all entries with proxies so the runtime proxy type map is populated correctly (fixesCreatePeerfor interface types)ForceUnconditionalEntries=trueworkaround for dotnet/runtime#127004 — trimmer stripsTypeMapAssociationattributes whenTypeMapreferences the same typeParameterHandle(wasdefault/row-0); use valid 1-based row indexRuntime fixes:
JavaPeerProxy.ShouldSkipActivation(IntPtr)to detect existing managed peers and prevent duplicate peer creation during UCO constructor callbacksBuild fixes:
_PrepareTrimmableNativeConfigAssembliestarget to populate native config unconditionally (not inside_GenerateJavaStubswhich can be skipped)_ShrunkAssembliesto keep_RemoveRegisterAttributecounts in sync_GenerateTrimmableTypeMapin inner per-RID builds[tests] Add CoreCLRTrimmable test plumbing
TrimmerRoots.xmlRuntimeFeature.TrimmableTypeMap[CI] Add CoreCLRTrimmable lane
Mono.Android.NET-Testswith_AndroidTypeMapImplementation=trimmableandUseMonoRuntime=falseExcluded tests (48)
All 48 excluded tests are precisely named in
NUnitInstrumentation.cs— no over-broad prefix matching. They are excluded only for trimmable builds (guarded byRuntimeFeature.TrimmableTypeMap). See #11170 for full inventory with root causes.JavaProxyObject/JavaProxyThrowablenot in APKnet.dot.jni.test.*/net.dot.jni.internal.*Java classesValidation
Local
Release + CoreCLR + trimmablerun:NativeTypeMap,Export, and other non-trimmable categories)