Conversation
nogringo
force-pushed
the
test/469-event-source-tracking
branch
from
ede4168 to
6172c98
Compare
📝 WalkthroughWalkthroughThree changes enhance mock relay testing infrastructure: a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NDK
participant MockRelay
participant Cache
participant EventVerifier
Client->>NDK: broadcast(event)
NDK->>EventVerifier: verify(event)
EventVerifier-->>NDK: verified
NDK->>MockRelay: EVENT message
MockRelay->>MockRelay: check bannedWord in content
alt Content contains banned word
MockRelay-->>NDK: OK(false, "blocked: content contains banned word")
NDK-->>Client: blocked
else Content allowed
MockRelay->>Cache: store event
MockRelay-->>NDK: OK(true)
NDK-->>Client: broadcastDone
end
sequenceDiagram
participant Client
participant NDK
participant Relay1
participant Relay2
participant Relay3
participant Cache
Client->>NDK: broadcast(event)
NDK->>Relay1: EVENT
NDK->>Relay2: EVENT
NDK->>Relay3: EVENT
Note over Relay1,Relay3: Some relays filter based on bannedWord
Relay1-->>NDK: OK(filtered)
Relay2-->>NDK: OK(accepted)
Relay3-->>NDK: OK(accepted)
NDK->>Cache: store event
Client->>NDK: request(event_id)
Cache-->>NDK: event with sources merged from Relay2, Relay3
NDK-->>Client: event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
packages/ndk/test/usecases/stream_response_cleaner/event_sources_merge_test.dart (1)
48-52: Consider usingaddTearDownfor robust cleanup.If the assertion on line 46 fails,
ndk.destroy()and the relaystopServer()calls won't execute, potentially leaving orphan servers. UsingaddTearDownensures cleanup runs regardless of test outcome.Proposed refactor using addTearDown
await relay1.startServer(); await relay2.startServer(); await relay3.startServer(); + addTearDown(() async { + await relay1.stopServer(); + await relay2.stopServer(); + await relay3.stopServer(); + }); final ndk = Ndk(NdkConfig( eventVerifier: MockEventVerifier(), cache: MemCacheManager(), bootstrapRelays: [relay1.url, relay2.url, relay3.url], )); + addTearDown(() => ndk.destroy()); // ... rest of test ... expect(events.first.sources.length, equals(2)); - - await ndk.destroy(); - await relay1.stopServer(); - await relay2.stopServer(); - await relay3.stopServer(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/test/usecases/stream_response_cleaner/event_sources_merge_test.dart` around lines 48 - 52, Replace the direct cleanup calls at the end of the test with an addTearDown registration so cleanup always runs even if assertions fail: register an async tearDown via addTearDown(() async { await ndk.destroy(); await relay1.stopServer(); await relay2.stopServer(); await relay3.stopServer(); }); — locate the cleanup calls around ndk.destroy() and relay*.stopServer() and move them into that addTearDown callback (ensuring the callback is registered before assertions run).packages/ndk/test/usecases/broadcast_sources_test.dart (1)
41-43: Consider usingaddTearDownfor robust cleanup.Same as the other test file—if assertions fail, cleanup won't execute. Consider wrapping
ndk.destroy()andrelay.stopServer()inaddTearDowncallbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/test/usecases/broadcast_sources_test.dart` around lines 41 - 43, The test currently calls await ndk.destroy() and await relay.stopServer() directly at the end, which can be skipped if assertions fail; instead register cleanup with addTearDown so they always run — e.g., in the test body call addTearDown(() async => await ndk.destroy()) and addTearDown(() async => await relay.stopServer()), ensuring you reference the existing ndk.destroy() and relay.stopServer() methods so teardown always executes even on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ndk/test/usecases/broadcast_sources_test.dart`:
- Line 9: The test name string has a typo: change the test declaration
test("braodcast should update source", ...) to test("broadcast should update
source", ...) so the test description reads correctly; update the string in the
test function in broadcast_sources_test.dart (the test call named test(...)
containing "braodcast should update source") to "broadcast should update
source".
- Around line 34-39: The test fails because relay OK responses for broadcasts
aren't updating the cached event's sources; modify the relay response handling
in RelayManager (the code path handling RelayBroadcastResponse in
_processDecodedMessage) to load the event from cache, append the relay URL to
its sources (if not present), and call cacheManager.saveEvent(updatedEvent) so
broadcasts update sources similar to _handleIncomingEvent; ensure you reference
the same event.id and avoid duplicating sources when saving.
In
`@packages/ndk/test/usecases/stream_response_cleaner/event_sources_merge_test.dart`:
- Line 14: The test has a copy-paste error where MockRelay instantiation for the
variable relay3 uses the name "relay 2"; update the MockRelay call for relay3 to
use the correct name (e.g., "relay 3") so the MockRelay(name: ...) for relay3 is
distinct from relay2 and improves test clarity and debugging.
---
Nitpick comments:
In `@packages/ndk/test/usecases/broadcast_sources_test.dart`:
- Around line 41-43: The test currently calls await ndk.destroy() and await
relay.stopServer() directly at the end, which can be skipped if assertions fail;
instead register cleanup with addTearDown so they always run — e.g., in the test
body call addTearDown(() async => await ndk.destroy()) and addTearDown(() async
=> await relay.stopServer()), ensuring you reference the existing ndk.destroy()
and relay.stopServer() methods so teardown always executes even on failures.
In
`@packages/ndk/test/usecases/stream_response_cleaner/event_sources_merge_test.dart`:
- Around line 48-52: Replace the direct cleanup calls at the end of the test
with an addTearDown registration so cleanup always runs even if assertions fail:
register an async tearDown via addTearDown(() async { await ndk.destroy(); await
relay1.stopServer(); await relay2.stopServer(); await relay3.stopServer(); }); —
locate the cleanup calls around ndk.destroy() and relay*.stopServer() and move
them into that addTearDown callback (ensuring the callback is registered before
assertions run).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52333132-7d7d-46f2-a5b9-c09c5b9229e2
📒 Files selected for processing (3)
packages/ndk/test/mocks/mock_relay.dartpackages/ndk/test/usecases/broadcast_sources_test.dartpackages/ndk/test/usecases/stream_response_cleaner/event_sources_merge_test.dart
| import '../mocks/mock_relay.dart'; | ||
|
|
||
| void main() async { | ||
| test("braodcast should update source", () async { |
There was a problem hiding this comment.
Typo in test name: "braodcast" should be "broadcast".
Proposed fix
- test("braodcast should update source", () async {
+ test("broadcast should update source", () async {📝 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.
| test("braodcast should update source", () async { | |
| test("broadcast should update source", () async { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/test/usecases/broadcast_sources_test.dart` at line 9, The test
name string has a typo: change the test declaration test("braodcast should
update source", ...) to test("broadcast should update source", ...) so the test
description reads correctly; update the string in the test function in
broadcast_sources_test.dart (the test call named test(...) containing "braodcast
should update source") to "broadcast should update source".
| await ndk.broadcast.broadcast(nostrEvent: event).broadcastDoneFuture; | ||
|
|
||
| final localEvent = await ndk.config.cache.loadEvent(event.id); | ||
|
|
||
| expect(localEvent, isNotNull); | ||
| expect(localEvent!.sources, isNotEmpty); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if broadcast updates cache with sources after relay responses
echo "=== Checking broadcast.dart for post-response cache updates ==="
cat packages/ndk/lib/domain_layer/usecases/broadcast/broadcast.dart
echo ""
echo "=== Looking for saveEvent calls with sources ==="
rg -n -C5 'saveEvent.*sources|sources.*saveEvent' --type dart
echo ""
echo "=== Checking if OK responses trigger cache updates ==="
rg -n -C5 '"OK".*sources|sources.*"OK"' --type dartRepository: relaystr/ndk
Length of output: 8951
🏁 Script executed:
#!/bin/bash
# Find where sources are assigned to events
echo "=== Looking for where sources are assigned/populated ==="
rg -n 'sources\s*=' --type dart | head -20
echo ""
echo "=== Checking relay_manager.dart for relay response handling ==="
fd -t f 'relay_manager.dart' -x cat {}
echo ""
echo "=== Checking for saveEvent calls and their context ==="
rg -n 'saveEvent' --type dart -B2 -A2Repository: relaystr/ndk
Length of output: 50368
🏁 Script executed:
#!/bin/bash
echo "=== Checking network_engine.dart for broadcast handling ==="
fd -t f 'network_engine.dart' -x cat {} | head -200
echo ""
echo "=== Looking at the test file ==="
cat packages/ndk/test/usecases/broadcast_sources_test.dartRepository: relaystr/ndk
Length of output: 1919
The test documents unimplemented functionality: broadcast responses must update the cached event with sources.
The event is saved to cache before broadcasting (line 81 in broadcast.dart), but relay responses never update it. When a relay responds with OK, relay_manager.dart only tracks the RelayBroadcastResponse—it does not update the cached event's sources field (see _processDecodedMessage() around line 445-463). Sources are only populated for subscription events in _handleIncomingEvent(), not for broadcasts. This test will fail unless the broadcast flow is updated to call cacheManager.saveEvent() again after receiving OK responses with the relay URL added to sources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/test/usecases/broadcast_sources_test.dart` around lines 34 - 39,
The test fails because relay OK responses for broadcasts aren't updating the
cached event's sources; modify the relay response handling in RelayManager (the
code path handling RelayBroadcastResponse in _processDecodedMessage) to load the
event from cache, append the relay URL to its sources (if not present), and call
cacheManager.saveEvent(updatedEvent) so broadcasts update sources similar to
_handleIncomingEvent; ensure you reference the same event.id and avoid
duplicating sources when saving.
|
|
||
| final relay1 = MockRelay(name: "relay 1"); | ||
| final relay2 = MockRelay(name: "relay 2"); | ||
| final relay3 = MockRelay(name: "relay 2", bannedWord: bannedWord); |
There was a problem hiding this comment.
Copy-paste error in relay name.
relay3 has the same name as relay2 ("relay 2"). This should likely be "relay 3" for clarity in test output and debugging.
Proposed fix
- final relay3 = MockRelay(name: "relay 2", bannedWord: bannedWord);
+ final relay3 = MockRelay(name: "relay 3", bannedWord: bannedWord);📝 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.
| final relay3 = MockRelay(name: "relay 2", bannedWord: bannedWord); | |
| final relay3 = MockRelay(name: "relay 3", bannedWord: bannedWord); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/ndk/test/usecases/stream_response_cleaner/event_sources_merge_test.dart`
at line 14, The test has a copy-paste error where MockRelay instantiation for
the variable relay3 uses the name "relay 2"; update the MockRelay call for
relay3 to use the correct name (e.g., "relay 3") so the MockRelay(name: ...) for
relay3 is distinct from relay2 and improves test clarity and debugging.
There was a problem hiding this comment.
@1-leo, thanks for confirming! The fix is straightforward — just change "relay 2" to "relay 3" in the relay3 instantiation:
final relay3 = MockRelay(name: "relay 3", bannedWord: bannedWord);This ensures each relay has a unique name, making test output and debugging much clearer.
🧠 Learnings used
Learnt from: CR
Repo: relaystr/ndk PR: 0
File: packages/sembast_cache_manager/CLAUDE.md:0-0
Timestamp: 2026-03-18T19:15:20.701Z
Learning: Applies to packages/sembast_cache_manager/**/*.dart : Implement the NDK CacheManager abstract class with all required methods for events, user metadata, contact lists, relay lists, and NIP-05 verification
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #470 +/- ##
==========================================
+ Coverage 73.36% 73.41% +0.04%
==========================================
Files 195 195
Lines 8969 9000 +31
==========================================
+ Hits 6580 6607 +27
- Misses 2389 2393 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| required StreamController<Nip01Event> outController, | ||
| required List<EventFilter> eventOutFilters, | ||
| }) : _trackingSet = trackingSet, | ||
| }) : _trackingMap = {for (var id in trackingSet) id: {}}, |
|
|
||
| if (_outController.isClosed) { | ||
| // check if event id is already seen | ||
| final existingSources = _trackingMap[event.id]; |
There was a problem hiding this comment.
when i understand correctly the sources map is per ndk session, no db integration for now, right?
| relayConnectivity.url | ||
| }.toList(); | ||
| final updatedEvent = event.copyWith(sources: updatedSources); | ||
| cacheManager!.saveEvent(updatedEvent); |
There was a problem hiding this comment.
should saveEvent be awaited?
|
|
||
| final relay1 = MockRelay(name: "relay 1"); | ||
| final relay2 = MockRelay(name: "relay 2"); | ||
| final relay3 = MockRelay(name: "relay 2", bannedWord: bannedWord); |
2 participants
Tests to demonstrate #469
Summary by CodeRabbit
Release Notes
New Features
Tests