Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: spr/main/3dd1cf11
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion crates/infra_utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ description = "Infrastructure utility."
workspace = true

[dependencies]
thiserror.workspace = true
tokio = { workspace = true, features = ["process"] }

[dev-dependencies]
rstest.workspace = true
29 changes: 29 additions & 0 deletions crates/infra_utils/src/command.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use std::env;

use tokio::process::Command;

use crate::path::project_path;

#[cfg(test)]
#[path = "command_test.rs"]
mod command_test;

/// Returns a shell command originating from the project root, with cargo environment variables
/// filtered out.
///
/// # Arguments
/// * `command_name` - The shell command name.
///
/// # Returns
/// * A [`std::process::Command`] object with the current directory set to the project root, and
/// cleared out cargo related environment variables.
pub fn create_shell_command(command_name: &str) -> Command {
let project_path = project_path().expect("Failed to get project path");
let mut command = Command::new(command_name);
command.current_dir(&project_path);
// Filter out all CARGO_ environment variables.
env::vars().filter(|(key, _)| key.starts_with("CARGO_")).for_each(|(key, _)| {
command.env_remove(key);
});
command
}
15 changes: 15 additions & 0 deletions crates/infra_utils/src/command_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use rstest::rstest;

use crate::command::create_shell_command;

#[rstest]
#[tokio::test]
async fn create_shell_command_example() {
let mut ls_command = create_shell_command("ls");
let output = ls_command.output().await.expect("Failed to execute command");
let stdout = String::from_utf8_lossy(&output.stdout);

assert!(output.status.success());
// Project root should contain the `crates` directory.
assert!(stdout.contains("crates"));
}
1 change: 1 addition & 0 deletions crates/infra_utils/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
pub mod command;
pub mod path;
35 changes: 14 additions & 21 deletions crates/infra_utils/src/path.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,16 @@
use std::env;
use std::path::{Path, PathBuf};
use std::sync::LazyLock;

use thiserror::Error;
use std::{env, fs};

#[cfg(test)]
#[path = "path_test.rs"]
mod path_test;

