chore(tests_integration): replace run with build and direct invocation

[Itay-Tsabary-Starkware]

Stack:

• chore(tests_integration): replace run with build and direct invocation #2270 ⬅
• chore(sequencer_node): replace run node shell command #2266

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

[reviewable-StarkWare]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (spr/main/3dd1cf11@6ffc1ed). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...rknet_sequencer_node/src/test_utils/compilation.rs	33.33%	2 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                  @@
##             spr/main/3dd1cf11    #2270   +/-   ##
====================================================
  Coverage                     ?   38.08%           
====================================================
  Files                        ?      274           
  Lines                        ?    31257           
  Branches                     ?    31257           
====================================================
  Hits                         ?    11905           
  Misses                       ?    18330           
  Partials                     ?     1022           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ArniStarkware]

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 35 at r1 (raw file):

    info!("Compiling the node.");
    compile_node_result().await.expect("Failed to compile the sequencer node.");
    let node_executable = resolve_project_relative_path("target/debug/starknet_sequencer_node")

At some point, into! should indicate that the node was compiled and that the compilation result is written to a specific path, namely, target/debug/starknet_sequencer_node. Maybe inside compile_node_result?

Also, the path target/debug/starknet_sequencer_node should be constructed.

Suggestion:

    info!("Compiling the node.");
    compile_node_result().await.expect("Failed to compile the sequencer node.");
    const STARKNET_SEQUENCER_NODE_BIN_NAME: &str = "starknet_sequencer_node";
    let path = Path::new("target/debug").join(STARKNET_SEQUENCER_NODE_BIN_NAME);
    
    let node_executable = resolve_project_relative_path(path)

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 37 at r1 (raw file):

    let node_executable = resolve_project_relative_path("target/debug/starknet_sequencer_node")
        .expect("Node executable should be available");
    let node_executable = node_executable.to_str().expect("Node executable path should be valid");

Why not?

Suggestion:

    let node_executable = resolve_project_relative_path("target/debug/starknet_sequencer_node")
        .expect("Node executable should be available").to_str().expect("Node executable path should be valid");

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 134 at r1 (raw file):

    // Wait for the node to start.
    match integration_test_setup.is_alive_test_client.await_alive(Duration::from_secs(5), 100).await

100 attempts? That sounds like a lot.
What are the repercussions?

Suggestion:

match integration_test_setup.is_alive_test_client.await_alive(Duration::from_secs(5), 30)

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 35 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

At some point, into! should indicate that the node was compiled and that the compilation result is written to a specific path, namely, target/debug/starknet_sequencer_node. Maybe inside compile_node_result?

Also, the path target/debug/starknet_sequencer_node should be constructed.

compile_node_result does mention that.
The new version includes a few slight improvements in the spirit of your suggestion though, thanks.

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 37 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Why not?

Originally:

temporary value dropped while borrowed
consider using a `let` binding to create a longer lived valuerustcClick for full compiler diagnostic
end_to_end_integration_test.rs(36, 111): temporary value is freed at the end of this statement
end_to_end_integration_test.rs(40, 26): borrow later used here

Found a different solution though.

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 134 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

100 attempts? That sounds like a lot.
What are the repercussions?

It seems that codecov succeeds at around 30 attempts. I took a safety margin to avoid flakiness.
A longer timeout means that in case of certain failures the test will take longer to stop.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 134 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

It seems that codecov succeeds at around 30 attempts. I took a safety margin to avoid flakiness.
A longer timeout means that in case of certain failures the test will take longer to stop.

TBH I've had enough with this, I'm removing this from the default cargo test and setting a specific ci job for this. This should significnatly improve everything.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 134 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

TBH I've had enough with this, I'm removing this from the default cargo test and setting a specific ci job for this. This should significnatly improve everything.

I changed this to 50, and also applied this edit in the previous pr (that keeps failing in codecov due to the above explanation). Waiting for a conceptual lgtm / alternative here before merging the previous.

[ArniStarkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

---

crates/starknet_integration_tests/tests/end_to_end_integration_test.rs line 134 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I changed this to 50, and also applied this edit in the previous pr (that keeps failing in codecov due to the above explanation). Waiting for a conceptual lgtm / alternative here before merging the previous.

I don't see the "50"

[ArniStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)