[tests] Cover interface-valued Java collections - #12614
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/InterfaceCollectionMarshallingTests.cs — 🤖 💡 suggestion Testing — Add an assertion message here so failures from JNIEnv.IsSameObject… |
What changed in this PR
Adds device-level regression coverage in tests/Mono.Android-Tests to ensure Java-created collections whose element (and/or key/value) types are Java interfaces marshal correctly in managed code (identity preservation, null handling, enumeration/mutation, inherited interfaces, and Java round-trips). This supports confidence that the interface-collection scenario described in #11770 behaves correctly on current main without requiring production/runtime changes.
Changes:
- Add a new NUnit fixture covering
JavaList<TInterface>,JavaCollection<TInterface>, andJavaDictionary<…>shapes whereTInterfaceis a bound Java interface, including round-trips and identity checks. - Add Java-side deterministic fixtures (
InterfaceCollectionHolder, package-privateInterfaceCollectionPeer) to produce stable interface-typed collections for testing. - Wire the new test fixture into
Mono.Android.NET-Tests.csproj.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj | Includes the new interface-collection marshalling test in the device test project. |
| tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/InterfaceCollectionPeer.java | Adds a package-private concrete peer implementing the test interfaces. |
| tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/InterfaceCollectionHolder.java | Adds Java factory/round-trip methods producing deterministic interface-typed collections/maps. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/InterfaceCollectionMarshallingTests.cs | New NUnit coverage for interface-valued collection/list/dictionary marshaling, identity, nulls, and inherited-interface invokers. |
Suppressed comments (1)
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/InterfaceCollectionMarshallingTests.cs:251
- 🤖 💡 suggestion Testing — Add an assertion message here so a failure clearly indicates that two distinct Java handles unexpectedly compared as the same object.
var firstPeer = (IJavaObject) first;
var secondPeer = (IJavaObject) second;
Assert.IsFalse (JNIEnv.IsSameObject (firstPeer.Handle, secondPeer.Handle));
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Dispose JNI class references if a later lookup fails, and include peer types and handles in identity assertion failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Parse DGML semantically in two passes so node/link order and XML formatting do not affect the result. Require every selected rooting node to be unique while preserving the complete canonical dependency and incoming-edge assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate DGML namespaces, node identities, and exact compiler symbol shapes so malformed or decorated nodes cannot satisfy the rooting chain. Add a per-run result token so stale logcat output cannot pass a retried fixture when log clearing fails. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 0 suggestions
The focused fixture is well isolated, exercises the intended interface wrapper shapes, and carefully validates JNI identity and NativeAOT rooting. The remaining issue is the shortened device-result timeout, which undercuts the suite shared protection against slow CI emulator launches.
CI build 1576508 is still in progress: four Android tools/Java.Interop jobs and CLA have passed, three platform build jobs are running, and no failures are currently reported.
Generated by Android PR Reviewer for #12614 · gpt56 · 148.8 AIC · ⌖ 8.86 AIC · ⊞ 25.7K
Comment /review to run again
Use DeviceTest.ActivityStartTimeoutInSeconds while waiting for the app result so slow CI emulator launches do not fail the focused NativeAOT case prematurely. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Found 1 warning and 0 errors. The fixture isolates the interface-valued collection paths well and the JNI local-reference cleanup is careful, but the result polling should use one target-aware logcat stream rather than repeatedly spawning full-buffer adb logcat -d snapshots.
CI is still in progress for build 1576613: the completed Android Tools macOS check and CLA have passed; six checks are running and the aggregate check is queued.
Generated by Android PR Reviewer for #12614 · gpt56 · 122.9 AIC · ⌖ 9.3 AIC · ⊞ 25.7K
Comment /review to run again
Use MonitorAdbLogcat instead of repeatedly dumping logcat inside WaitFor. Start the app after monitoring begins, retain the per-run result token and shared timeout, and make the streaming process honor ADB_TARGET like RunAdbCommand. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Match the dictionary factory method node emitted by the current NativeAOT scanner instead of expecting a conditional type-metadata node. Keep exact labels for each factory and preserve the existing rooting-chain and unexpected-dependency assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move the six behavioral cases into the existing Mono.Android.NET-Tests project with NUnit assertions and individual discovery. Exercise managed dictionary enumeration instead of raw Map.Entry traversal, and share the unbound Java fixture and JNI holder with a minimal NativeAOT rooting probe. Keep the exclusive NativeAOT dependency-graph assertions in MSBuildDeviceIntegration without a custom six-case runner or pass counter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve the runtime test project conflict by retaining both InterfaceCollectionMarshallingTests and the new upstream InvokerActivationTests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ILC can place the dictionary newobj edge behind a conditional factory/type-metadata dependency, as seen in CI build 1585926, rather than directly on the factory method. Accept both representations while requiring the conditional Primary and Secondary inputs and preserving exclusive incoming-edge checks. Add a reduced CI graph plus positive and negative NUnit coverage for direct and conditional roots, missing or unrelated dependencies, and ambiguous nodes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
[Test]cases in the existingMono.Android.NET-Testsproject for interface-valued Java collectionsThe behavioral fixture uses the existing NUnit runner and assertions, so each case has its own result and a failure does not prevent the remaining cases from running. Dictionary enumeration exercises the managed wrappers rather than traversing raw
java.util.Map.Entryobjects.The NUnit fixture and isolated probe share the Java collection fixture and raw JNI holder. The holder and base/derived concrete peers remain unbound and are retained by focused ProGuard rules. Both use the existing
ValueProviderandExtendedValueProviderinterfaces. Collection conversion callsJavaConvert.FromJniHandle(..., Type)with user-facingIList<T>,ICollection<T>, orIDictionary<TKey,TValue>targets, without directly constructing closed Java collection wrappers.Base-interface cases assert exact
IValueProviderInvokeractivation; inherited-interface coverage asserts exactIExtendedValueProviderInvokeractivation. The NUnit cases also assert open wrapper definitions and runtime generic arguments.NativeAOT rooting isolation
The small
MSBuildDeviceIntegrationapp only creates and disposes a list, collection, and dictionary through the shared JNI holder. The host-side NUnit test monitors its per-run completion token using target-awareMonitorAdbLogcat, then reads its single-RID scan DGML.The graph assertions check the factory-to-constructed-type edge, generic dictionary and canonical constructor dependencies, and activation constructor. They reject unexpected incoming dependencies and other closed reference-wrapper activation constructors sharing the same canonical code. Keeping this probe separate prevents unrelated tests and the runtime test project's blanket roots from masking missing factory roots.
Validation
Using the locally built SDK and
emulator-5554with another device attached:llvm-ir, Release, arm64trimmable, Release, arm64trimmable, Release, arm64The runtime cases run through the existing MTP/NUnit integration with
IncludeCategories=InterfaceCollectionsand produce individual TRX results. No production/runtime changes are needed for this coverage.Out of scope
JavaCollection<T>.Removehas a pre-existing runtime-independent JNI descriptor bug: it looks upCollection.remove(int)as(I)Ljava/lang/Object;and fails under thellvm-irbaseline before interface marshaling. This fixture usesClearfor collection mutation; remove remains covered byJavaList<T>and all dictionary shapes.Fixes #11770