Skip to content

test(client/doublezero): return different min, max, avg from make_latency#3564

Open
axaysagathiya wants to merge 3 commits intomalbeclabs:mainfrom
axaysagathiya:tests/make-latency-min-max-avg
Open

test(client/doublezero): return different min, max, avg from make_latency#3564
axaysagathiya wants to merge 3 commits intomalbeclabs:mainfrom
axaysagathiya:tests/make-latency-min-max-avg

Conversation

@axaysagathiya
Copy link
Copy Markdown

@axaysagathiya axaysagathiya commented Apr 22, 2026

Summary of Changes

Updated make_latency function in tests to accept and return distinct latency values for minimum, average, and maximum latency fields.

Closes #3495

Testing Verification

cd client/doublezero && cargo test dzd_latency

@axaysagathiya axaysagathiya marked this pull request as draft April 22, 2026 06:12
@axaysagathiya axaysagathiya force-pushed the tests/make-latency-min-max-avg branch from c1a8cb6 to 7460093 Compare April 22, 2026 18:19
@axaysagathiya axaysagathiya marked this pull request as ready for review April 22, 2026 18:19
@axaysagathiya
Copy link
Copy Markdown
Author

Is the current approach acceptable, or do we want to modify it?

We can accept the min_latency_ns as a function argument.
Then, we can calculate
the avg_latency_ns as min_latency_ns plus 1 millisecond, and
the max_latency_ns as min_latency_ns plus 2 milliseconds.

@martinsander00
Copy link
Copy Markdown
Contributor

martinsander00 commented Apr 22, 2026

Thanks for picking this up! Two suggestions to make the change worth it:

1. Convert the two existing "min vs avg" tests to use make_latency, they're natural callers for the new signature:

  • test_retrieve_latencies_sorts_by_min_not_avg (lines 1013–1032)
  • test_best_latency_current_device_seeded_by_min_not_avg (lines 1066–1085)

2. Make one existing test adversarial. Right now every call site passes monotonic triples (10M, 11M, 12M), so swapping min_latency_nsavg_latency_ns in the ranking code wouldn't fail anything. In test_best_latency_selects_lowest, change the values so min and avg disagree on the winner (e.g., pk1 min=12M/avg=5M, pk2 min=9M/avg=20M, pk3 min=15M/avg=15M), same assertion, but now it actually catches a wrong-field bug.

@axaysagathiya axaysagathiya force-pushed the tests/make-latency-min-max-avg branch from 7460093 to f27fa3f Compare April 23, 2026 14:09
@axaysagathiya axaysagathiya force-pushed the tests/make-latency-min-max-avg branch from f27fa3f to 1650f39 Compare April 23, 2026 18:03
@martinsander00
Copy link
Copy Markdown
Contributor

two things:

  1. can you use the same number notation we use in the rest of the codebase? Rn looks like you are using two different notations (1_000_000 vs 1000000)
  2. seems like due to some cross repo access policies our workflows don't run on forks. I'll chat with the team to see what we can do about it

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.

Improve make_latency functionality in tests to return different latencies (min, max, avg)

2 participants