Skip to content

Conversation

@sanity
Copy link
Collaborator

@sanity sanity commented Nov 19, 2025

Problem

  • test_put_contract_three_hop_returns_response relied on random node locations from #[freenet_test]; it flaked because the contract’s ring location sometimes made the gateway closer than the storage peer, breaking the intended three-hop path and timing out (issue test: fix flaky test_put_contract_three_hop_returns_response #2112).
  • Contract ring location differs between builds, so hard-coding static node coordinates isn’t reliable.

Approach

  • Extend #[freenet_test] with a node_locations_fn hook so tests can compute locations at runtime (e.g., based on a contract key). Keep the existing literal node_locations path and document both; enforce exclusivity and length checks.
  • For the three-hop PUT test, compute node locations from the contract’s ring location (gateway offset +0.2, peer-a +0.5, peer-c at the contract). Use a shared LazyLock to compile the contract once and reuse the key/state. Assert actual node locations and distances so we fail fast if topology drifts, and log distances for debugging.

Testing

  • cargo test -p freenet --test operations test_put_contract_three_hop_returns_response -- --exact

@sanity sanity requested a review from Copilot November 19, 2025 03:33
Copilot finished reviewing on behalf of sanity November 19, 2025 03:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a flaky test by introducing deterministic node locations in the #[freenet_test] macro, ensuring reliable three-hop routing topology in the test_put_contract_three_hop_returns_response test.

  • Extends #[freenet_test] macro with optional node_locations parameter for setting explicit ring positions
  • Pins test nodes to fixed locations (gateway: 0.45, peer-a: 0.15, peer-c: 0.88) to guarantee intended routing path
  • Adds validation assertions to verify peer-c is closest to the contract, enforcing the three-hop path

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
crates/freenet-macros/src/parser.rs Adds parsing and validation for the new node_locations array parameter
crates/freenet-macros/src/codegen.rs Implements helper function to use configured locations or fall back to random generation
crates/freenet-macros/README.md Documents the new node_locations parameter with usage rules and examples
crates/core/tests/operations.rs Updates three-hop PUT test to use deterministic locations with topology validation assertions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sanity sanity force-pushed the fix/issue-2112-flaky-test branch 2 times, most recently from faa65ec to 1009984 Compare November 19, 2025 04:05
Copy link
Collaborator Author

@sanity sanity left a comment

Choose a reason for hiding this comment

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

Addressed Copilot review:

  • Added range validation for node_locations and node_locations_fn results (must be in [0.0, 1.0]).
  • Completed README example for node_locations with a full test function.
    Re-ran: cargo test -p freenet --test operations test_put_contract_three_hop_returns_response -- --exact

let locs = vec![#(#values),*];
for (idx, loc) in locs.iter().enumerate() {
if !(0.0..=1.0).contains(loc) {
return Err(anyhow::anyhow!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

is better to output token stream with compiler diagnostics (location etc.) for errors in macros, look it up if you want to improve this

#[freenet_test(
nodes = ["gateway", "peer-a", "peer-c"],
gateways = ["gateway"],
node_locations_fn = three_hop_node_locations,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be more neat if we do something like:

nodes = { "gateway": {"location": x}, ...}

so we can pass node specific config and extend this as necessary

if you have something like { "gateway", "peer-a", } it would be an ordered map (can use IndexMap internally for sorting this out in the macro) and just use the default values/behavior (in this case, generate locations randomly)

@sanity sanity force-pushed the fix/issue-2112-flaky-test branch from 1009984 to 9d1683f Compare November 19, 2025 14:12
Copy link
Collaborator

@iduartgomez iduartgomez left a comment

Choose a reason for hiding this comment

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

The comments would be nice additions but not a blocker.

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.

3 participants