Skip to content

Reduce flakiness in the ci #2370

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 17, 2025
Merged

Reduce flakiness in the ci #2370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f66acf
fix: Create a temporary path using module and test name when initiali…
sfauvel Mar 12, 2025
3e4e765
refacto: adapt build of container to use a unique temporary snapshot …
sfauvel Mar 13, 2025
e57806f
refacto: create a `temp_dir`macro that create a temporary path from t…
sfauvel Mar 14, 2025
c89bb76
Remove ̀tests` folder when creating temp dir
sfauvel Mar 17, 2025
af37a52
chore: upgrade crate versions
sfauvel Mar 17, 2025
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: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion mithril-aggregator/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "mithril-aggregator"
version = "0.7.14"
version = "0.7.15"
description = "A Mithril Aggregator server"
authors = { workspace = true }
edition = { workspace = true }
Expand Down
2 changes: 1 addition & 1 deletion mithril-aggregator/src/commands/database_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl MigrateCommand {
environment: ExecutionEnvironment::Production,
data_stores_directory: self.stores_directory.clone(),
// Temporary solution to avoid the need to provide a full configuration
..Configuration::new_sample()
..Configuration::new_sample(std::env::temp_dir())
};
debug!(root_logger, "DATABASE MIGRATE command"; "config" => format!("{config:?}"));
println!(
Expand Down
33 changes: 18 additions & 15 deletions mithril-aggregator/src/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ impl Default for ZstandardCompressionParameters {

impl Configuration {
/// Create a sample configuration mainly for tests
pub fn new_sample() -> Self {
pub fn new_sample(tmp_path: PathBuf) -> Self {
let genesis_verification_key = ProtocolGenesisSigner::create_deterministic_genesis_signer()
.create_genesis_verifier()
.to_verification_key();
Expand Down Expand Up @@ -263,8 +263,9 @@ impl Configuration {
// crate directory.
// Know issue:
// - There may be collision of the `snapshot_directory` between tests. Tests that
// depend on the `snapshot_directory` should specify their own.
snapshot_directory: std::env::temp_dir(),
// depend on the `snapshot_directory` should specify their own,
// and they can use the `temp_dir` macro for that.
snapshot_directory: tmp_path,
data_stores_directory: PathBuf::from(":memory:"),
genesis_verification_key: genesis_verification_key.to_json_hex().unwrap(),
reset_digests_cache: false,
Expand Down Expand Up @@ -592,14 +593,16 @@ impl Source for DefaultConfiguration {

#[cfg(test)]
mod test {
use mithril_common::temp_dir;

use super::*;

#[test]
fn safe_epoch_retention_limit_wont_change_a_value_higher_than_three() {
for limit in 4..=10u64 {
let configuration = Configuration {
store_retention_limit: Some(limit as usize),
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};
assert_eq!(configuration.safe_epoch_retention_limit(), Some(limit));
}
Expand All @@ -609,7 +612,7 @@ mod test {
fn safe_epoch_retention_limit_wont_change_a_none_value() {
let configuration = Configuration {
store_retention_limit: None,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};
assert_eq!(configuration.safe_epoch_retention_limit(), None);
}
Expand All @@ -619,7 +622,7 @@ mod test {
for limit in 0..=3 {
let configuration = Configuration {
store_retention_limit: Some(limit),
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};
assert_eq!(configuration.safe_epoch_retention_limit(), Some(3));
}
Expand All @@ -645,7 +648,7 @@ mod test {
fn compute_allowed_signed_entity_types_discriminants_append_default_discriminants() {
let config = Configuration {
signed_entity_types: None,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert_eq!(
Expand All @@ -660,14 +663,14 @@ mod test {
fn allow_http_serve_directory() {
let config = Configuration {
snapshot_uploader_type: SnapshotUploaderType::Local,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert!(config.allow_http_serve_directory());

let config = Configuration {
snapshot_uploader_type: SnapshotUploaderType::Gcp,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert!(!config.allow_http_serve_directory());
Expand All @@ -679,7 +682,7 @@ mod test {
server_ip: "1.2.3.4".to_string(),
server_port: 5678,
public_server_url: None,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert_eq!(
Expand All @@ -694,7 +697,7 @@ mod test {
server_ip: "1.2.3.4".to_string(),
server_port: 5678,
public_server_url: Some("https://example.com".to_string()),
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert_eq!(
Expand All @@ -709,7 +712,7 @@ mod test {
server_ip: "1.2.3.4".to_string(),
server_port: 6789,
public_server_url: None,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

let joined_url = config
Expand All @@ -730,7 +733,7 @@ mod test {
public_server_url: Some(format!(
"https://example.com/{subpath_without_trailing_slash}"
)),
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

let joined_url = config.get_server_url().unwrap().join("some/path").unwrap();
Expand All @@ -744,7 +747,7 @@ mod test {
fn is_slave_aggregator_returns_true_when_in_slave_mode() {
let config = Configuration {
master_aggregator_endpoint: Some("some_endpoint".to_string()),
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert!(config.is_slave_aggregator());
Expand All @@ -754,7 +757,7 @@ mod test {
fn is_slave_aggregator_returns_false_when_in_master_mode() {
let config = Configuration {
master_aggregator_endpoint: None,
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};

assert!(!config.is_slave_aggregator());
Expand Down
4 changes: 2 additions & 2 deletions mithril-aggregator/src/dependency_injection/builder/enablers/cardano_node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ impl DependenciesBuilder {

#[cfg(test)]
mod tests {
use mithril_common::entities::SignedEntityTypeDiscriminants;
use mithril_common::{entities::SignedEntityTypeDiscriminants, temp_dir};

use crate::Configuration;

Expand All @@ -211,7 +211,7 @@ mod tests {
) {
let configuration = Configuration {
signed_entity_types: Some(signed_entity_types),
..Configuration::new_sample()
..Configuration::new_sample(temp_dir!())
};
let mut dep_builder = DependenciesBuilder::new_with_stdout_logger(configuration);

Expand Down
18 changes: 5 additions & 13 deletions mithril-aggregator/src/dependency_injection/builder/protocol/artifacts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ impl DependenciesBuilder {

#[cfg(test)]
mod tests {
use mithril_common::test_utils::TempDir;
use mithril_common::temp_dir_create;
use mithril_persistence::sqlite::ConnectionBuilder;

use crate::dependency_injection::builder::CARDANO_DB_ARTIFACTS_DIR;
Expand All @@ -419,21 +419,17 @@ mod tests {

#[tokio::test]
async fn if_not_local_uploader_create_cardano_database_immutable_dirs() {
let snapshot_directory = TempDir::create(
"builder",
"if_not_local_uploader_create_cardano_database_immutable_dirs",
);
let snapshot_directory = temp_dir_create!();
let cdb_dir = snapshot_directory.join(CARDANO_DB_ARTIFACTS_DIR);
let ancillary_dir = cdb_dir.join("ancillary");
let immutable_dir = cdb_dir.join("immutable");
let digests_dir = cdb_dir.join("digests");

let mut dep_builder = {
let config = Configuration {
snapshot_directory,
// Test environment yield dumb uploaders
environment: ExecutionEnvironment::Test,
..Configuration::new_sample()
..Configuration::new_sample(snapshot_directory)
};

DependenciesBuilder::new_with_stdout_logger(config)
Expand All @@ -455,22 +451,18 @@ mod tests {

#[tokio::test]
async fn if_local_uploader_creates_all_cardano_database_subdirs() {
let snapshot_directory = TempDir::create(
"builder",
"if_local_uploader_creates_all_cardano_database_subdirs",
);
let snapshot_directory = temp_dir_create!();
let cdb_dir = snapshot_directory.join(CARDANO_DB_ARTIFACTS_DIR);
let ancillary_dir = cdb_dir.join("ancillary");
let immutable_dir = cdb_dir.join("immutable");
let digests_dir = cdb_dir.join("digests");

let mut dep_builder = {
let config = Configuration {
snapshot_directory,
// Must use production environment to make `snapshot_uploader_type` effective
environment: ExecutionEnvironment::Production,
snapshot_uploader_type: SnapshotUploaderType::Local,
..Configuration::new_sample()
..Configuration::new_sample(snapshot_directory)
};

DependenciesBuilder::new_with_stdout_logger(config)
Expand Down
17 changes: 15 additions & 2 deletions mithril-aggregator/src/dependency_injection/containers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,23 @@ impl DependencyContainer {

#[cfg(test)]
pub(crate) mod tests {

use std::path::PathBuf;

use crate::{dependency_injection::DependenciesBuilder, Configuration, DependencyContainer};

pub async fn initialize_dependencies() -> DependencyContainer {
let config = Configuration::new_sample();
/// Initialize dependency container with a unique temporary snapshot directory build from test path.
/// This macro should used directly in a function test to be able to retrieve the function name.
#[macro_export]
macro_rules! initialize_dependencies {
() => {{
initialize_dependencies(mithril_common::temp_dir!())
}};
}

pub async fn initialize_dependencies(tmp_path: PathBuf) -> DependencyContainer {
let config = Configuration::new_sample(tmp_path);

let mut builder = DependenciesBuilder::new_with_stdout_logger(config);

builder.build_dependency_container().await.unwrap()
Expand Down
18 changes: 10 additions & 8 deletions mithril-aggregator/src/http_server/routes/artifact_routes/cardano_database.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,9 @@ mod tests {
.expect_get_cardano_database_list_message()
.return_once(|_| Ok(vec![CardanoDatabaseSnapshotListItemMessage::dummy()]))
.once();
let mut dependency_manager = initialize_dependencies().await;

let mut dependency_manager = initialize_dependencies!().await;

dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down Expand Up @@ -235,7 +237,7 @@ mod tests {
.expect_get_cardano_database_list_message()
.return_once(|_| Err(HydrationError::InvalidData("invalid data".to_string()).into()))
.once();
let mut dependency_manager = initialize_dependencies().await;
let mut dependency_manager = initialize_dependencies!().await;
dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down Expand Up @@ -266,7 +268,7 @@ mod tests {
) {
let method = Method::GET.as_str();
let path = "/artifact/cardano-database/{hash}";
let dependency_manager = Arc::new(initialize_dependencies().await);
let dependency_manager = Arc::new(initialize_dependencies!().await);
let initial_counter_value = dependency_manager
.metrics_service
.get_artifact_detail_cardano_database_total_served_since_startup()
Expand Down Expand Up @@ -296,7 +298,7 @@ mod tests {
.expect_get_cardano_database_message()
.return_once(|_| Ok(Some(CardanoDatabaseSnapshotMessage::dummy())))
.once();
let mut dependency_manager = initialize_dependencies().await;
let mut dependency_manager = initialize_dependencies!().await;
dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down Expand Up @@ -330,7 +332,7 @@ mod tests {
.expect_get_cardano_database_message()
.return_once(|_| Ok(None))
.once();
let mut dependency_manager = initialize_dependencies().await;
let mut dependency_manager = initialize_dependencies!().await;
dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down Expand Up @@ -363,7 +365,7 @@ mod tests {
.expect_get_cardano_database_message()
.return_once(|_| Err(HydrationError::InvalidData("invalid data".to_string()).into()))
.once();
let mut dependency_manager = initialize_dependencies().await;
let mut dependency_manager = initialize_dependencies!().await;
dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down Expand Up @@ -396,7 +398,7 @@ mod tests {
.expect_get_cardano_database_digest_list_message()
.return_once(|| Ok(vec![CardanoDatabaseDigestListItemMessage::dummy()]))
.once();
let mut dependency_manager = initialize_dependencies().await;
let mut dependency_manager = initialize_dependencies!().await;
dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down Expand Up @@ -429,7 +431,7 @@ mod tests {
.expect_get_cardano_database_digest_list_message()
.return_once(|| Err(HydrationError::InvalidData("invalid data".to_string()).into()))
.once();
let mut dependency_manager = initialize_dependencies().await;
let mut dependency_manager = initialize_dependencies!().await;
dependency_manager.message_service = Arc::new(mock_http_message_service);

let method = Method::GET.as_str();
Expand Down
Loading