Skip to content

[test][node] Cover join stage-1 with no known right neighbor - #141

Merged
TheP2P (thep2p) merged 2 commits into
thep2p/90-join-stage1-one-node-graph-testfrom
thep2p/90-join-stage1-no-right-neighbor-test
Sep 30, 2026
Merged

TheP2P (thep2p) merged 2 commits into
thep2p/90-join-stage1-one-node-graph-testfrom
thep2p/90-join-stage1-no-right-neighbor-test

Conversation

@thep2p

Copy link
Copy Markdown
Collaborator

Summary

  • Add test_join_stage1_link_level0_no_right_neighbor_sends_single_link_request: when s's neighbor query resolves z as None, join_stage1_link_level0 never sends a second GetLinkOp and resolves once the single s-side request is applied, leaving the right-side table entry unset.
  • Required acceptance-criteria coverage for [Node] Implement: level-0 join linking (Phase 1) #90 (issue [Node] Implement: level-0 join linking (Phase 1) #90's third required test scenario). No production code change: the None-handling branch this exercises already shipped in the orchestration PR earlier in this stack.

Test plan

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test

@thep2p
TheP2P (thep2p) added this pull request to stack #143 September 14, 2026 20:23
@thep2p
TheP2P (thep2p) force-pushed the thep2p/90-join-stage1-no-right-neighbor-test branch from b3343b9 to 1ea3dff Compare September 15, 2026 17:27
@thep2p
TheP2P (thep2p) force-pushed the thep2p/90-join-stage1-no-right-neighbor-test branch from 1ea3dff to dd6863e Compare September 15, 2026 17:50
@thep2p
TheP2P (thep2p) force-pushed the thep2p/90-join-stage1-no-right-neighbor-test branch from dd6863e to 98cbcd6 Compare September 15, 2026 21:09
@thep2p
TheP2P (thep2p) force-pushed the thep2p/90-join-stage1-no-right-neighbor-test branch 2 times, most recently from 8e4ab3e to 5619ba8 Compare September 30, 2026 00:52
@thep2p
TheP2P (thep2p) force-pushed the thep2p/90-join-stage1-no-right-neighbor-test branch from 5619ba8 to 86b51db Compare September 30, 2026 16:16
@thep2p
TheP2P (thep2p) force-pushed the thep2p/90-join-stage1-no-right-neighbor-test branch from 86b51db to b898a24 Compare September 30, 2026 18:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The reviewed test changes are complete and introduce no unresolved issues.

Review effort: Lite
Findings: None

What changed in this PR

Adds regression coverage for stage-1 joining when the predecessor has no right neighbor.

Changes:

  • Verifies only one link request is sent.
  • Confirms the join completes with the right table entry unset.
File Description
src/​node/​base_node.rs Adds the no-right-neighbor stage-1 join test.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@thep2p

TheP2P (thep2p) commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Claude (@claude) review

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Claude finished TheP2P (@thep2p)'s task —— View job


PR Review: Join stage-1, no right neighbor

  • Read repo guidelines (no CLAUDE.md present)
  • Read the changed test in src/node/base_node.rs
  • Cross-check against production join_stage1_link_level0
  • Review test correctness and quality

Verdict: Approve ✅

This is a clean, well-scoped test-only PR. The new test faithfully exercises the production None-branch and the refactor to poll_until_some is a nice readability win.

What I verified

1. The test mirrors real behavior. In join_stage1_link_level0 (src/node/base_node.rs), when get_neighbor returns None the code takes the branch that only awaits search_link.await? and sends no second link request. The test drives exactly this: RetNeighborOp { neighbor: None }, then asserts link_request_count == 1 and that the right (query_direction) entry stays None. Correct.

2. Direction logic is right. introducer < node_id ⇒ search_direction = Right, query_direction = Left. The test builds introducer = random_identifier_less_than(&node_id) and asserts the search goes Direction::Right and the neighbor query reuses Direction::Right. Matches production (get_neighbor(search_result, search_direction, …)). Good.

3. The poll_until_some refactor is sound. Replacing the hand-rolled loop { … yield_now().await } blocks with poll_until_some(|| …) (and s.zip(m) / .then_some(())) preserves the same bounded-poll semantics while cutting repetition across four existing tests. The helper (src/node/testutil.rs) yields between probes, so it keeps the project's "no wall-clock sleep" discipline.

Minor observations (non-blocking)

  • Left-entry assertion via the reply, not a distinct identity. The SetLinkOp reply carries dir: Direction::Left, linked: Some(s_identity), and the test then asserts the left slot holds s_id. This confirms the reply is routed to the correct slot, which is the point of the test — just noting the left-side value comes from the injected reply rather than from production choosing it, so it's a routing assertion, not a value-selection one. That's appropriate for this unit.
  • Doc comment polish (commit 35a14e9) reads clearly now and drops the introducer.id() < u.id() jargon in favor of plain language — good improvement.
  • Couldn't run cargo fmt/clippy/test locally (command approval not granted in this environment), so I'm relying on CI for the mechanical checks. The code reads as fmt/clippy-clean and the test logic is correct by inspection.

Nice incremental coverage for #90.
• Branch: thep2p/90-join-stage1-no-right-neighbor-test

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, well-scoped test-only PR. The new test faithfully exercises the production None-branch of join_stage1_link_level0 (only search_link is awaited, no second link request), the direction logic matches production (introducer < node_id => search Right, query Left), and the poll_until_some refactor preserves bounded-poll semantics while cutting repetition. Approving. Note: relied on CI for fmt/clippy/test as I couldn't run cargo locally.

@thep2p
TheP2P (thep2p) merged commit d8b26e5 into main Sep 30, 2026
6 checks passed
@thep2p
TheP2P (thep2p) deleted the thep2p/90-join-stage1-no-right-neighbor-test branch September 30, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants