apollo_infra: concurrent tests

[victorkstarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[victorkstarkware]

• apollo_infra: concurrent tests #11500 👈 (View in Graphite)
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 2 comments.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @victorkstarkware).

---

crates/apollo_infra/src/tests/mod.rs line 190 at r1 (raw file):

/// Each test that binds ports should use a different instance_index to get disjoint port ranges.
/// This is necessary because nextest runs each test in its own process, so a shared static
/// counter doesn't work - each process would start from the same base port.

Please let's be less specific regarding the test engine. Suggestion

This is necessary to allow running tests concurrently in different processes, which do not have a shared memory.

Also, please change /// (outward facing comment / doc) to // (inner comment).

Also 2: do we need the pub modifier here? Will pub(crate) work as well?

Code quote:

/// This is necessary because nextest runs each test in its own process, so a shared static
/// counter doesn't work - each process would start from the same base port.

---

crates/apollo_infra/src/tests/mod.rs line 191 at r1 (raw file):

/// This is necessary because nextest runs each test in its own process, so a shared static
/// counter doesn't work - each process would start from the same base port.
pub fn available_ports(instance_index: u16) -> AvailablePorts {

get_available_ports or available_ports_factory ?

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @victorkstarkware).

---

crates/apollo_infra/src/tests/remote_component_client_server_test.rs line 186 at r1 (raw file):

    let setup_value: ValueB = Felt::from(90);
    let mut ports = available_ports(1);

rename to available_ports throughout, following on the fn name change suggested above ^

Code quote:

ports

[victorkstarkware]

@victorkstarkware made 3 comments.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware).

---

crates/apollo_infra/src/tests/mod.rs line 190 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please let's be less specific regarding the test engine. Suggestion

This is necessary to allow running tests concurrently in different processes, which do not have a shared memory.

Also, please change /// (outward facing comment / doc) to // (inner comment).

Also 2: do we need the pub modifier here? Will pub(crate) work as well?

Done. And pub not really required - removed.

---

crates/apollo_infra/src/tests/mod.rs line 191 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

get_available_ports or available_ports_factory ?

Done.

---

crates/apollo_infra/src/tests/remote_component_client_server_test.rs line 186 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

rename to available_ports throughout, following on the fn name change suggested above ^

Done.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware reviewed 5 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @victorkstarkware).