Feat(test)/unity test framework#276
Open
JoshhSandhu wants to merge 3 commits intomagicblock-labs:mainfrom
Open
Feat(test)/unity test framework#276JoshhSandhu wants to merge 3 commits intomagicblock-labs:mainfrom
JoshhSandhu wants to merge 3 commits intomagicblock-labs:mainfrom
Conversation
Relax assertion in ECDSASignatures.cs: p1363Offset check changed from > 0 to >= 0 to allow a zero offset when packing P-256 P1363 signature components. Prevents erroneous assertion failures when a valid zero offset is used.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a large set of Unity edit-mode tests (cryptography, JSON‑RPC, Mobile Wallet Adapter client/session), supporting assembly and metadata, a test mock, and a one-line change relaxing an assertion in ECDSA signature packing and a corrected decryption error message. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs`:
- Line 99: The test DecryptSessionPayload_* is asserting the wrong log message —
it expects an encrypt-related error ("Cannot encrypt, no session key has been
established") in a decrypt test; update the LogAssert.Expect call in the test
method (the assertion on the log message) to assert the corresponding decrypt
message (e.g., "Cannot decrypt, no session key has been established") or
otherwise match the actual error produced by the DecryptSessionPayload method so
the test validates the correct behavior.
In `@Tests/EditMode/EditMode.asmdef`:
- Line 13: Update the Tests/EditMode assembly definition by setting the
"allowUnsafeCode" property to false (change the "allowUnsafeCode" entry
currently true in EditMode.asmdef) so the assembly disables unsafe code; ensure
the JSON key remains present and set to false to maintain tighter security
defaults.
In `@Tests/EditMode/Mocks/MockMessageSender.cs`:
- Around line 15-25: The mock currently stores and returns references to
caller-provided byte[] buffers which can be mutated later; update
MockMessageSender to defensive-copy buffers: in Send(byte[] message) create and
store a new byte[] copy into SentMessages (rather than the original reference),
and change the LastMessage getter to return a copy of the stored buffer (or
null) so callers cannot mutate internal state; reference the Send method, the
SentMessages collection, and the LastMessage property when making these changes.
In `@Tests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs`:
- Line 53: Change the request-construction tests in
MobileWalletAdapterClientTests from void to async Task and await the
asynchronous calls instead of discarding them; specifically update each test
that currently does `_ = _client.Authorize(...)` (and other similar `_ =
_client.*` calls at the listed locations) to `await _client.Authorize(...)`,
adjust the test method signatures to `async Task` so exceptions surface and the
Task is awaited, and apply this change consistently across all instances
referenced (lines for the Authorize call and the other similar calls in the same
test class).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed9d59b4-718f-478f-a2c8-bd2bc21d45e0
📒 Files selected for processing (19)
Runtime/codebase/SolanaMobileStack/Crypto/ECDSASignatures.csTests.metaTests/EditMode.metaTests/EditMode/Crypto.metaTests/EditMode/Crypto/EcdsaSignaturesTests.csTests/EditMode/Crypto/EcdsaSignaturesTests.cs.metaTests/EditMode/Crypto/MobileWalletAdapterSessionTests.csTests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs.metaTests/EditMode/EditMode.asmdefTests/EditMode/EditMode.asmdef.metaTests/EditMode/JsonRpc.metaTests/EditMode/JsonRpc/ResponseModelTests.csTests/EditMode/JsonRpc/ResponseModelTests.cs.metaTests/EditMode/Mocks.metaTests/EditMode/Mocks/MockMessageSender.csTests/EditMode/Mocks/MockMessageSender.cs.metaTests/EditMode/MwaClient.metaTests/EditMode/MwaClient/MobileWalletAdapterClientTests.csTests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs.meta
| const string cluster = "mainnet-beta"; | ||
|
|
||
| // Act | ||
| _ = _client.Authorize(identityUri, iconUri, identityName, cluster); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm client API returns Task"
rg -nP --type=cs -C2 '\bTask<[^>]+>\s+Authorize\s*\(|\bTask<[^>]+>\s+Reauthorize\s*\(' Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
echo "2) Show discarded-task call sites in this test file"
rg -nP --type=cs -C1 '_\s*=\s*_client\.(Authorize|Reauthorize)\s*\(' Tests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs
echo "3) Inspect mock sender completion behavior"
rg -nP --type=cs -C4 'class\s+MockMessageSender|Task\.CompletedTask|Task\.FromResult|Send\s*\(' Tests/EditModeRepository: magicblock-labs/Solana.Unity-SDK
Length of output: 2971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full Authorize implementation ==="
sed -n '22,32p' Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
echo -e "\n=== Full Reauthorize implementation ==="
sed -n '34,45p' Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
echo -e "\n=== Check Send signature ==="
rg -nP --type=cs 'Task\s+Send\s*\(|void\s+Send\s*\(' Runtime/codebase/SolanaMobileStack
echo -e "\n=== Check IMessageSender interface ==="
rg -nP --type=cs -A5 'interface\s+IMessageSender' Runtime/codebase/SolanaMobileStackRepository: magicblock-labs/Solana.Unity-SDK
Length of output: 1607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find SendRequest implementation ==="
rg -nP --type=cs -A10 'Task<.*>\s+SendRequest\s*<' Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
echo -e "\n=== Check if there's async waiting after Send() ==="
rg -nP --type=cs -B5 -A10 'SendRequest' Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs | head -60Repository: magicblock-labs/Solana.Unity-SDK
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for SendRequest definition ==="
rg -nP --type=cs 'Task<.*>\s+SendRequest' Runtime/codebase/SolanaMobileStack/
echo -e "\n=== Check if MobileWalletAdapterClient has a base class ==="
rg -nP --type=cs 'class MobileWalletAdapterClient' Runtime/codebase/SolanaMobileStack/
echo -e "\n=== Look for SendRequest in broader context ==="
rg -nP --type=cs 'SendRequest' Runtime/codebase/SolanaMobileStack/ | head -20Repository: magicblock-labs/Solana.Unity-SDK
Length of output: 1120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== SendRequest implementation in JsonRpc20Client ==="
sed -n '25,50p' Runtime/codebase/SolanaMobileStack/JsonRpc20Client.cs
echo -e "\n=== Check full JsonRpc20Client context ==="
wc -l Runtime/codebase/SolanaMobileStack/JsonRpc20Client.cs
echo -e "\n=== Full JsonRpc20Client file (if small enough) ==="
cat -n Runtime/codebase/SolanaMobileStack/JsonRpc20Client.csRepository: magicblock-labs/Solana.Unity-SDK
Length of output: 4291
Convert request-construction tests to async Task and await call results.
These methods return Task<AuthorizationResult>, but discarding them with _ = swallows potential exceptions and masks async failures. Convert to async Task test methods and await the calls.
Proposed pattern (apply across similar tests)
+using System.Threading.Tasks;
...
-[Test]
-public void Authorize_SendsJsonRpc_WithCorrectMethod()
+[Test]
+public async Task Authorize_SendsJsonRpc_WithCorrectMethod()
{
// Arrange
var identityUri = new Uri("https://example.com");
var iconUri = new Uri("/icon.png", UriKind.Relative);
const string identityName = "TestApp";
const string cluster = "mainnet-beta";
// Act
- _ = _client.Authorize(identityUri, iconUri, identityName, cluster);
+ await _client.Authorize(identityUri, iconUri, identityName, cluster);
// Assert
var request = DecodeLastRequest();
Assert.AreEqual("authorize", request.Method,
"Method must be 'authorize'");
}Also applies to: 68-68, 83-83, 98-98, 114-114, 129-129, 133-133, 161-161, 186-186, 202-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs` at line 53,
Change the request-construction tests in MobileWalletAdapterClientTests from
void to async Task and await the asynchronous calls instead of discarding them;
specifically update each test that currently does `_ = _client.Authorize(...)`
(and other similar `_ = _client.*` calls at the listed locations) to `await
_client.Authorize(...)`, adjust the test method signatures to `async Task` so
exceptions surface and the Task is awaited, and apply this change consistently
across all instances referenced (lines for the Authorize call and the other
similar calls in the same test class).
Contributor
Author
There was a problem hiding this comment.
The discard is intentional. Authorize() / Reauthorize() call SendRequest<T>() which sends the payload to IMessageSender synchronously and then returns a TaskCompletionSource.Task that only completes when a wallet response arrives via Receive(). In these tests no response ever arrives because MockMessageSender is a one-way capture, so awaiting the task would deadlock every test.
The tests only need to inspect the outgoing request payload, which is already captured in the mock by the time the next line runs. The fire-and-forget is the correct pattern here.
There was a problem hiding this comment.
The PR description image shows the Unity Test Runner with all 31 EditMode tests passing (green checkmarks), including the 11 MobileWalletAdapterClientTests.
@JoshhSandhu, that's a clear and correct explanation — thank you. The TaskCompletionSource-backed response tracking means await without a paired Receive() would block indefinitely, making the discard the only safe option for outbound-payload-only tests. I'm withdrawing the suggestion.
(ᓀ‸ᓂ) withdrawing suggestion
✏️ Learnings added
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 276
File: Tests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs:53-53
Timestamp: 2026-04-09T00:49:25.699Z
Learning: In `magicblock-labs/Solana.Unity-SDK`, `MobileWalletAdapterClient.Authorize()` and `Reauthorize()` call `SendRequest<T>()`, which delivers the payload to `IMessageSender` **synchronously** and then returns a `TaskCompletionSource<T>.Task` that only completes when a wallet response arrives via `Receive()`. In EditMode unit tests that use `MockMessageSender` (a one-way payload capture with no response path), awaiting these tasks would deadlock. The correct pattern is `_ = _client.Authorize(...)` — discard the task after the synchronous send so the test can immediately inspect the captured payload. Do not flag this as a missing `await`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:291-294
Timestamp: 2026-03-29T01:23:40.762Z
Learning: In `magicblock-labs/Solana.Unity-SDK`, in `Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`, the `SignMessage()` method deliberately uses `cachedPk` (captured from `Account?.PublicKey` or `PlayerPrefs.GetString("pk")` before the `LocalAssociationScenario`) rather than `authorization.PublicKey` returned by the first lambda. This is intentional: under the current `async void Action<IAdapterOperations>` pattern, mutations made to closed-over variables in the first lambda are not guaranteed to be visible in the second lambda, so cross-lambda variable sharing is unsafe. The fix (using `authorization.PublicKey` for signing) requires migrating to `Func<IAdapterOperations, Task>`, which is deferred to PR `#260`.
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:63-93
Timestamp: 2026-03-29T00:04:25.932Z
Learning: In `magicblock-labs/Solana.Unity-SDK`, the `async client => { ... }` lambdas passed to `LocalAssociationScenario.StartAndExecute()` use `Action<IAdapterOperations>` (effectively async void), which can swallow exceptions. This is a known architectural issue intentionally left unchanged to avoid conflicting with PR `#260`, which is rewriting `LocalAssociationScenario.ExecuteNextAction()` and is expected to migrate the delegate type to `Func<IAdapterOperations, Task>`.
fix decrypt error string, defensive copy in mock, disable unsafe code in test assembly
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
1 participant
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.
Problem
The Solana Unity SDK did not have any automated test infrastructure before this PR.
That meant core Mobile Wallet Adapter logic had no repeatable regression coverage, including crypto helpers, session state behavior, JSON-RPC request construction, and response model convenience properties. It also made it harder to validate changes requested during recent MWA work without relying entirely on manual testing.
Solution
This PR adds the first Unity Test Framework scaffold to the SDK and introduces 31 EditMode unit tests across 4 test classes.
The new tests focus on logic that can be exercised in pure EditMode without requiring Android runtime dependencies:
EcdsaSignaturescrypto helpersMobileWalletAdapterSessionMobileWalletAdapterClientJSON-RPC request buildingResponse<T>computed propertiesA
MockMessageSendertest double was added forIMessageSenderso request payloads can be asserted without a real transport.This PR is intentionally scoped to the parts of the MWA stack that are already testable in Editor.
SolanaMobileWalletAdapterlifecycle and reconnect flows are not included yet because they currently depend on constructor behavior that throws on non-Android platforms.Before & After Screenshots
Insert screenshots of example code output
BEFORE:
N/A. No automated tests existed in the SDK before this PR.
AFTER:
Other changes (e.g. bug fixes, small refactors)
Tests/structure for Unity Test Framework coverageDeploy Notes
This PR primarily adds test infrastructure and EditMode unit coverage. It does not introduce new package dependencies or deployment steps.
New scripts:
Tests/EditMode/Crypto/EcdsaSignaturesTests.cs: EditMode coverage for DER/P1363 conversion and P-256 key encode/decodeTests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs: EditMode coverage for session token and pre-ECDH error pathsTests/EditMode/JsonRpc/ResponseModelTests.cs: EditMode coverage forResponse<T>success/failure propertiesTests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs: EditMode coverage for JSON-RPC request buildingTests/EditMode/Mocks/MockMessageSender.cs: Test double forIMessageSenderNew dependencies:
Summary by CodeRabbit
Tests
Bug Fixes
Chores