test: add comprehensive unit tests for uncovered modules#4
Open
b3y0urs3lf wants to merge 2 commits intomainfrom
Open
test: add comprehensive unit tests for uncovered modules#4b3y0urs3lf wants to merge 2 commits intomainfrom
b3y0urs3lf wants to merge 2 commits intomainfrom
Conversation
Add 324 new tests across 12 test files covering previously untested areas: - NostrClient (72 tests): connection lifecycle, message handling, publishing, subscriptions, reconnection, NIP-17 messaging, nametag queries, disconnect cleanup, configuration combinations, concurrent operations - Schnorr edge cases (33 tests): input validation for wrong key/message lengths, tampered signatures, cross-key verification - EventKinds comprehensive (49 tests): isReplaceable, isEphemeral, isParameterizedReplaceable boundary testing, getName for all kinds - Nametag edge cases (42 tests): phone heuristic edge cases, normalization, hash determinism, display formatting, country code handling - Event mutability (25 tests): tags/content mutability, JSON serialization, ID calculation determinism, isValidEventData edge cases - NIP-44 edge cases (29 tests): corrupted data, version byte validation, payload boundaries, padding algorithm verification - KeyManager NIP-44 (18 tests): encrypt/decrypt with bytes/hex keys, conversation key symmetry - Event edge cases (15 tests): empty/many/unicode tags, long content, deterministic IDs, JSON roundtrip - Security critical paths (15 tests): key clearing, memory zeroing, copy semantics, NIP-17 sender anonymity, timestamp randomization - WebSocket adapter (11 tests): extractMessageData for various data types - NIP-04 edge cases (9 tests): corrupted ciphertext, invalid IV, GZIP errors - CallbackEventListener (6 tests): callback invocation patterns Known issues documented in tests: - Event.tags is mutable (modifying breaks signature) - Event.toJSON() returns same tags reference (not a copy) - BIP-340 signatures use randomness (not deterministic) Total test count: 274 -> 598 tests (118% increase) All tests pass, no source code modifications.
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the unit test suite to cover previously untested (or lightly tested) parts of the SDK—especially around NostrClient, crypto edge cases (Schnorr/NIP-04/NIP-44), protocol helpers, and security-sensitive behaviors—without modifying production source code.
Changes:
- Adds extensive unit tests for
NostrClientcovering connection lifecycle, message handling, publishing, subscriptions, reconnection, NIP-17 flows, and concurrency. - Adds edge-case/unit coverage for Schnorr signing/verification, NIP-04 corrupted inputs, NIP-44 corrupted inputs + padding boundaries, and KeyManager NIP-44 helpers.
- Adds protocol/utility coverage for
EventKinds,Eventedge cases & mutability behaviors, nametag normalization/hashing/display, plus a BDD scenario specification file.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/websocket-adapter.test.ts | Adds tests for extractMessageData and ready-state constants; includes a placeholder createWebSocket test. |
| tests/unit/security-critical.test.ts | Adds tests for key clearing behavior, copy semantics, NIP-17 anonymity/timestamping, conversation-key symmetry, and AUTH event structure. |
| tests/unit/schnorr-edge-cases.test.ts | Adds boundary and tamper-case tests for Schnorr key/message/signature validation and verification behavior. |
| tests/unit/nostr-client.test.ts | Adds a large suite of unit tests using mocked WebSockets to cover key NostrClient behaviors and edge cases. |
| tests/unit/nip44-edge-cases.test.ts | Adds corrupted payload/version checks and padding boundary/roundtrip tests for NIP-44. |
| tests/unit/nip04-edge-cases.test.ts | Adds malformed/corrupted ciphertext and IV-format tests for NIP-04 decrypt paths. |
| tests/unit/nametag-edge-cases.test.ts | Adds tests for phone heuristics, normalization, hashing determinism, display masking, and country-code handling. |
| tests/unit/keymanager-nip44.test.ts | Adds KeyManager NIP-44 encrypt/decrypt tests for byte/hex keys, boundaries, cleared-state behavior, and symmetry. |
| tests/unit/event-mutability.test.ts | Adds tests documenting mutability pitfalls (tags/content) and JSON/reference behaviors. |
| tests/unit/event-kinds-comprehensive.test.ts | Adds comprehensive boundary and mapping tests for EventKinds classification and naming. |
| tests/unit/event-edge-cases.test.ts | Adds event construction/signing tests for extreme tags/content, unicode, tag helpers, and JSON roundtrips. |
| tests/unit/callback-listener.test.ts | Adds tests validating CallbackEventListener callback invocation patterns. |
| tests/bdd-test-scenarios.feature | Adds a large BDD-style scenario specification describing desired behaviors across features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b9fac72 to
3a5826f
Compare
Address all 13 review comments: replace no-op assertions with meaningful checks, fix incorrect calcPaddedLen BDD spec values, remove unused imports/variables, add missing reqsBefore assertion, improve type casts, and properly exercise createWebSocket. Co-Authored-By: Claude Opus 4.6 <[email protected]>
3a5826f to
4d3d08b
Compare
MastaP
requested changes
Mar 2, 2026
Member
MastaP
left a comment
There was a problem hiding this comment.
- BDD feature file is dead weight — bdd-test-scenarios.feature (1206 lines) isn't wired to any runner. It's documentation pretending to be a test artifact.
Should either be connected to a framework (cucumber-js) or moved to docs. - Polling loop in NostrClient test — nostr-client.test.ts line ~1025 uses a setTimeout polling loop (50 iterations x 20ms) to wait for async encryption in the
publishEncryptedMessage test. Fragile; should use flushMicrotasks() consistently like the other tests do. - Merge conflict risk — The PR branch is based on c11f996 (v0.3.2) and is missing ff9b11f (nametag validation) from main. The existing nametag.test.ts was
modified in that commit, so merging will likely conflict. The PR needs a rebase onto current main. - No tests for isValidNametag() — The nametag validation function added in ff9b11f has no edge-case coverage in the new nametag-edge-cases.test.ts. After
rebasing, tests for that function should be added.
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.
Add 324 new tests across 12 test files covering previously untested areas:
Known issues documented in tests:
Total test count: 274 -> 598 tests (118% increase) All tests pass, no source code modifications.