test(starknet_state_sync): test state sync runner after refactor to p2p #2695
Conversation
|
This change is |
|
Artifacts upload workflows: |
|
Benchmark movements: |
AlonLStarkWare
force-pushed
the
alonl/refactor_p2p_state_sync_runner_tests
branch
from
3746d22 to
375c4bc
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AlonLStarkWare and @noamsp-starkware)
crates/starknet_state_sync/src/runner/test.rs line 20 at r1 (raw file):
#[test] fn run_returns_when_sync_client_future_returns() {
Add one for server as well
crates/starknet_state_sync/src/runner/test.rs line 30 at r1 (raw file):
#[test] fn run_returns_error_when_sync_server_future_returns() {
Keep the same order as the termination tests (network, client, server)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @ShahakShama)
crates/starknet_state_sync/src/runner/test.rs line 20 at r1 (raw file):
Previously, ShahakShama wrote…
Add one for server as well
Because the server is a future without a value unlike the client and network which return a result, there is only one flow to test for the server.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlonLStarkWare and @noamsp-starkware)
crates/starknet_state_sync/src/runner/test.rs line 30 at r1 (raw file):
#[test] fn run_returns_error_when_sync_server_future_returns() {
You should fix the source code to return Ok(()) if the sync server component finished and change this test accordingly
There was a problem hiding this comment.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @ShahakShama)
crates/starknet_state_sync/src/runner/test.rs line 30 at r1 (raw file):
Previously, ShahakShama wrote…
You should fix the source code to return Ok(()) if the sync server component finished and change this test accordingly
I don't really understand why but done
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlonLStarkWare and @noamsp-starkware)
crates/starknet_state_sync/src/runner/test.rs line 30 at r1 (raw file):
Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…
I don't really understand why but done
Rename the test as well
why - There's no difference between this case and the sync client case. In both cases one future finishes so they should return the same
AlonLStarkWare
force-pushed
the
alonl/refactor_p2p_state_sync_runner_tests
branch
from
81848a2 to
c7975a5
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @ShahakShama)
crates/starknet_state_sync/src/runner/test.rs line 30 at r1 (raw file):
Previously, ShahakShama wrote…
Rename the test as well
why - There's no difference between this case and the sync client case. In both cases one future finishes so they should return the same
Done.
No description provided.