Uh oh!
There was an error while loading. Please reload this page.
[tests] Cover virtual callbacks during construction - #12566
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 | tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/VirtualCallbackConstructorTests.cs — 💡 suggestion — javaClass is declared with using var but never used, which generates CS0219… |
What changed in this PR
This PR expands the trimmable typemap runtime coverage test suite by adding a new fixture that validates virtual managed callbacks invoked from Java base constructors during object construction. It targets constructor-time peer identity stability, re-entrant/concurrent lookup behavior, and exception propagation in both managed-first and Java-first activation paths.
Changes:
- Added a new Java base type whose constructor calls an overridable method to trigger virtual callbacks during construction.
- Added a new managed test fixture covering managed-first vs Java-first construction, peer identity invariants, bounded concurrency, and callback exception behavior.
- Wired the new test into the
Mono.Android.NET-Testsproject compilation list.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj | Includes the new virtual-callback constructor parity tests in the on-device test project. |
| tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/VirtualCallbackConstructorBase.java | Introduces the Java base constructor callback trigger used by the new parity fixture. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/VirtualCallbackConstructorTests.cs | Implements NUnit coverage for constructor-time virtual callbacks, identity invariants, bounded concurrency, and exception propagation. |
Uh oh!
There was an error while loading. Please reload this page.
simonrozsival
commented
Sep 2, 2026
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
simonrozsival
commented
Sep 3, 2026
/review |
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Verdict: ✅ LGTM
Findings: 0 errors · 0 warnings · 1 suggestion
The fixture exercises the important managed-first and Java-first construction paths, reentrant and concurrent lookup, exactly-once activation, and exception propagation. The bounded waits avoid hanging a device-test lane, and all CI checks are green. I left one non-blocking inline suggestion to clean up unexpected duplicate JNI peers on assertion-failure paths.
Generated by Android PR Reviewer for #12566 · gpt56 · 107.7 AIC · ⌖ 8.88 AIC · ⊞ 25.7K
Comment /review to run again
| var registered = Java.Lang.Object.GetObject<VirtualCallbackConstructorDerived> ( | ||
| instance.Handle, | ||
| JniHandleOwnership.DoNotTransfer); | ||
| Assert.AreSame (instance, registered); |
There was a problem hiding this comment.
🤖 💡 JNI references — This assertion deliberately detects a duplicate managed peer, but if GetObject returns a different wrapper, Assert.AreSame exits without disposing that unexpected wrapper and its JNI reference. Please mirror ConstructorActivationTests.AssertRegisteredSame: use a try/finally and dispose registered when it is non-null and not ReferenceEquals(instance, registered). The same failure-path cleanup should cover any distinct wrappers captured in ConcurrentPeers before those references are cleared.
Rule: Dispose unexpected JNI peers
Uh oh!
There was an error while loading. Please reload this page.
Summary
Matrix
The unchanged fixture passes under llvm-ir/MonoVM, trimmable/CoreCLR, and trimmable/NativeAOT (3/3 each). No production change was required.
Part of #12561