#[derive(Debug, Error)]
pub enum PathResolutionError {
// TODO(Arni): Handle manifest dir not exist here?
#[error("No file exists at '{path}'")]
PathDoesNotExist { path: PathBuf },
/// This error is raised when file existence can be neither confirmed nor denied. See
/// [`std::path::Path::try_exists`] for more information.
#[error(transparent)]
IoError(#[from] std::io::Error),
}

// TODO(tsabary): wrap path-related env::* invocations in the repo as utility functions
static PATH_TO_CARGO_MANIFEST_DIR: LazyLock<Option<PathBuf>> =
LazyLock::new(|| env::var("CARGO_MANIFEST_DIR").ok().map(|dir| Path::new(&dir).into()));

// TODO(Tsabary): should not be public. Use a getter instead.
pub fn cargo_manifest_dir() -> Option<PathBuf> {
PATH_TO_CARGO_MANIFEST_DIR.clone()
}
Expand All @@ -34,16 +22,21 @@ pub fn cargo_manifest_dir() -> Option<PathBuf> {
/// * `relative_path` - A string slice representing the relative path from the project root.
///
/// # Returns
/// * An absolute `PathBuf` representing the resolved path starting from the project root.
pub fn resolve_project_relative_path(relative_path: &str) -> Result<PathBuf, PathResolutionError> {
/// * A `PathBuf` representing the resolved path starting from the project root.
pub fn resolve_project_relative_path(relative_path: &str) -> Result<PathBuf, std::io::Error> {
let base_dir = path_of_project_root();

let path = base_dir.join(relative_path);
if !path.try_exists()? {
return Err(PathResolutionError::PathDoesNotExist { path });
}
let absolute_path = fs::canonicalize(path)?;

Ok(path)
Ok(absolute_path)
}

/// Returns the absolute path of the project root directory.
///
/// # Returns
/// * A `PathBuf` representing the path of the project root.
pub fn project_path() -> Result<PathBuf, std::io::Error> {
resolve_project_relative_path(".")
}

fn path_of_project_root() -> PathBuf {
Expand Down
9 changes: 2 additions & 7 deletions crates/infra_utils/src/path_test.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
use crate::path::{path_of_project_root, resolve_project_relative_path, PathResolutionError};
use crate::path::{path_of_project_root, resolve_project_relative_path};

// TODO: Add a test for PathResolutionError::IoError.
#[test]
fn resolve_project_relative_path_on_non_existent_path() {
let relative_path = "does_not_exist.txt";
let expected_path = path_of_project_root().join(relative_path);
assert!(!expected_path.exists());
let result = resolve_project_relative_path(relative_path);

if let Err(PathResolutionError::PathDoesNotExist { path }) = result {
assert_eq!(path, expected_path);
} else {
panic!("Expected PathDoesNotExist error, got {:?}", result);
}
assert!(result.is_err(), "Expected an non-existent path error, got {:?}", result);
}

#[test]
Expand Down
3 changes: 0 additions & 3 deletions crates/papyrus_config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@

use clap::parser::MatchesError;
use dumping::REQUIRED_PARAM_DESCRIPTION_PREFIX;
use infra_utils::path::PathResolutionError;
use serde::{Deserialize, Serialize};
use serde_json::Value;
use validator::ValidationError;
Expand Down Expand Up @@ -180,8 +179,6 @@ pub enum ConfigError {
#[error(transparent)]
CommandMatches(#[from] MatchesError),
#[error(transparent)]
GetPathError(#[from] PathResolutionError),
#[error(transparent)]
IOError(#[from] std::io::Error),
// TODO(Eitan): Improve error message
#[error("Insert a new param is not allowed: {param_path}.")]
Expand Down
1 change: 1 addition & 0 deletions crates/starknet_integration_tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ tracing.workspace = true

[dev-dependencies]
futures.workspace = true
infra_utils.workspace = true
pretty_assertions.workspace = true
rstest.workspace = true
starknet_sequencer_infra.workspace = true
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use std::env;
use std::path::PathBuf;
use std::process::Stdio;
use std::time::Duration;

use infra_utils::command::create_shell_command;
use mempool_test_utils::starknet_api_test_utils::{AccountId, MultiAccountTransactionGenerator};
use papyrus_execution::execution_utils::get_nonce_at;
use papyrus_storage::state::StateStorageReader;
Expand All @@ -14,8 +14,9 @@ use starknet_api::state::StateNumber;
use starknet_integration_tests::integration_test_setup::IntegrationTestSetup;
use starknet_integration_tests::utils::{create_integration_test_tx_generator, send_account_txs};
use starknet_sequencer_infra::trace_util::configure_tracing;
use starknet_sequencer_node::test_utils::compilation::compile_node_result;
use starknet_types_core::felt::Felt;
use tokio::process::{Child, Command};
use tokio::process::Child;
use tokio::task::{self, JoinHandle};
use tokio::time::interval;
use tracing::{error, info};
Expand All @@ -27,19 +28,9 @@ fn tx_generator() -> MultiAccountTransactionGenerator {

// TODO(Tsabary): Move to a suitable util location.
async fn spawn_node_child_task(node_config_path: PathBuf) -> Child {
// Get the current working directory for the project
let project_path = env::current_dir().expect("Failed to get current directory").join("../..");

// TODO(Tsabary): Capture output to a log file, and present it in case of a failure.
// TODO(Tsabary): Change invocation from "cargo run" to separate compilation and invocation
// (build, and then invoke the binary).
Command::new("cargo")
.arg("run")
.arg("--bin")
.arg("starknet_sequencer_node")
.arg("--quiet")
.current_dir(&project_path)
.arg("--")
compile_node_result().await.expect("Failed to compile the sequencer node.");
create_shell_command("target/debug/starknet_sequencer_node")
.arg("--config_file")
.arg(node_config_path.to_str().unwrap())
.stderr(Stdio::inherit())
Expand Down
25 changes: 13 additions & 12 deletions crates/starknet_sequencer_node/src/test_utils/compilation.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::process::{Command, ExitStatus, Stdio};
use std::{env, io};
use std::io;
use std::process::{ExitStatus, Stdio};

use infra_utils::command::create_shell_command;
use tracing::info;

#[cfg(test)]
Expand All @@ -16,26 +17,26 @@ pub enum NodeCompilationError {
}

/// Compiles the node using `cargo build` for testing purposes.
fn compile_node() -> io::Result<ExitStatus> {
info!("Compiling the project");
// Get the current working directory for the project
let project_path = env::current_dir().expect("Failed to get current directory");
async fn compile_node() -> io::Result<ExitStatus> {
info!("Compiling the starknet_sequencer_node binary");

// Run `cargo build` to compile the project
let compilation_result = Command::new("cargo")
let compilation_result = create_shell_command("cargo")
.arg("build")
.current_dir(&project_path)
.arg("--bin")
.arg("starknet_sequencer_node")
.arg("--quiet")
.stderr(Stdio::inherit())
.stdout(Stdio::inherit())
.status();
.status()
.await?;

info!("Compilation result: {:?}", compilation_result);
compilation_result
Ok(compilation_result)
}

pub fn compile_node_result() -> Result<(), NodeCompilationError> {
match compile_node() {
pub async fn compile_node_result() -> Result<(), NodeCompilationError> {
match compile_node().await {
Ok(status) if status.success() => Ok(()),
Ok(status) => Err(NodeCompilationError::Status(status)),
Err(e) => Err(NodeCompilationError::IO(e)),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use rstest::rstest;

use crate::test_utils::compilation::compile_node_result;

#[test]
fn test_compile_node() {
assert!(compile_node_result().is_ok(), "Compilation failed");
#[rstest]
#[tokio::test]
async fn test_compile_node() {
assert!(compile_node_result().await.is_ok(), "Compilation failed");
}
Loading