Add comprehensive unit tests (366 new tests)#4
Open
b3y0urs3lf wants to merge 1 commit into
Open
Conversation
No production code changes — only test files added/extended. New test files (14): - SecurityCriticalTest: key clearing, copy semantics, NIP-17 anonymity - NIP44EdgeCasesTest: version validation, payload bounds, padding - EventMutabilityTest: tags mutability, null-ID equality, JSON serialization - NostrEventListenerTest: callback patterns, default methods - Bech32Test, NIP04EncryptionTest, NostrKeyManagerTest, SchnorrSignerTest - EventKindsTest, EventTest, NametagBindingTest, NametagUtilsTest - PaymentRequestProtocolTest, TokenTransferProtocolTest Extended existing tests (2): - NIP44EncryptionTest: padding boundaries, tampering detection - NIP17ProtocolTest: gift wrap scenarios, rumor handling Key findings from testing: Security concerns documented: - clear() doesn't prevent further key use (zeroed key still works) - No curve point validation in NIP-04 ECDH - Ephemeral key material lingers in JVM heap (BigInteger immutable) Design decisions with user impact: - Two null-ID Events are considered equal (HashSet data loss risk) - Phone heuristic can misclassify text nametags with many digits - NIP-04 uses SHA-256 hashed shared secret (non-standard, Unicity-specific) - getTags() returns mutable internal reference All core logic verified correct: - Encryption round-trips (NIP-04, NIP-44, NIP-17) work properly - Schnorr signing deterministic, verification rejects tampering - NIP-44 padding follows spec (power-of-2 chunks, min 32 bytes) - Gift wrap timestamp randomization within ±2 day window - Payment request/response/decline flow works end-to-end - Defensive copies made for private keys in NostrKeyManager Total: 442 tests across 19 test files
MastaP
added a commit
that referenced
this pull request
May 7, 2026
…connect re-check Four issues from Copilot review #4 on the Java PR: 1. RelayConnection.connected was a non-volatile boolean. Written from the OkHttp WebSocket listener thread (onOpen / onClosed / onFailure) and from the executor thread (disconnect / scheduleReconnect); read from listener threads via allRelaysDoneFor for multi-relay query settlement. Without volatile, a stale "connected = true" read after disconnect could prevent prompt query settlement on the very disconnect path allRelaysDoneFor is supposed to handle. Made connected (and wasConnected for symmetry) volatile. 2. Relay disconnect mid-query never triggered allRelaysDoneFor re-check — Copilot raised this on TS but it applies equally to Java. Added a synthetic onError fan-out in BOTH onClosed and onFailure for active subs, so queryWithFirstSeenWins settles promptly when a relay drops without sending EOSE/CLOSED. 3. Tests didn't actually drive the relay-aware path. Existing reflection invoked handleRelayMessage(String), passing relay=null, so the per-relay closedSubIds / eosedSubIds bookkeeping was never exercised. Added two new tests that construct a RelayConnection via reflection (it's a non-static inner class, so we pass the outer NostrClient to its constructor), mark it connected, and drive handleRelayMessage(RelayConnection, String) directly: - closedFrameOnRealRelayPopulatesPerRelayClosedSubIds — covers terminal CLOSED → closedSubIds populated; and auth-required CLOSED → closedSubIds NOT populated (transient). - eosedFrameOnRealRelayPopulatesPerRelayEosedSubIds — covers EOSE → eosedSubIds populated. 4. Misleading test class Javadocs (both RelayFixesTest and RelayFixesE2ETest claimed coverage they didn't actually have). Updated to accurately describe which paths each suite exercises and to point at the new relay-aware tests for the per-relay bookkeeping. 13/13 unit + 5/5 e2e passing.
MastaP
added a commit
that referenced
this pull request
May 11, 2026
…connect re-check Four issues from Copilot review #4 on the Java PR: 1. RelayConnection.connected was a non-volatile boolean. Written from the OkHttp WebSocket listener thread (onOpen / onClosed / onFailure) and from the executor thread (disconnect / scheduleReconnect); read from listener threads via allRelaysDoneFor for multi-relay query settlement. Without volatile, a stale "connected = true" read after disconnect could prevent prompt query settlement on the very disconnect path allRelaysDoneFor is supposed to handle. Made connected (and wasConnected for symmetry) volatile. 2. Relay disconnect mid-query never triggered allRelaysDoneFor re-check — Copilot raised this on TS but it applies equally to Java. Added a synthetic onError fan-out in BOTH onClosed and onFailure for active subs, so queryWithFirstSeenWins settles promptly when a relay drops without sending EOSE/CLOSED. 3. Tests didn't actually drive the relay-aware path. Existing reflection invoked handleRelayMessage(String), passing relay=null, so the per-relay closedSubIds / eosedSubIds bookkeeping was never exercised. Added two new tests that construct a RelayConnection via reflection (it's a non-static inner class, so we pass the outer NostrClient to its constructor), mark it connected, and drive handleRelayMessage(RelayConnection, String) directly: - closedFrameOnRealRelayPopulatesPerRelayClosedSubIds — covers terminal CLOSED → closedSubIds populated; and auth-required CLOSED → closedSubIds NOT populated (transient). - eosedFrameOnRealRelayPopulatesPerRelayEosedSubIds — covers EOSE → eosedSubIds populated. 4. Misleading test class Javadocs (both RelayFixesTest and RelayFixesE2ETest claimed coverage they didn't actually have). Updated to accurately describe which paths each suite exercises and to point at the new relay-aware tests for the per-relay bookkeeping. 13/13 unit + 5/5 e2e passing.
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.
No production code changes — only test files added/extended.
New test files (14):
Extended existing tests (2):
Key findings from testing:
Security concerns documented:
Design decisions with user impact:
All core logic verified correct:
Total: 442 tests across 19 test files