Add relay integration tests and network condition testing #3508
Conversation
- Add .github/workflows/network-tests.yml for CI testing under 3G/Edge conditions using sitespeedio/throttle (wraps macOS pfctl/dnctl) - Add scripts/throttle-network.sh for local network throttling - Add .beads/ directory for tracking research and implementation notes - WebSocket testing approaches (10 approaches analyzed) - Issue tracking for poor connectivity testing work - Architecture analysis of relay/WebSocket stack The network throttling affects all TCP traffic including WebSocket, enabling realistic testing of relay connections under poor conditions. Signed-off-by: alltheseas <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Implements Phase 1 of WebSocket testing infrastructure: - Extract WebSocketProtocol from WebSocket class for dependency injection - Modify RelayConnection to accept injectable WebSocketProtocol - Create MockWebSocket for unit testing without real network - Add comprehensive RelayConnectionTests (19 tests) covering: - Connection state management - Disconnection handling - Error handling with exponential backoff - Ping/pong behavior - Message sending - Network condition simulation This enables testing of relay connection logic in isolation without requiring real WebSocket connections or network throttling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Phase 2 of WebSocket testing infrastructure: - Create RelayIntegrationTests.swift with real relay connections - Tests NIP-01 protocol flows: - REQ → EVENT → EOSE subscription flow - EVENT → OK publish flow - CLOSE subscription management - Reconnection after disconnect - Multiple rapid subscriptions stress test - Supports local strfry relay (Docker) or public relay fallback - Add throttled network test variants for 3G/Edge simulation - Update CI workflow to: - Start strfry Docker container for testing - Run RelayConnectionTests and RelayIntegrationTests - Proper cleanup of strfry container Run locally with strfry: docker run -d -p 7777:7777 --name strfry-test dockurr/strfry Run with throttling: sudo throttle --profile 3g xcodebuild test -only-testing:damusTests/RelayIntegrationTests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add WebSocket injection to RelayPool.add_relay() to enable testing with MockWebSocket. Create RelayPoolMultiRelayTests with 8 tests covering: - Multi-relay connection management - Partial relay failure handling - Message routing to all connected relays - Message queuing when disconnected - Queue flushing on reconnect - Relay isolation (disconnect doesn't affect others) - Reconnection handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
PostBoxTests (17 tests): - Event queuing and OK response handling - Multi-relay send coordination - on_flush callbacks (.once and .all modes) - Delayed send and cancel functionality - Retry logic with exponential backoff EventHolderTests (13 tests): - Queue vs immediate insert modes - Event deduplication across modes - Flush behavior and event ordering - Queue callbacks and reset These tests cover the core posting and timeline infrastructure for network resilience testing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Adds comprehensive test coverage for Direct Message functionality: DirectMessageTests (17 tests): - NIP-04 encryption/decryption round-trips - Unicode and emoji message handling - DM event creation (kind 4) - DirectMessageModel: initialization, is_request logic - DirectMessagesModel: lookup, conversation filtering - handle_incoming_dm: insertion, sorting, deduplication DMPostBoxTests (9 tests): - DM sending to relay - OK response handling - Multi-relay delivery - Network disconnection/queuing - Delayed send and cancellation - on_flush callback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
ZapNetworkTests (15 tests): - Zap request creation (kind 9734) - PostBox integration for zap events - Multi-relay broadcast - on_flush callbacks - ZapType enum behavior ProfileNetworkTests (15 tests): - Metadata event creation (kind 0) - Profile JSON encoding - PostBox integration - Multi-relay broadcast - on_flush callbacks Closes beads: damus-b6g, damus-0o9 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
SearchFilterTests (13 tests): - Hashtag tag detection via events - event_matches_hashtag behavior - event_matches_filter behavior - NostrFilter.filter_hashtag creation - Search filter kinds SearchEventHolderTests (6 tests): - Search result deduplication - Time-based sorting (newest first) - Batch insertion - Reset clears results - Queued search results - on_queue callback for preloading SearchStateTests (4 tests): - Filter limit configuration - Text search filter kinds - Hashtag search filter - Combined hashtag/kind filter SearchHomeFilterTests (3 tests): - Base filter kinds (text, chat) - Until timestamp - Follow pack filter SearchEventKindTests (5 tests): - Text note is_textlike - Longform is_textlike - Highlight is_textlike - Reaction not textlike - Metadata not textlike Closes beads: damus-2zx 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
DMPostBoxTests additions: - testDMRejectedByRelayIsRemoved: Verify rejection handling - testMultipleDMsQueuedDuringOutage: Queue integrity during disconnect - testConnectionFlappingDuringDMDelivery: Rapid connect/disconnect cycles - testPartialRelaySuccess: Mixed accept/reject across relays - testDMEncryptedContentSurvivesQueue: Encryption preserved in queue Documents actual PostBox behavior under poor network conditions. Closes beads: damus-uwn 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…rks (18 tests) ReactionNetworkTests (5 tests): - Like event (kind 7) sent to relay - Reaction removed on OK - Reaction queued when disconnected - Custom emoji reactions - Multi-relay delivery BoostNetworkTests (5 tests): - Boost event (kind 6) sent to relay - Boost removed on OK - Boost queued when disconnected - Boost content contains original event - Boost includes proper e and p tags FollowNetworkTests (5 tests): - Follow list (kind 3) sent to relay - Follow list removed on OK - Follow list queued when disconnected - Multiple pubkeys in follow list - Multi-relay delivery for redundancy MuteListNetworkTests (3 tests): - Mute list (kind 10000) sent to relay - Mute list removed on OK - Mute list queued when disconnected BookmarkListNetworkTests (3 tests): - Bookmark list (kind 10003) sent to relay - Bookmark list removed on OK - Bookmark list queued when disconnected Closes beads: damus-85i, damus-96t, damus-bvg, damus-hxj, damus-xtm 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
NotificationsNetworkTests (8 tests): - Event queuing and immediate insertion - Flush moves queued events to notifications - Duplicate event rejection - Reactions grouped by target event - Notifications sorted by time (newest first) - Unique pubkeys extraction ThreadLoadingNetworkTests (9 tests): - ThreadEventMap add/retrieve/contains operations - Duplicate event handling in map - Chronological sorting of events - Reply hierarchy tracking - Thread/meta/quote filter creation - Missing parent handled gracefully (no crash) UserLookupNetworkTests (11 tests): - Profile metadata filter (kind 0) - Profile filter with author pubkey - Multiple profile lookup - Metadata event creation and JSON content - Relay list filter (kind 10002) - Contacts filter (kind 3) - Profile subscription with multiple kinds Closes beads: damus-hw6, damus-0tg, damus-hoz 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add test/strfry-test.conf with writePolicy.plugin="" to accept all events
- Update CI workflow to mount config and use ghcr.io/dockur/strfry
- Update RelayIntegrationTests.swift comments with new Docker command
The default dockurr/strfry image has a write policy that blocks test events
("pubkey not in whitelist"). This config disables that restriction for testing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Always use relay.damus.io since iOS Simulator can't reach Docker localhost - Switch from Timer-based to async polling for more reliable connection waiting - Add FORCE_PUBLIC_RELAY environment variable option for local development - Improve isLocalRelayAvailable() with multiple connection checks All 8 integration tests now pass reliably. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- getRelayURL() no longer overrides networkTimeout if already set higher, preserving 60s timeout for throttled tests - connectToRelay() now throws on timeout instead of just XCTFail, preventing cascading failures from unconnected relay - Restore local strfry check for CI environments where Docker works 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughIntroduces comprehensive network-condition testing infrastructure for the Damus iOS app, including WebSocket protocol abstraction for dependency injection, mock implementations, extensive test suites for relay/DM/Zap/profile/search networking, macOS throttling script, CI workflow for automated network-throttled tests, and beads issue tracking configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant RCon as RelayConnection
participant WSProto as WebSocketProtocol
participant MockWS as MockWebSocket
participant RPub as Relay (Publisher)
Note over Test,RPub: Old Flow (Hardcoded WebSocket)
Test->>RCon: init(url)
RCon->>RCon: lazy var socket = WebSocket(url)
Note over Test,RPub: New Flow with Dependency Injection
Test->>MockWS: create mock socket
Test->>RCon: init(url, webSocket: mock)
RCon->>RCon: self.socket = webSocket
RCon->>WSProto: connect()
MockWS->>MockWS: subject.send(.connected)
WSProto-->>RPub: publishes connection event
Note over Test,RPub: Testing Network Failure Scenario
Test->>MockWS: simulateNetworkLost()
MockWS->>WSProto: subject.send(.disconnected(...))
RCon->>RCon: handle disconnect, trigger backoff
RCon->>RCon: attempt reconnect
Test->>MockWS: simulateConnect()
MockWS->>WSProto: subject.send(.connected)
RCon->>RCon: reset backoff, re-subscribe
Note over Test,RPub: Message Send with OK Response
Test->>RCon: send message
RCon->>WSProto: send EVENT
MockWS->>RPub: forwards to relay
RPub->>MockWS: responds with OK
MockWS->>WSProto: subject.send(.message(ok))
RCon->>RCon: process OK, update state
rect rgb(240, 248, 255)
Note over Test,RPub: Benefits of Protocol-Based Abstraction
Test->>MockWS: full control over timing/failures
Test->>MockWS: deterministic event sequencing
Test->>MockWS: no real network calls needed
end
sequenceDiagram
participant CI as GitHub Actions<br/>(network-tests.yml)
participant Xcode as Xcode Build<br/>& Test
participant Throttle as Throttle Script<br/>(macOS dnctl)
participant Relay as Strfry Test<br/>Relay
participant Sim as iOS<br/>Simulator
CI->>Throttle: start throttle profile (3g/edge)
Throttle->>Throttle: configure dummynet pipe<br/>with bandwidth/delay/loss
CI->>Relay: start Docker container<br/>ws://localhost:7777
Relay->>Relay: listen on port 7777
CI->>Sim: boot iPhone 15 simulator
Sim->>Sim: ready for tests
par Test Execution Under Throttling
CI->>Xcode: run network-condition tests<br/>(CODE_SIGNING_ALLOWED=NO)
Xcode->>Sim: execute test suite
Sim->>Relay: establish WebSocket<br/>through throttled pipe
Note over Throttle: packet loss, latency,<br/>bandwidth limits applied
Relay-->>Sim: respond with throttled delay
Sim->>Xcode: collect results
end
CI->>Throttle: stop throttle
CI->>Relay: stop Docker relay
CI->>Xcode: upload test results<br/>with profile label
rect rgb(240, 248, 255)
Note over CI,Sim: CI Matrix: runs with [3g, edge] profiles<br/>on push to master, manual dispatch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: Substantial heterogeneous changes spanning WebSocket protocol abstraction (affecting core relay/connection logic), 10+ new comprehensive test files with multiple test classes per file (DM, Zap, Profile, Event, Relay networking), infrastructure additions (CI workflow, throttling script, test relay config), and beads project setup. While many test files follow repeatable patterns, each domain (relay, DM, Zap, search, etc.) requires separate reasoning. The protocol refactoring and dependency injection across RelayConnection/RelayPool demand careful attention to ensure correct wiring and backward compatibility. Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Comment |
|
need help setting this up in ci |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
damus/Core/Nostr/RelayPool.swift (1)
171-188: Add docstring for add_relay method.The modified
add_relaymethod now accepts an optionalwebSocketparameter for dependency injection, but lacks documentation. Per coding guidelines, docstring coverage is required for added or modified code.📝 Proposed docstring
+ /// Adds a relay to the pool with optional WebSocket injection for testing. + /// + /// - Parameters: + /// - desc: The relay descriptor containing URL and read/write info + /// - webSocket: Optional WebSocket instance for dependency injection (primarily for testing) + /// - Throws: RelayError.RelayAlreadyExists if the relay is already in the pool func add_relay(_ desc: RelayDescriptor, webSocket: WebSocketProtocol? = nil) async throws(RelayError) {Based on coding guidelines.
🤖 Fix all issues with AI Agents
In @.github/workflows/network-tests.yml:
- Around line 65-75: The xcodebuild test step ("Run Tests under throttled
network") can hang under throttled networks; prefix the xcodebuild invocation
with a timeout (e.g., timeout 10m or gtimeout 10m) so the command is killed
after 10 minutes, e.g. change the run block that calls xcodebuild test (the
command that writes NetworkTestResults.xcresult and pipes to xcpretty) to be
executed under a timeout wrapper to prevent infinite hanging.
In @damus.xcodeproj/project.pbxproj:
- Line 7244: There is a duplicate/malformed PBXBuildFile entry with ID
D73E5EFE2C6A97F4007EB227 referenced in multiple build phases; locate all
references to D73E5EFE2C6A97F4007EB227 in project.pbxproj (e.g., the
PBXSourcesBuildPhase and the other BuildPhase where it appears) and remove the
incorrect duplicate reference so the ID appears only once in the appropriate
PBXBuildFile list, and if necessary remove or correct the corresponding
PBXBuildFile block so the project has a single valid BuildFile entry for that
file.
- Line 1512: The PBXBuildFile entry with UUID D73E5EFE2C6A97F4007EB227 is
malformed (it lacks a required fileRef) and should be fixed; either remove the
entire PBXBuildFile entry for D73E5EFE2C6A97F4007EB227 from the project.pbxproj
or restore it by adding the correct fileRef (e.g., fileRef = <UUID> /*
<Filename> */;) that matches the intended source file so the PBXBuildFile object
is valid.
In @damusTests/RelayPoolTests.swift:
- Around line 58-84: The test uses a fire-and-forget Task so assertions run
asynchronously and the test can finish before they execute; change testAddRelays
to be async (e.g., func testAddRelays(urls: [String], expectedError:
RelayPool.RelayError? = nil) async) and remove the Task block, performing the
awaits directly (e.g., await relayPool.add_relay(descriptor)) so the caller can
await it; also update all callers (individual test methods) to be async and
await testAddRelays (or alternatively replace the Task with an XCTestExpectation
and fulfill it on completion), ensuring error handling stays the same for
RelayPool.RelayError and other catches.
In @scripts/throttle-network.sh:
- Around line 59-65: The check_root function currently references $* but the
callers don't pass arguments, so the suggested sudo usage is incomplete; update
the case statement callers to invoke check_root "$@" (preserving original CLI
args) and also change the echo inside check_root to use "$@" (or "$*") to
correctly render the full command and its arguments in the error message.
🧹 Nitpick comments (12)
.beads/beads-backup/.gitignore (1)
1-34: Duplicate gitignore patterns across.beads/and.beads/beads-backup/directories.The content is functionally identical to
.beads/.gitignore. While having separate gitignore files in different directories is valid Git practice, this creates a maintenance burden where pattern changes must be replicated in two places. Consider whether a single shared file or documentation-driven approach would be clearer.damusTests/EventHolderTests.swift (1)
73-74: Consider using@discardableResultpattern or explicit_ =for clarity.Multiple
holder.insert()calls intentionally discard the return value. While this is valid when testing side effects rather than return values, adding explicit_ = holder.insert(...)would silence potential compiler warnings and make intent clearer.Also applies to: 160-161, 220-220, 224-224, 244-244, 247-247, 269-270, 295-295, 298-299
damusTests/ProfileNetworkTests.swift (1)
292-303: Consider more thorough JSON validation.While the current test checks for basic JSON structure, you could strengthen it by attempting to decode the JSON back to validate it's well-formed and contains expected fields.
💡 Enhanced validation example
func testEncodeJsonProducesValidJSON() throws { let profile = Profile(name: "jsonuser", display_name: "JSON User") guard let json = encode_json(profile) else { XCTFail("Failed to encode profile to JSON") return } XCTAssertFalse(json.isEmpty) XCTAssertTrue(json.hasPrefix("{")) XCTAssertTrue(json.hasSuffix("}")) + + // Validate JSON is well-formed by attempting decode + guard let jsonData = json.data(using: .utf8), + let decoded = try? JSONSerialization.jsonObject(with: jsonData) as? [String: Any] else { + XCTFail("JSON is not well-formed") + return + } + + XCTAssertEqual(decoded["name"] as? String, "jsonuser") + XCTAssertEqual(decoded["display_name"] as? String, "JSON User") }damusTests/RelayConnectionTests.swift (1)
87-117: Consider modernizing to async/await Task.sleep.The test correctly validates backoff reset behavior, but could be simplified and more readable using structured concurrency instead of nested DispatchQueue.main.asyncAfter callbacks.
💡 Async/await refactor example
func testConnectResetsBackoff() { // First, simulate an error to increase backoff connection.connect() mockSocket.simulateError(URLError(.networkConnectionLost)) - // Wait for backoff to be applied - let backoffExpectation = expectation(description: "Backoff increased") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - backoffExpectation.fulfill() - } - waitForExpectations(timeout: 1.0) + // Wait for backoff to be applied + try await Task.sleep(for: .milliseconds(100)) // Now connect successfully mockSocket.reset() connection.connect(force: true) - let connectExpectation = expectation(description: "Connected") - connection.$isConnected - .dropFirst() - .sink { isConnected in - if isConnected { - connectExpectation.fulfill() - } - } - .store(in: &cancellables) mockSocket.simulateConnect() - waitForExpectations(timeout: 1.0) + try await Task.sleep(for: .milliseconds(100)) XCTAssertEqual(connection.backoff, 1.0, "Backoff should reset to 1.0 after successful connection") }Note: You'd need to mark the test as
async throwsand use Swift concurrency patterns throughout.damus/Core/Nostr/RelayConnection.swift (1)
126-132: Disconnect implementation correctly uses protocol.The explicit
closeCodeandreasonparameters align with the WebSocketProtocol signature and provide better control over connection closure.Optional: Remove extra blank line
The extra blank line at 129 could be removed for consistency:
func disconnect() { socket.disconnect(closeCode: .normalClosure, reason: nil) subscriptionToken = nil - isConnected = false isConnecting = false }.github/workflows/network-tests.yml (2)
3-20: Consider CI cost and runtime for network-throttled tests.Network-throttled tests will be significantly slower than regular tests. Running them on every push and PR to master could increase CI costs and feedback time.
Consider:
- Running only on master pushes by default (not PRs)
- Using workflow path filters to run only when networking code changes
- Adding a label-based trigger (e.g.,
run-network-tests)Example with path filters:
on: push: branches: [master] paths: - 'damus/Core/Nostr/**' - 'damusTests/**NetworkTests.swift' workflow_dispatch: inputs: network_profile: # ... existing inputs
48-56: Add health check for strfry relay readiness.The fixed 5-second sleep may not be sufficient if Docker needs to pull the image or if the relay takes longer to start. A health check would make the workflow more reliable.
Replace sleep with health check
- name: Start strfry test relay run: | # Start strfry with permissive config (no write policy restrictions) docker run -d -p 7777:7777 \ -v ${{ github.workspace }}/test/strfry-test.conf:/etc/strfry.conf \ --name strfry-test ghcr.io/dockur/strfry - # Wait for relay to be ready - sleep 5 + # Wait for relay to be ready (with health check) + for i in {1..30}; do + if curl -f -s http://localhost:7777 >/dev/null 2>&1 || nc -z localhost 7777; then + echo "Relay is ready" + break + fi + echo "Waiting for relay to start... ($i/30)" + sleep 1 + done echo "strfry test relay started on ws://localhost:7777"damusTests/PostBoxTests.swift (2)
95-116: Test uses Task.sleep which may cause flakiness.The 100ms sleep on Line 103 is used to wait for async message delivery. While this works, it can cause flakiness if the system is under load.
Consider using polling or expectation fulfillment instead:
func testEventSentToRelay() async throws { guard let event = makeTestEvent() else { XCTFail("Failed to create test event") return } await postbox.send(event, to: [testRelayURL]) // Poll for message with timeout let deadline = Date().addingTimeInterval(1.0) while mockSocket.sentMessages.isEmpty && Date() < deadline { try await Task.sleep(for: .milliseconds(10)) } XCTAssertGreaterThan(mockSocket.sentMessages.count, 0, "Should have sent message to relay") if let sentMessage = mockSocket.sentMessages.first { if case .string(let str) = sentMessage { XCTAssertTrue(str.contains("EVENT"), "Message should be an EVENT") } else { XCTFail("Expected string message") } } }This approach is more resilient to timing variations.
246-274: Consider using expectation to verify callback fires only once.The test relies on sleep and then checks the count, but an inverted expectation with
isInverted = truewould be more robust for verifying the callback doesn't fire a second time.Use expectation for more robust test
func testOnFlushOnceFiresOnlyOnce() async throws { let relay2URL = RelayURL("wss://relay2.test.com")! let mockSocket2 = MockWebSocket() let descriptor2 = RelayPool.RelayDescriptor(url: relay2URL, info: .readWrite) try await pool.add_relay(descriptor2, webSocket: mockSocket2) mockSocket2.simulateConnect() try await Task.sleep(for: .milliseconds(100)) guard let event = makeTestEvent() else { XCTFail("Failed to create test event") return } var callbackCount = 0 + let firstCallExpectation = XCTestExpectation(description: "First callback fires") + let secondCallExpectation = XCTestExpectation(description: "Second callback should not fire") + secondCallExpectation.isInverted = true await postbox.send(event, to: [testRelayURL, relay2URL], on_flush: .once({ _ in callbackCount += 1 + if callbackCount == 1 { + firstCallExpectation.fulfill() + } else { + secondCallExpectation.fulfill() + } })) // Both relays respond simulateOKResponse(eventId: event.id) let result2 = CommandResult(event_id: event.id, ok: true, msg: "") postbox.handle_event(relay_id: relay2URL, .nostr_event(.ok(result2))) - try await Task.sleep(for: .milliseconds(50)) + await fulfillment(of: [firstCallExpectation, secondCallExpectation], timeout: 1.0) XCTAssertEqual(callbackCount, 1, ".once callback should fire only once") }.beads/websocket-testing-approaches.md (1)
1-1031: Excellent comprehensive testing documentation!This document provides thorough analysis of multiple testing approaches with clear tradeoffs, implementation examples, and phased rollout plans. It serves as valuable context for understanding the WebSocket testing infrastructure changes in this PR.
Optional: Fix markdown linting issues
Static analysis flagged several minor markdown issues:
- Missing language specifiers on some fenced code blocks (lines 14, 21, 200, 345, 394)
- Some tables need blank lines before/after (lines 353, 610, 618, 624, 728)
- Emphasis used instead of headings (lines 266, 287, 298)
These are purely stylistic and don't affect readability, but could be fixed for consistency if desired.
damusTests/EventNetworkTests.swift (1)
1196-1199: Weak test assertion – consider expanding coverage.This test only verifies that
ndbis not nil, which is already guaranteed by thesetUpmethod. Consider either removing this test or expanding it to verify actual profile caching behavior.damusTests/RelayIntegrationTests.swift (1)
496-521: Heuristic may not reliably detect throttling.This helper always waits 5 seconds via
Task.sleepregardless of actual connection time, so the elapsed time will always be ~5+ seconds. The checkelapsed > 3.0will essentially always return true. Consider measuring just the connection attempt duration, or using a different approach.🔎 Suggested improvement
func isNetworkThrottled() async -> Bool { let startTime = Date() // Try a quick connection let testPool = RelayPool(ndb: Ndb.test, keypair: keypair) let descriptor = RelayPool.RelayDescriptor( url: Self.publicRelayURL, info: .readWrite ) do { try await testPool.add_relay(descriptor) await testPool.connect() - // Wait for connection - try? await Task.sleep(for: .seconds(5)) + // Poll for connection with short intervals + for _ in 0..<10 { + try? await Task.sleep(for: .milliseconds(300)) + if await testPool.num_connected >= 1 { + break + } + } await testPool.disconnect() } catch { // Ignore errors } let elapsed = Date().timeIntervalSince(startTime) // If connection took > 3 seconds, likely throttled return elapsed > 3.0 }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.beads/.gitignore.beads/README.md.beads/beads-backup/.gitignore.beads/beads-backup/README.md.beads/beads-backup/config.yaml.beads/beads-backup/interactions.jsonl.beads/beads-backup/issues.jsonl.beads/beads-backup/metadata.json.beads/config.yaml.beads/interactions.jsonl.beads/issues.jsonl.beads/metadata.json.beads/websocket-testing-approaches.md.github/workflows/network-tests.ymldamus.xcodeproj/project.pbxprojdamus/Core/Nostr/RelayConnection.swiftdamus/Core/Nostr/RelayPool.swiftdamus/Core/Nostr/WebSocket.swiftdamusTests/DirectMessageTests.swiftdamusTests/EventHolderTests.swiftdamusTests/EventNetworkTests.swiftdamusTests/Mocking/MockWebSocket.swiftdamusTests/PostBoxTests.swiftdamusTests/ProfileNetworkTests.swiftdamusTests/RelayConnectionTests.swiftdamusTests/RelayIntegrationTests.swiftdamusTests/RelayPoolTests.swiftdamusTests/SearchNetworkTests.swiftdamusTests/ZapNetworkTests.swiftscripts/throttle-network.shtest/strfry-test.conf
🧰 Additional context used
📓 Path-based instructions (1)
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
**/*.swift: Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Favor Swift-first solutions that lean on nostrdb types (Ndb, NdbNote, iterators) before introducing new storage mechanisms
Ensure docstring coverage for any code added, or modified
Ensure nevernesting: favor early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers of if statements
Files:
damusTests/ProfileNetworkTests.swiftdamusTests/PostBoxTests.swiftdamusTests/EventNetworkTests.swiftdamus/Core/Nostr/RelayConnection.swiftdamusTests/Mocking/MockWebSocket.swiftdamusTests/EventHolderTests.swiftdamusTests/RelayConnectionTests.swiftdamusTests/DirectMessageTests.swiftdamus/Core/Nostr/RelayPool.swiftdamusTests/SearchNetworkTests.swiftdamusTests/RelayIntegrationTests.swiftdamusTests/ZapNetworkTests.swiftdamus/Core/Nostr/WebSocket.swiftdamusTests/RelayPoolTests.swift
🧠 Learnings (4)
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Add or update unit tests in damusTests/ alongside feature changes, especially when touching parsing, storage, or replay logic
Applied to files:
damusTests/EventNetworkTests.swift.beads/websocket-testing-approaches.mddamusTests/DirectMessageTests.swiftdamusTests/SearchNetworkTests.swiftdamusTests/RelayIntegrationTests.swift
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to **/*.swift : Ensure docstring coverage for any code added, or modified
Applied to files:
damus.xcodeproj/project.pbxproj
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Ensure new targets or resources integrate cleanly with the damus.xcodeproj main scheme
Applied to files:
damus.xcodeproj/project.pbxproj
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to **/*.swift : Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Applied to files:
damus.xcodeproj/project.pbxproj
🧬 Code graph analysis (9)
damusTests/EventNetworkTests.swift (7)
damusTests/Mocking/MockWebSocket.swift (2)
simulateConnect(117-119)simulateDisconnect(125-127)damus/Core/Nostr/NostrEvent.swift (2)
make_like_event(491-513)make_boost_event(472-489)damus/Notify/LikedNotify.swift (1)
liked(22-24)damus/Features/Chat/Models/ThreadModel.swift (2)
contains(270-272)get(280-282)nostrdb/NdbTagElem.swift (1)
id(128-131)damus/Shared/Utilities/EventHolder.swift (2)
set_should_queue(19-21)flush(82-104)nostrdb/NdbNote.swift (1)
direct_replies(538-540)
damus/Core/Nostr/RelayConnection.swift (3)
damus/Core/Nostr/RelayPool.swift (1)
disconnect(238-245)damus/Core/Nostr/WebSocket.swift (1)
disconnect(87-102)damusTests/Mocking/MockWebSocket.swift (1)
disconnect(88-94)
damusTests/EventHolderTests.swift (1)
damus/Shared/Utilities/EventHolder.swift (2)
set_should_queue(19-21)flush(82-104)
damusTests/RelayConnectionTests.swift (2)
damus/Core/Nostr/RelayConnection.swift (5)
connect(102-124)disconnect(126-132)send_raw(138-140)ping(82-100)disablePermanently(134-136)damusTests/Mocking/MockWebSocket.swift (7)
connect(81-86)simulateConnect(117-119)simulateError(131-133)reset(194-206)disconnect(88-94)simulateDisconnect(125-127)ping(102-112)
damusTests/SearchNetworkTests.swift (2)
damus/Features/Search/Models/SearchModel.swift (3)
tag_is_hashtag(94-97)event_matches_hashtag(85-92)event_matches_filter(99-104)damus/Shared/Utilities/EventHolder.swift (2)
set_should_queue(19-21)flush(82-104)
damusTests/RelayIntegrationTests.swift (1)
damusTests/AuthIntegrationTests.swift (2)
set_message_received_function(201-203)set_message_sent_function(205-207)
damusTests/ZapNetworkTests.swift (4)
damusTests/ProfileNetworkTests.swift (2)
setUp(23-37)tearDown(39-46)damusTests/Mocking/MockWebSocket.swift (2)
simulateConnect(117-119)simulateDisconnect(125-127)nostrdb/NdbNote.swift (1)
last_refid(551-553)damus/Core/Nostr/NostrEvent.swift (1)
encode_json(358-362)
damus/Core/Nostr/WebSocket.swift (2)
damus/Core/Nostr/RelayConnection.swift (4)
connect(102-124)disconnect(126-132)send(142-156)ping(82-100)damusTests/Mocking/MockWebSocket.swift (4)
connect(81-86)disconnect(88-94)send(96-100)ping(102-112)
damusTests/RelayPoolTests.swift (2)
damus/Core/Nostr/RelayPool.swift (4)
add_relay(171-188)close(55-64)connect(229-236)count_queued(377-386)damusTests/Mocking/MockWebSocket.swift (3)
simulateConnect(117-119)simulateError(131-133)simulateDisconnect(125-127)
🪛 LanguageTool
.beads/websocket-testing-approaches.md
[uncategorized] ~627-~627: The official name of this software platform is spelled with a capital “H”.
Context: .../TestRelay/| New directory | +500 | |.github/workflows/tests.yml` | Add relay servic...
(GITHUB)
[grammar] ~657-~657: Ensure spelling is correct
Context: ...ivity testing requires: - Latency: 100-500ms round trip - Bandwidth limits: 780 Kbps...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~917-~917: The operating system from Apple is written “macOS”.
Context: ...itespeedio/throttle 2. GitHub Actions macos runners: → Yes, sudo works. Implement...
(MAC_OS)
🪛 markdownlint-cli2 (0.18.1)
.beads/websocket-testing-approaches.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
287-287: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
298-298: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
345-345: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
353-353: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
394-394: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
610-610: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
618-618: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
624-624: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
728-728: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Shellcheck (0.11.0)
scripts/throttle-network.sh
[warning] 59-65: check_root references arguments, but none are ever passed.
(SC2120)
🔇 Additional comments (68)
.beads/beads-backup/README.md (1)
1-81: Verify external URLs and clarify unusual documentation placement.The README documentation is well-written and clearly formatted. However:
Unusual placement: This primary README is located in
.beads/beads-backup/, which is conventionally a backup/restore location. Consider placing it at.beads/README.mdor documenting why it belongs in the backup subdirectory.External URL verification: The file references several external URLs (lines 9, 75-77, 64) pointing to
github.com/steveyegge/beads. Please verify these links are accessible and current..beads/metadata.json (1)
1-4: Beads infrastructure is intentional and directly relevant to the relay testing objectives in this PR.The
.beads/directory contains AI-native issue tracking and the comprehensivewebsocket-testing-approaches.mddocumentation (1031 lines) that directly addresses the PR's stated goals: relay integration tests, network condition testing, and NIP-01 protocol compliance. The same commit (8567e73) that added Beads also fixed RelayIntegrationTests and added.github/workflows/network-tests.yml, indicating this is coordinated infrastructure work for testing.The document covers:
- NIP-01 Nostr protocol specification
- Six WebSocket testing approaches (mock protocols, callback injection, local relay servers, network conditioning, etc.)
- CI integration with system-level network throttling using
sitespeedio/throttle- Phased implementation plan for relay connection mocking
No scope issue exists—this is an intentional part of the relay testing infrastructure.
Regarding the minor point: add a trailing newline to
.beads/metadata.json(line 4) to follow Unix file convention.Likely an incorrect or invalid review comment.
.beads/README.md (1)
1-81: Note: These Beads infrastructure files appear orthogonal to the stated PR objectives.The README is well-written and accurately documents the Beads tool. However, the PR summary emphasizes relay integration tests, WebSocket abstraction, network throttling, and CI workflows—none of which appear directly related to issue tracking infrastructure. Confirm that adding Beads setup is intentional and aligns with team standards for this project.
.beads/.gitignore (1)
1-34: Well-structured gitignore configuration.The patterns correctly ignore runtime artifacts (databases, daemon files, merge conflicts) while preserving the JSONL exports and config that serve as the source of truth. The structure and comments are clear.
.beads/config.yaml (1)
1-62: Well-documented configuration template.The configuration file provides clear documentation for all Beads CLI options with appropriate examples and notes about environment variable overrides. All settings are commented out, serving as a reference template.
damusTests/EventHolderTests.swift (7)
1-14: Good test structure with proper docstrings and MainActor annotation.Test class follows coding guidelines with docstring coverage and uses
@MainActorappropriately for thread-safe EventHolder testing.
18-24: Helper method is well-designed with appropriate defaults.Good use of optional createdAt parameter with a sensible default. The docstring clearly describes its purpose.
28-43: Test correctly verifies immediate insertion behavior.Good use of guard-let early return pattern per coding guidelines.
62-83: Flush test properly validates queue-to-events transfer.The test correctly verifies that events move from
incomingtoeventsarray after flush.
146-166: Ordering test validates sorted insertion correctly.Good test of the newest-first ordering behavior. The test inserts events in reverse chronological order and verifies they're sorted correctly.
170-204: Callback tests provide complete coverage for on_queue behavior.Both positive (callback fires in queued mode) and negative (callback doesn't fire in immediate mode) cases are covered.
281-304: Comprehensive queued count tracking test.The test verifies count accuracy through insertion and flush operations, providing good coverage of the
queuedproperty..beads/beads-backup/config.yaml (1)
1-62: Backup configuration mirrors primary config.This backup configuration file appropriately mirrors
.beads/config.yamlfor the beads backup workflow..beads/beads-backup/issues.jsonl (1)
1-16: Issue tracking data file with comprehensive project history.The JSONL file contains well-structured issue tracking entries for the network testing and image upload work. No sensitive data is exposed.
test/strfry-test.conf (1)
1-31: Appropriate permissive configuration for CI relay testing.The strfry configuration correctly sets up a permissive relay for integration testing with
writePolicy.plugin = ""to accept all events. The binding to0.0.0.0:7777aligns with the CI workflow andRelayIntegrationTestsexpectations..beads/issues.jsonl (1)
1-40: Comprehensive issue tracking for relay testing infrastructure.The JSONL file provides thorough tracking of the network testing work with well-structured issues, dependencies, and detailed progress notes. The architecture investigation notes (e.g., Local Relay Model in damus-cfl) serve as valuable documentation.
damus.xcodeproj/project.pbxproj (2)
14-21: Test infrastructure additions look well-structured.The new test files and mock utilities are properly integrated:
- PBXFileReference entries include
includeInIndex = 1for proper indexing- Corresponding PBXBuildFile entries correctly reference the file references
- Files are organized into appropriate groups (damusTests, Mocking, Utilities)
- Test files are added to the test target's Sources build phase
The consistent addition of
NetworkConditionSimulator.swiftacross multiple targets (main app, share extension, highlighter extension) is appropriate for a utility needed in different contexts.Also applies to: 502-502, 626-640, 1084-1094, 1116-1116, 2009-2009, 2135-2137, 2447-2447, 2707-2709, 2719-2723, 2736-2736, 2746-2746, 2752-2752, 2900-2900, 2915-2915, 3918-3930, 5237-5238, 6380-6394
8449-8451: Package reference rename is consistently applied.The
secp256k1package reference rename tosecp256k1.swiftis applied uniformly across:
XCRemoteSwiftPackageReferencedeclaration- All
XCSwiftPackageProductDependencyentriesThis maintains project integrity. Based on learnings, ensure this change integrates cleanly with the main scheme.
What is the correct repository name for the secp256k1 Swift package by jb55?Also applies to: 8545-8546, 8550-8551, 8575-8576, 8620-8621
damusTests/ProfileNetworkTests.swift (7)
23-37: LGTM! Good async setup pattern.The setup correctly initializes the test environment with dependency injection. The 100ms sleep allows time for async connection state to settle before tests run.
39-46: LGTM! Proper test cleanup.Correctly closes resources and clears state to prevent test interference.
48-53: LGTM! Useful test helper.This method reduces duplication and makes test intent clearer.
58-69: LGTM! Clean test structure.Good use of guard clause for early return and clear assertions.
173-194: LGTM! Solid async test pattern.The test properly validates message delivery and handles JSON formatting variations.
236-257: LGTM! Good multi-relay test coverage.Validates that PostBox correctly tracks events sent to multiple relays.
262-283: LGTM! Proper async callback testing.Good use of XCTest expectations to validate callback invocation.
scripts/throttle-network.sh (5)
19-21: LGTM! Good error handling and namespacing.The
set -eensures the script fails fast on errors, and the named anchor prevents conflicts with other PF rules.
23-32: LGTM! Comprehensive network profile definitions.The profiles cover a good range of network conditions from excellent (LTE) to very poor, enabling realistic testing scenarios.
67-105: LGTM! Robust throttling setup with proper cleanup.The function validates input, cleans up existing state, and correctly configures dummynet and PF for bidirectional traffic throttling.
107-117: LGTM! Clean throttling teardown.Properly cleans up both PF and dummynet state, with appropriate error suppression for idempotent cleanup.
119-130: LGTM! Helpful diagnostic output.Provides comprehensive status information for troubleshooting network throttling issues.
damusTests/Mocking/MockWebSocket.swift (5)
12-37: LGTM! Excellent documentation.The comprehensive docstring with usage examples makes this test utility easy to understand and use correctly.
38-77: LGTM! Well-designed test utility with proper encapsulation.Good use of
private(set)for test observation properties and NSLock for thread safety in async test scenarios.
81-112: LGTM! Thread-safe protocol implementation.Consistent use of locking ensures thread safety across all WebSocket operations, critical for reliable async testing.
116-179: LGTM! Comprehensive simulation helpers.The simulation methods cover all WebSocket lifecycle events and Nostr-specific messages, significantly reducing test boilerplate.
181-206: LGTM! Proper test lifecycle support.The
reset()method ensures clean state between tests, andrespondToPing()provides deterministic ping response handling.damusTests/RelayConnectionTests.swift (6)
24-41: LGTM! Clean test setup with proper dependency injection.Good use of weak self in closures to avoid retain cycles, and clear event capture for test assertions.
52-56: LGTM! Clear initial state validation.Simple, focused test that establishes baseline expectations for connection state.
200-231: LGTM! Validates exponential backoff correctly.The test properly validates that backoff doubles with each error (1 → 2 → 4 → 8), confirming exponential behavior.
233-251: LGTM! Important error filtering validation.Correctly validates that benign errors (like "socket not connected") don't trigger unnecessary reconnection attempts.
379-407: LGTM! Valuable real-world scenario testing.Tests the connection's resilience to intermittent disconnects common in mobile networks, ensuring proper recovery behavior.
410-433: LGTM! Good stress test for connection robustness.Validates that the connection state machine handles rapid disconnects gracefully without crashes or inconsistent state.
damus/Core/Nostr/RelayConnection.swift (2)
65-80: Excellent dependency injection implementation!The protocol-based dependency injection is well-implemented and maintains backward compatibility through the default parameter. The documentation is clear and helpful.
57-57: Protocol-based socket property looks good.The change from a lazy concrete WebSocket to an injected WebSocketProtocol is correct and enables testability.
.github/workflows/network-tests.yml (1)
98-105: Matrix job configuration is well-designed.The matrix job running only on master pushes is a good balance between coverage and CI cost. The
fail-fast: falsestrategy ensures all network profiles are tested even if one fails.damusTests/PostBoxTests.swift (4)
22-47: Test setup and teardown are well-structured.The async setup properly initializes the test environment with a mock socket and simulates connection. The teardown ensures proper cleanup.
225-243: Callback test properly uses XCTestExpectation.The use of
XCTestExpectationwith timeout is the right approach for testing async callbacks. Good verification of callback count.
309-326: Delayed send test efficiently verifies behavior without waiting.The test correctly verifies that delayed events are queued but not immediately sent, without actually waiting for the 5-second delay. This keeps tests fast.
389-402: Test validates backoff algorithm correctly.The test verifies the exponential backoff behavior (10 * 1.5 = 15). While this couples the test to the implementation, it's appropriate for validating retry logic.
damus/Core/Nostr/WebSocket.swift (4)
11-38: WebSocketProtocol is well-designed and documented.The protocol provides a clean abstraction for WebSocket operations with comprehensive documentation. The AnyObject constraint is appropriate for reference semantics, and the method signatures support the full range of WebSocket operations needed for testing.
42-60: Description property improves observability.The computed description property provides useful logging information for connection events. Returning nil for message events to avoid noise is a pragmatic choice.
62-72: WebSocket conforms to protocol correctly.The WebSocket class correctly implements WebSocketProtocol. The
subjectproperty must be public to satisfy the protocol requirements. While this exposes the internal PassthroughSubject, it's a necessary tradeoff for dependency injection, and observers cannot improperly inject events.
87-102: Default parameters improve ergonomics.Adding default parameters to the
disconnectmethod improves call-site ergonomics while maintaining protocol compliance. The implementation correctly handles the optional reason parameter and emits the disconnected event with properly converted reason string.damusTests/EventNetworkTests.swift (7)
16-165: LGTM! Well-structured reaction network tests.The test class correctly covers reaction (kind 7) event publishing, OK handling, disconnect queueing, custom emoji content, and multi-relay scenarios. The early return pattern with guard statements keeps control flow flat per coding guidelines.
170-302: LGTM! Comprehensive boost event tests.Good coverage of boost (kind 6) creation, relay delivery, OK handling, and tag verification. The
testBoostContentContainsOriginalEventandtestBoostIncludesProperTagstests provide valuable NIP-18 compliance verification.
307-461: LGTM! Good follow list network test coverage.Correctly tests kind 3 follow list events including multi-pubkey scenarios and multi-relay redundancy. The helper method
makeFollowListEventproperly constructs p-tags for the follow list.
466-683: LGTM! Mute and bookmark list tests follow consistent patterns.Both test classes correctly verify NIP-51 list events (kind 10000 for mute, kind 10003 for bookmarks) with appropriate tag structures (p-tags for mute, e-tags for bookmarks).
688-883: LGTM! NotificationsModel tests cover key behaviors.Good coverage of event queuing, flushing, deduplication, reaction grouping, time-based sorting, and pubkey extraction. The
@MainActorannotation is appropriate for this model that likely publishes changes to the main thread.
888-1064: LGTM! Thorough ThreadEventMap and filter tests.Good coverage of thread data structures including event map operations, chronological sorting, reply hierarchy tracking, and graceful handling of missing parents. The filter tests verify correct NostrFilter construction for thread-related queries.
1069-1238: LGTM! Profile and filter tests are well-structured.Good coverage of NostrFilter construction for profile metadata (kind 0), relay lists (kind 10002), and contacts (kind 3). The metadata event creation tests verify proper JSON content generation.
damusTests/SearchNetworkTests.swift (3)
16-183: LGTM! Comprehensive search filter tests.Good coverage of hashtag detection, event matching, and filter creation. The case-sensitivity test at line 155 correctly documents the actual behavior rather than assuming case-insensitive matching.
188-329: LGTM! EventHolder behavior tests are thorough.Good coverage of deduplication, time-based sorting (newest-first), batch insertion, queue/flush mechanics, and the
on_queuecallback for preloading. The@MainActorannotation is appropriate for this observable object.
334-475: LGTM! Filter and event kind tests are well-organized.Good coverage of search filter construction with limits, hashtag filters, combined filters, and the
is_textlikeproperty for various event kinds (text, longform, highlight, reaction, metadata).damusTests/RelayPoolTests.swift (1)
95-323: LGTM! Multi-relay coordination tests are well-designed.Comprehensive coverage of multi-relay scenarios including connection management, partial failures, message routing, queue behavior, and reconnection handling. The helper methods
addMockRelayandconnectAllMockRelaysreduce boilerplate effectively.damusTests/DirectMessageTests.swift (2)
14-408: LGTM! Comprehensive DM and NIP-04 test coverage.Excellent coverage of NIP-04 encryption (round-trip, wrong key, empty, unicode), DM event structure (kind 4, p-tags), DirectMessageModel behavior (is_request logic), and DirectMessagesModel operations (lookup, create, filtering). The tests verify both cryptographic correctness and model logic.
413-796: LGTM! Thorough DM PostBox integration tests.Excellent coverage of DM delivery scenarios including multi-relay broadcasting, disconnect queueing, reconnection retry, delayed sending, cancellation, callback firing, rejection handling, and encrypted content preservation through the queue lifecycle.
damusTests/RelayIntegrationTests.swift (1)
28-488: LGTM! Well-designed relay integration tests.Good coverage of NIP-01 protocol flows (EVENT, REQ, EOSE, CLOSE, OK) with proper fallback from local strfry to public relay. The
FORCE_PUBLIC_RELAYenvironment variable support is helpful for local development. The throttled network tests with extended timeouts (60s) appropriately account for degraded conditions.damusTests/ZapNetworkTests.swift (2)
14-358: LGTM! Comprehensive zap request and PostBox tests.Good coverage of zap request creation (kind 9734, p/e-tags, zap types), PostBox integration (send, OK, disconnect, multi-relay), and receipt parsing. The tests correctly handle the
potentially_anon_outer_requestwrapper for different zap types.
363-389: LGTM! ZapType enum tests verify expected behavior.Good coverage of
ZapTyperaw values, string conversion methods, and the failable string initializer including the invalid input case.
| - name: Run Tests under throttled network | ||
| run: | | ||
| xcodebuild test \ | ||
| -scheme damus \ | ||
| -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.2' \ | ||
| -resultBundlePath NetworkTestResults.xcresult \ | ||
| -only-testing:damusTests/UploadRetryTests \ | ||
| -only-testing:damusTests/RelayConnectionTests \ | ||
| -only-testing:damusTests/RelayIntegrationTests \ | ||
| CODE_SIGNING_ALLOWED=NO \ | ||
| | xcpretty --color |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout to prevent hanging tests under throttled network.
Network-throttled tests could hang indefinitely if connection issues occur. Consider adding a timeout to the xcodebuild command.
Add timeout parameter
- name: Run Tests under throttled network
run: |
xcodebuild test \
-scheme damus \
-destination 'platform=iOS Simulator,name=iPhone 15,OS=17.2' \
-resultBundlePath NetworkTestResults.xcresult \
-only-testing:damusTests/UploadRetryTests \
-only-testing:damusTests/RelayConnectionTests \
-only-testing:damusTests/RelayIntegrationTests \
CODE_SIGNING_ALLOWED=NO \
+ -test-timeouts-enabled YES \
+ -default-test-execution-time-allowance 600 \
| xcpretty --colorThis sets a 10-minute timeout per test, which should be sufficient for throttled conditions while preventing infinite hangs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run Tests under throttled network | |
| run: | | |
| xcodebuild test \ | |
| -scheme damus \ | |
| -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.2' \ | |
| -resultBundlePath NetworkTestResults.xcresult \ | |
| -only-testing:damusTests/UploadRetryTests \ | |
| -only-testing:damusTests/RelayConnectionTests \ | |
| -only-testing:damusTests/RelayIntegrationTests \ | |
| CODE_SIGNING_ALLOWED=NO \ | |
| | xcpretty --color | |
| - name: Run Tests under throttled network | |
| run: | | |
| xcodebuild test \ | |
| -scheme damus \ | |
| -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.2' \ | |
| -resultBundlePath NetworkTestResults.xcresult \ | |
| -only-testing:damusTests/UploadRetryTests \ | |
| -only-testing:damusTests/RelayConnectionTests \ | |
| -only-testing:damusTests/RelayIntegrationTests \ | |
| CODE_SIGNING_ALLOWED=NO \ | |
| -test-timeouts-enabled YES \ | |
| -default-test-execution-time-allowance 600 \ | |
| | xcpretty --color |
🤖 Prompt for AI Agents
In @.github/workflows/network-tests.yml around lines 65 - 75, The xcodebuild
test step ("Run Tests under throttled network") can hang under throttled
networks; prefix the xcodebuild invocation with a timeout (e.g., timeout 10m or
gtimeout 10m) so the command is killed after 10 minutes, e.g. change the run
block that calls xcodebuild test (the command that writes
NetworkTestResults.xcresult and pipes to xcpretty) to be executed under a
timeout wrapper to prevent infinite hanging.
| D73E5EFC2C6A97F4007EB227 /* DamusAppNotificationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D78CD5972B8990300014D539 /* DamusAppNotificationView.swift */; }; | ||
| D73E5EFD2C6A97F4007EB227 /* InnerTimelineView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CE0E2B529A3ED5500DB4CA2 /* InnerTimelineView.swift */; }; | ||
| D73E5EFE2C6A97F4007EB227 /* (null) in Sources */ = {isa = PBXBuildFile; }; | ||
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */ = {isa = PBXBuildFile; }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if this UUID exists and in what context
rg -n "D73E5EFE2C6A97F4007EB227" damus.xcodeproj/project.pbxproj
echo "---"
# Check the file size and read context around line 1512
wc -l damus.xcodeproj/project.pbxproj
echo "---"
# Read around line 1512
sed -n '1505,1520p' damus.xcodeproj/project.pbxproj
echo "---"
# Read around line 7244
sed -n '7237,7252p' damus.xcodeproj/project.pbxprojRepository: damus-io/damus
Length of output: 3792
Remove malformed PBXBuildFile entry missing fileRef at line 1512.
The build file entry at line 1512 is missing the required fileRef property:
D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */ = {isa = PBXBuildFile; };
All adjacent entries properly reference files (e.g., fileRef = <UUID> /* FileName.swift */;). This orphaned entry should be removed or corrected if it was meant to reference a specific file.
🤖 Prompt for AI Agents
In @damus.xcodeproj/project.pbxproj at line 1512, The PBXBuildFile entry with
UUID D73E5EFE2C6A97F4007EB227 is malformed (it lacks a required fileRef) and
should be fixed; either remove the entire PBXBuildFile entry for
D73E5EFE2C6A97F4007EB227 from the project.pbxproj or restore it by adding the
correct fileRef (e.g., fileRef = <UUID> /* <Filename> */;) that matches the
intended source file so the PBXBuildFile object is valid.
| D73E5EFC2C6A97F4007EB227 /* DamusAppNotificationView.swift in Sources */, | ||
| D73E5EFD2C6A97F4007EB227 /* InnerTimelineView.swift in Sources */, | ||
| D73E5EFE2C6A97F4007EB227 /* (null) in Sources */, | ||
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */, |
There was a problem hiding this comment.
Duplicate malformed BuildFile entry.
Same issue as line 1512 — this is a second reference to the malformed D73E5EFE2C6A97F4007EB227 build file in a different build phase. Both occurrences should be addressed together.
🤖 Prompt for AI Agents
In @damus.xcodeproj/project.pbxproj at line 7244, There is a duplicate/malformed
PBXBuildFile entry with ID D73E5EFE2C6A97F4007EB227 referenced in multiple build
phases; locate all references to D73E5EFE2C6A97F4007EB227 in project.pbxproj
(e.g., the PBXSourcesBuildPhase and the other BuildPhase where it appears) and
remove the incorrect duplicate reference so the ID appears only once in the
appropriate PBXBuildFile list, and if necessary remove or correct the
corresponding PBXBuildFile block so the project has a single valid BuildFile
entry for that file.
| func testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) { | ||
| let ndb = Ndb()! | ||
| let relayPool = RelayPool(ndb: ndb) | ||
| let info = RelayInfo(read: true, write: true) | ||
| let info: NIP65.RelayList.RelayItem.RWConfiguration = .readWrite | ||
|
|
||
| Task { | ||
| do { | ||
| for relay in urls { | ||
| guard let url = RelayURL(relay) else { | ||
| XCTFail("Invalid URL encountered: \(relay)") | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| for relay in urls { | ||
| guard let url = RelayURL(relay) else { | ||
| XCTFail("Invalid URL encountered: \(relay)") | ||
| return | ||
| let descriptor = RelayPool.RelayDescriptor(url: url, info: info) | ||
| try await relayPool.add_relay(descriptor) | ||
| } | ||
|
|
||
| let descriptor = RelayDescriptor(url: url, info: info) | ||
| try relayPool.add_relay(descriptor) | ||
| if expectedError != nil { | ||
| XCTFail("Expected \(expectedError!) error, but no error was thrown.") | ||
| } | ||
| } catch let error as RelayPool.RelayError where expectedError == .RelayAlreadyExists { | ||
| XCTAssertEqual(error, expectedError!, "Expected RelayAlreadyExists error, got \(error)") | ||
| } catch { | ||
| XCTFail("An unexpected error was thrown: \(error)") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fire-and-forget Task block makes assertions unreliable.
The Task { } block is not awaited, so the test method returns immediately without waiting for the async code to execute. This means assertions inside the Task may never run, or run after XCTest considers the test complete. Either make the calling test methods async and use await, or use XCTestExpectation to wait for completion.
🔎 Proposed fix using async/await
-func testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) {
+func testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) async {
let ndb = Ndb()!
let relayPool = RelayPool(ndb: ndb)
let info: NIP65.RelayList.RelayItem.RWConfiguration = .readWrite
- Task {
- do {
- for relay in urls {
- guard let url = RelayURL(relay) else {
- XCTFail("Invalid URL encountered: \(relay)")
- return
- }
-
- let descriptor = RelayPool.RelayDescriptor(url: url, info: info)
- try await relayPool.add_relay(descriptor)
+ do {
+ for relay in urls {
+ guard let url = RelayURL(relay) else {
+ XCTFail("Invalid URL encountered: \(relay)")
+ return
}
- if expectedError != nil {
- XCTFail("Expected \(expectedError!) error, but no error was thrown.")
- }
- } catch let error as RelayPool.RelayError where expectedError == .RelayAlreadyExists {
- XCTAssertEqual(error, expectedError!, "Expected RelayAlreadyExists error, got \(error)")
- } catch {
- XCTFail("An unexpected error was thrown: \(error)")
+ let descriptor = RelayPool.RelayDescriptor(url: url, info: info)
+ try await relayPool.add_relay(descriptor)
}
+
+ if expectedError != nil {
+ XCTFail("Expected \(expectedError!) error, but no error was thrown.")
+ }
+ } catch let error as RelayPool.RelayError where expectedError == .RelayAlreadyExists {
+ XCTAssertEqual(error, expectedError!, "Expected RelayAlreadyExists error, got \(error)")
+ } catch {
+ XCTFail("An unexpected error was thrown: \(error)")
}
}Then update the calling tests to be async:
func testAddRelay_ValidRelayURL_NoErrors() async {
await testAddRelays(urls: ["wss://relay.damus.io"])
}🤖 Prompt for AI Agents
In @damusTests/RelayPoolTests.swift around lines 58 - 84, The test uses a
fire-and-forget Task so assertions run asynchronously and the test can finish
before they execute; change testAddRelays to be async (e.g., func
testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) async)
and remove the Task block, performing the awaits directly (e.g., await
relayPool.add_relay(descriptor)) so the caller can await it; also update all
callers (individual test methods) to be async and await testAddRelays (or
alternatively replace the Task with an XCTestExpectation and fulfill it on
completion), ensuring error handling stays the same for RelayPool.RelayError and
other catches.
| check_root() { | ||
| if [[ $EUID -ne 0 ]]; then | ||
| echo "Error: This script must be run with sudo" | ||
| echo " sudo $0 $*" | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Minor: check_root doesn't receive arguments for error message.
The function references $* on line 62 to show the user what command to run, but it's never passed arguments when called (lines 135, 139). This means the error message will be incomplete, showing sudo ./throttle-network.sh instead of sudo ./throttle-network.sh start 3g.
🔎 Proposed fix to pass arguments through
check_root() {
if [[ $EUID -ne 0 ]]; then
echo "Error: This script must be run with sudo"
echo " sudo $0 $*"
exit 1
fi
}Update the case statement to pass arguments:
case "${1:-}" in
start)
- check_root
+ check_root "$@"
start_throttle "$2"
;;
stop)
- check_root
+ check_root "$@"
stop_throttle
;;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 59-65: check_root references arguments, but none are ever passed.
(SC2120)
🤖 Prompt for AI Agents
In @scripts/throttle-network.sh around lines 59 - 65, The check_root function
currently references $* but the callers don't pass arguments, so the suggested
sudo usage is incomplete; update the case statement callers to invoke check_root
"$@" (preserving original CLI args) and also change the echo inside check_root
to use "$@" (or "$*") to correctly render the full command and its arguments in
the error message.
1 participant
Summary
Adds comprehensive relay integration tests and network condition testing infrastructure. Verifies NIP-01 protocol compliance and tests behavior under degraded network conditions (3G, slow connections).
Closes #3507
Commit Overview
writePolicy.plugin = ""accepts all eventsLessons Learned
1. iOS Simulator Can't Reach Docker localhost
ws://localhost:7777(strfry in Docker)2. Network Throttling with sitespeedio/throttle
3g,3gslow,edge,verybadsudo throttle --profile 3g/sudo throttle --stopscripts/throttle-network.shuses macOS dummynet directly3. Relay Configuration for Testing
test/strfry-test.confwithwritePolicy.plugin = ""(accept all)-v $(pwd)/test/strfry-test.conf:/etc/strfry.conf4. Async Testing Patterns
try await Task.sleep(for: .milliseconds(500))XCTFail(), to prevent cascading failuresTest Results
RelayIntegrationTests (8 tests) - All Pass
Network Test Coverage
Total: 137 new network-related tests
Running Throttled Tests Locally
Running with Local strfry Relay (CI)
Checklist
Signed off by alltheseas
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.