From eb783fe33afbd12345d09c6924d4be2628cd7a7e Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 17 Jun 2024 13:25:43 -0700 Subject: [PATCH 1/5] Rework storage tests to test both encoding formats Fix tests failing if the default encoding is switched to legacy in the code; make the tests not rely on what the coded default is. Signed-off-by: J Robert Ray --- Cargo.lock | 58 +++---- Cargo.toml | 1 + crates/spfs/Cargo.toml | 2 +- crates/spfs/src/bootstrap_test.rs | 6 +- crates/spfs/src/graph/layer_test.rs | 77 ++++++--- crates/spfs/src/runtime/storage_test.rs | 209 ++++++++++++++++++------ 6 files changed, 243 insertions(+), 110 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b7ce0329b1..cf27c436f3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2124,9 +2124,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.18.0" +version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd8b5dd2ae5ed71462c540258bedcb51965123ad7e7ccf4b9a8cafaa4a63576d" +checksum = "3fdb12b2476b595f9358c5161aa467c2438859caa136dec86c26fdd2efe17b92" [[package]] name = "oorandom" @@ -2503,30 +2503,6 @@ dependencies = [ "indexmap 1.9.3", ] -[[package]] -name = "proc-macro-error" -version = "1.0.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" -dependencies = [ - "proc-macro-error-attr", - "proc-macro2", - "quote", - "syn 1.0.109", - "version_check", -] - -[[package]] -name = "proc-macro-error-attr" -version = "1.0.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" -dependencies = [ - "proc-macro2", - "quote", - "version_check", -] - [[package]] name = "proc-macro2" version = "1.0.78" @@ -3040,6 +3016,15 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scc" +version = "2.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76ad2bbb0ae5100a07b7a6f2ed7ab5fd0045551a4c507989b7a620046ea3efdc" +dependencies = [ + "sdd", +] + [[package]] name = "schannel" version = "0.1.22" @@ -3055,6 +3040,12 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "sdd" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b84345e4c9bd703274a082fb80caaa99b7612be48dfaa1dd9266577ec412309d" + [[package]] name = "security-framework" version = "2.9.2" @@ -3288,28 +3279,27 @@ dependencies = [ [[package]] name = "serial_test" -version = "0.8.0" +version = "3.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7eec42e7232e5ca56aa59d63af3c7f991fe71ee6a3ddd2d3480834cf3902b007" +checksum = "4b4b487fe2acf240a021cf57c6b2b4903b1e78ca0ecd862a71b71d2a51fed77d" dependencies = [ "futures", - "lazy_static", "log", + "once_cell", "parking_lot", + "scc", "serial_test_derive", ] [[package]] name = "serial_test_derive" -version = "0.8.0" +version = "3.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f1b95bb2f4f624565e8fe8140c789af7e2082c0e0561b5a82a1b678baa9703dc" +checksum = "82fe9db325bcef1fbcde82e078a5cc4efdf787e96b3b9cf45b50b529f2083d67" dependencies = [ - "proc-macro-error", "proc-macro2", "quote", - "rustversion", - "syn 1.0.109", + "syn 2.0.48", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index f1128eda05..87eefc882e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -87,6 +87,7 @@ sentry-tracing = { version = "0.32.2" } serde = "1.0" serde_json = "1.0" serde_yaml = "0.9.25" +serial_test = "3.1" shellexpand = "3.1.0" spfs = { path = "crates/spfs" } spfs-cli-common = { path = "crates/spfs-cli/common" } diff --git a/crates/spfs/Cargo.toml b/crates/spfs/Cargo.toml index 86d6bfe887..90235bb915 100644 --- a/crates/spfs/Cargo.toml +++ b/crates/spfs/Cargo.toml @@ -109,7 +109,7 @@ tonic-build = { workspace = true } [dev-dependencies] criterion = { version = "0.3", features = ["async_tokio", "html_reports"] } rstest = { version = "0.15.0", default_features = false } -serial_test = "0.8.0" +serial_test = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter"] } static_assertions = { workspace = true } diff --git a/crates/spfs/src/bootstrap_test.rs b/crates/spfs/src/bootstrap_test.rs index 1c6fd4297d..f8548a7878 100644 --- a/crates/spfs/src/bootstrap_test.rs +++ b/crates/spfs/src/bootstrap_test.rs @@ -20,7 +20,7 @@ use crate::runtime; case("tcsh", "test.csh", "echo hi; setenv TEST_VALUE 'spfs-test-value'") )] #[tokio::test] -#[serial_test::serial] // env and config manipulation must be reliable +#[serial_test::serial(env)] // env and config manipulation must be reliable async fn test_shell_initialization_startup_scripts( shell: &str, startup_script: &str, @@ -99,7 +99,7 @@ async fn test_shell_initialization_startup_scripts( #[rstest(shell, case("bash"), case("tcsh"))] #[tokio::test] -#[serial_test::serial] // env and config manipulation must be reliable +#[serial_test::serial(env)] // env and config manipulation must be reliable async fn test_shell_initialization_no_startup_scripts(shell: &str, tmpdir: tempfile::TempDir) { init_logging(); let shell_path = match which(shell) { @@ -154,7 +154,7 @@ async fn test_shell_initialization_no_startup_scripts(shell: &str, tmpdir: tempf #[cfg(unix)] #[rstest(shell, case("bash"), case("tcsh"))] #[tokio::test] -#[serial_test::serial] // env manipulation must be reliable +#[serial_test::serial(env)] // env manipulation must be reliable async fn test_find_alternate_bash(shell: &str, tmpdir: tempfile::TempDir) { init_logging(); let original_path = std::env::var("PATH").unwrap_or_default(); diff --git a/crates/spfs/src/graph/layer_test.rs b/crates/spfs/src/graph/layer_test.rs index 1878f9c751..a8a22889ea 100644 --- a/crates/spfs/src/graph/layer_test.rs +++ b/crates/spfs/src/graph/layer_test.rs @@ -5,10 +5,10 @@ use rstest::rstest; use super::Layer; -use crate::encoding; use crate::encoding::prelude::*; -use crate::graph::object::EncodingFormat; +use crate::graph::object::{DigestStrategy, EncodingFormat}; use crate::graph::{AnnotationValue, Object}; +use crate::{encoding, Config}; #[rstest] fn test_layer_encoding_manifest_only() { @@ -22,34 +22,62 @@ fn test_layer_encoding_manifest_only() { assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) } -#[rstest] -fn test_layer_encoding_annotation_only() { +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] +#[serial_test::serial(config)] +fn test_layer_encoding_annotation_only( + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let expected = Layer::new_with_annotation( "key".to_string(), AnnotationValue::String("value".to_string()), ); - tracing::error!("Expected: {:?}", expected); let mut stream = Vec::new(); - expected.encode(&mut stream).unwrap(); - - let decoded = Object::decode(&mut stream.as_slice()); - if EncodingFormat::default() == EncodingFormat::Legacy { - if decoded.is_ok() { - panic!("This test should fail when EncodingFormat::Legacy is the default") + match expected.encode(&mut stream) { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Encode should fail if encoding format is legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error encoding layer: {e}") } - // Don't run the rest of the test when EncodingFormat::Legacy is used - return; }; + let decoded = Object::decode(&mut stream.as_slice()); + let actual = decoded.unwrap().into_layer().unwrap(); println!(" Actual: {:?}", actual); assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()) } -#[rstest] -fn test_layer_encoding_manifest_and_annotations() { +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] +#[serial_test::serial(config)] +fn test_layer_encoding_manifest_and_annotations( + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let expected = Layer::new_with_manifest_and_annotations( encoding::EMPTY_DIGEST.into(), vec![( @@ -60,7 +88,19 @@ fn test_layer_encoding_manifest_and_annotations() { println!("Expected: {:?}", expected); let mut stream = Vec::new(); - expected.encode(&mut stream).unwrap(); + match expected.encode(&mut stream) { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Encode should fail if encoding format is legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error encoding layer: {e}") + } + }; let actual = Object::decode(&mut stream.as_slice()) .unwrap() @@ -68,10 +108,9 @@ fn test_layer_encoding_manifest_and_annotations() { .unwrap(); println!(" Actual: {:?}", actual); - match EncodingFormat::default() { + match write_encoding_format { EncodingFormat::Legacy => { - // Legacy encoding does not support annotaion data, so these won't match - assert_ne!(actual.digest().unwrap(), expected.digest().unwrap()) + unreachable!(); } EncodingFormat::FlatBuffers => { // Under flatbuffers encoding both will contain the diff --git a/crates/spfs/src/runtime/storage_test.rs b/crates/spfs/src/runtime/storage_test.rs index 30f51fbacf..ce53f732d2 100644 --- a/crates/spfs/src/runtime/storage_test.rs +++ b/crates/spfs/src/runtime/storage_test.rs @@ -13,13 +13,13 @@ use rstest::rstest; use spfs_encoding::Digestible; use super::{makedirs_with_perms, Data, Storage}; -use crate::encoding; use crate::fixtures::*; -use crate::graph::object::EncodingFormat; +use crate::graph::object::{DigestStrategy, EncodingFormat}; use crate::graph::{AnnotationValue, Layer, Platform}; use crate::runtime::storage::{LiveLayerApiVersion, LiveLayerContents}; use crate::runtime::{BindMount, KeyValuePair, LiveLayer, LiveLayerFile}; use crate::storage::prelude::Database; +use crate::{encoding, Config}; #[rstest] fn test_bindmount_creation() { @@ -176,9 +176,22 @@ async fn test_storage_create_runtime(tmpdir: tempfile::TempDir) { .is_err()); } -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_storage_runtime_with_annotation(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_storage_runtime_with_annotation( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let root = tmpdir.path().to_string_lossy().to_string(); let repo = crate::storage::RepositoryHandle::from( crate::storage::fs::FsRepository::create(root) @@ -198,10 +211,22 @@ async fn test_storage_runtime_with_annotation(tmpdir: tempfile::TempDir) { // Test - insert data let key = "some_field".to_string(); let value = "some value".to_string(); - assert!(runtime + match runtime .add_annotation(key.clone(), value.clone(), limit) .await - .is_ok()); + { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; // Test - insert some more data let value2 = "some other value".to_string(); @@ -213,12 +238,8 @@ async fn test_storage_runtime_with_annotation(tmpdir: tempfile::TempDir) { // Test - retrieve data - the first inserted data should be the // what is retrieved because of how adding to the runtime stack // works. - if EncodingFormat::default() == EncodingFormat::Legacy { - if (runtime.annotation(&key).await).is_ok() { - panic!("This should fail when EncodingFormat::Legacy is the default") - } - // Don't run the rest of the test when EncodingFormat::Legacy is used - return; + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); }; let result = runtime.annotation(&key).await.unwrap(); @@ -227,9 +248,22 @@ async fn test_storage_runtime_with_annotation(tmpdir: tempfile::TempDir) { assert!(value == *result.unwrap()); } -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_storage_runtime_add_annotations_list(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_storage_runtime_add_annotations_list( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let root = tmpdir.path().to_string_lossy().to_string(); let repo = crate::storage::RepositoryHandle::from( crate::storage::fs::FsRepository::create(root) @@ -255,35 +289,54 @@ async fn test_storage_runtime_add_annotations_list(tmpdir: tempfile::TempDir) { let annotations: Vec = vec![(key.clone(), value.clone()), (key2.clone(), value2.clone())]; - assert!(runtime.add_annotations(annotations, limit).await.is_ok()); + match runtime.add_annotations(annotations, limit).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; // Test - retrieve data both pieces of data let result = runtime.annotation(&key).await.unwrap(); - if EncodingFormat::default() == EncodingFormat::Legacy { - assert!( - result.is_none(), - "No annotation should be found under Legacy encoding" - ); + if write_encoding_format == EncodingFormat::Legacy { + unreachable!() } else { assert!(result.is_some()); assert!(value == *result.unwrap()); } let result2 = runtime.annotation(&key2).await.unwrap(); - if EncodingFormat::default() == EncodingFormat::Legacy { - assert!( - result2.is_none(), - "No annotation should be found under Legacy encoding" - ); + if write_encoding_format == EncodingFormat::Legacy { + unreachable!() } else { assert!(result2.is_some()); assert!(value2 == *result2.unwrap()); } } -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_storage_runtime_with_nested_annotation(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_storage_runtime_with_nested_annotation( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + // Setup the objects needed for the runtime used in the test let root = tmpdir.path().to_string_lossy().to_string(); let repo = crate::storage::RepositoryHandle::from( @@ -297,7 +350,19 @@ async fn test_storage_runtime_with_nested_annotation(tmpdir: tempfile::TempDir) let value = "somevalue".to_string(); let annotation_value = AnnotationValue::String(value.clone()); let layer = Layer::new_with_annotation(key.clone(), annotation_value); - repo.write_object(&layer).await.unwrap(); + match repo.write_object(&layer).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; // make a platform that contains the annotation layer let layers: Vec = vec![layer.digest().unwrap()]; @@ -314,12 +379,8 @@ async fn test_storage_runtime_with_nested_annotation(tmpdir: tempfile::TempDir) .expect("failed to create runtime in storage"); runtime.push_digest(platform.digest().unwrap()); - if EncodingFormat::default() == EncodingFormat::Legacy { - if (runtime.annotation(&key).await).is_ok() { - panic!("This should fail when EncodingFormat::Legacy is the default") - } - // Don't run the rest of the test when EncodingFormat::Legacy is used - return; + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); }; // Test - retrieve the data even though it is nested inside a @@ -330,9 +391,22 @@ async fn test_storage_runtime_with_nested_annotation(tmpdir: tempfile::TempDir) assert!(value == *result.unwrap()); } -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_storage_runtime_with_annotation_all(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_storage_runtime_with_annotation_all( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let root = tmpdir.path().to_string_lossy().to_string(); let repo = crate::storage::RepositoryHandle::from( crate::storage::fs::FsRepository::create(root) @@ -353,10 +427,22 @@ async fn test_storage_runtime_with_annotation_all(tmpdir: tempfile::TempDir) { let key = "some_field".to_string(); let value = "somevalue".to_string(); - assert!(runtime + match runtime .add_annotation(key.clone(), value.clone(), limit) .await - .is_ok()); + { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; let key2 = "some_field2".to_string(); let value2 = "somevalue2".to_string(); @@ -366,12 +452,8 @@ async fn test_storage_runtime_with_annotation_all(tmpdir: tempfile::TempDir) { .is_ok()); // Test - get all the data back out - if EncodingFormat::default() == EncodingFormat::Legacy { - if (runtime.all_annotations().await).is_ok() { - panic!("This should fail when EncodingFormat::Legacy is the default") - } - // Don't run the rest of the test when EncodingFormat::Legacy is used - return; + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); }; let result = runtime.all_annotations().await.unwrap(); @@ -388,9 +470,22 @@ async fn test_storage_runtime_with_annotation_all(tmpdir: tempfile::TempDir) { } } -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_storage_runtime_with_nested_annotation_all(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_storage_runtime_with_nested_annotation_all( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + // setup the objects needed for the runtime used in the test let root = tmpdir.path().to_string_lossy().to_string(); let repo = crate::storage::RepositoryHandle::from( @@ -404,7 +499,19 @@ async fn test_storage_runtime_with_nested_annotation_all(tmpdir: tempfile::TempD let value = "somevalue".to_string(); let annotation_value = AnnotationValue::String(value.clone()); let layer = Layer::new_with_annotation(key.clone(), annotation_value); - repo.write_object(&layer.clone()).await.unwrap(); + match repo.write_object(&layer.clone()).await { + Ok(_) if write_encoding_format == EncodingFormat::Legacy => { + panic!("Writing annotations should fail when using EncodingFormat::Legacy") + } + Ok(_) => {} + Err(_) if write_encoding_format == EncodingFormat::Legacy => { + // This error is expected + return; + } + Err(e) => { + panic!("Error adding annotations: {e}") + } + }; let key2 = "some_field2".to_string(); let value2 = "somevalue2".to_string(); @@ -436,12 +543,8 @@ async fn test_storage_runtime_with_nested_annotation_all(tmpdir: tempfile::TempD // Test - get all the data back out even thought it is nested at // different levels in different platform objects in the runtime - if EncodingFormat::default() == EncodingFormat::Legacy { - if (runtime.all_annotations().await).is_ok() { - panic!("This should fail when EncodingFormat::Legacy is the default") - } - // Don't run the rest of the test when EncodingFormat::Legacy is used - return; + if write_encoding_format == EncodingFormat::Legacy { + unreachable!(); }; let result = runtime.all_annotations().await.unwrap(); From 1897684af99108d4c391db4eeed6880ebfd717dc Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 17 Jun 2024 14:45:24 -0700 Subject: [PATCH 2/5] Expand matrix testing to more tests These other tests are subject to spurious failures if run concurrently with the tests that change the encoding format. Now they can test for correct behavior for both digest strategies. Signed-off-by: J Robert Ray --- crates/spfs/src/storage/fs/renderer_test.rs | 37 ++++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/crates/spfs/src/storage/fs/renderer_test.rs b/crates/spfs/src/storage/fs/renderer_test.rs index 0a7ebd8561..d3bbf02be3 100644 --- a/crates/spfs/src/storage/fs/renderer_test.rs +++ b/crates/spfs/src/storage/fs/renderer_test.rs @@ -9,13 +9,27 @@ use rstest::rstest; use super::was_render_completed; use crate::encoding::prelude::*; use crate::fixtures::*; +use crate::graph::object::{DigestStrategy, EncodingFormat}; use crate::storage::fs::{FsRepository, OpenFsRepository}; use crate::storage::{Repository, RepositoryHandle}; -use crate::tracking; +use crate::{tracking, Config}; -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_render_manifest(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_render_manifest( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let storage = OpenFsRepository::create(tmpdir.path().join("storage")) .await .unwrap(); @@ -52,9 +66,22 @@ async fn test_render_manifest(tmpdir: tempfile::TempDir) { assert_eq!(actual.digest().unwrap(), expected.digest().unwrap()); } -#[rstest] +#[rstest( + write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers], + write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt], +)] #[tokio::test] -async fn test_render_manifest_with_repo(tmpdir: tempfile::TempDir) { +#[serial_test::serial(config)] +async fn test_render_manifest_with_repo( + tmpdir: tempfile::TempDir, + write_encoding_format: EncodingFormat, + write_digest_strategy: DigestStrategy, +) { + let mut config = Config::default(); + config.storage.encoding_format = write_encoding_format; + config.storage.digest_strategy = write_digest_strategy; + config.make_current().unwrap(); + let tmprepo = Arc::new( FsRepository::create(tmpdir.path().join("repo")) .await From 94d05bdc2931fffe20da8d908ad270661cccd5bc Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 17 Jun 2024 15:12:30 -0700 Subject: [PATCH 3/5] Shore up more spurious test failures Related to concurrent tests and config changing. Signed-off-by: J Robert Ray --- crates/spfs/src/graph/object_test.rs | 14 +++++++++++--- crates/spfs/src/storage/repository_test.rs | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/spfs/src/graph/object_test.rs b/crates/spfs/src/graph/object_test.rs index 8d3d4c9884..e144576d3d 100644 --- a/crates/spfs/src/graph/object_test.rs +++ b/crates/spfs/src/graph/object_test.rs @@ -86,17 +86,25 @@ fn test_digest_with_salting() { } #[rstest] -fn test_digest_with_encoding() { +#[case::legacy(DigestStrategy::Legacy)] +#[case::kind_and_salt(DigestStrategy::WithKindAndSalt)] +fn test_digest_with_encoding(#[case] digest_strategy: DigestStrategy) { // check that two objects with the same digest strategy // can be saved with two different encoding methods and // still yield the same result let legacy_platform = Platform::builder() - .with_header(|h| h.with_encoding_format(EncodingFormat::Legacy)) + .with_header(|h| { + h.with_digest_strategy(digest_strategy) + .with_encoding_format(EncodingFormat::Legacy) + }) .build() .digest() .unwrap(); let flatbuf_platform = Platform::builder() - .with_header(|h| h.with_encoding_format(EncodingFormat::FlatBuffers)) + .with_header(|h| { + h.with_digest_strategy(digest_strategy) + .with_encoding_format(EncodingFormat::FlatBuffers) + }) .build() .digest() .unwrap(); diff --git a/crates/spfs/src/storage/repository_test.rs b/crates/spfs/src/storage/repository_test.rs index 21b86f39fc..89590acde2 100644 --- a/crates/spfs/src/storage/repository_test.rs +++ b/crates/spfs/src/storage/repository_test.rs @@ -141,6 +141,8 @@ async fn test_commit_broken_link( #[case::tar(tmprepo("tar"))] #[cfg_attr(feature = "server", case::rpc(tmprepo("rpc")))] #[tokio::test] +// This test just needs the config to not change while it is running. +#[serial_test::serial(config)] async fn test_commit_dir( #[case] #[future] From 11a027e971156446bebdc40722be6367641add09 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 17 Jun 2024 16:14:12 -0700 Subject: [PATCH 4/5] Serialize more tests sensitive to config changes Signed-off-by: J Robert Ray --- crates/spfs/src/sync_test.rs | 2 ++ crates/spfs/src/tracking/manifest_test.rs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/crates/spfs/src/sync_test.rs b/crates/spfs/src/sync_test.rs index b6455924bf..719f95784b 100644 --- a/crates/spfs/src/sync_test.rs +++ b/crates/spfs/src/sync_test.rs @@ -207,6 +207,8 @@ async fn test_sync_missing_from_source( #[case::tar(tmprepo("tar"), tmprepo("tar"))] #[cfg_attr(feature = "server", case::rpc(tmprepo("rpc"), tmprepo("rpc")))] #[tokio::test] +// This test just needs the config to not change while it is running. +#[serial_test::serial(config)] async fn test_sync_through_tar( #[case] #[future] diff --git a/crates/spfs/src/tracking/manifest_test.rs b/crates/spfs/src/tracking/manifest_test.rs index 38cda506b6..301268d900 100644 --- a/crates/spfs/src/tracking/manifest_test.rs +++ b/crates/spfs/src/tracking/manifest_test.rs @@ -80,6 +80,8 @@ async fn test_manifest_sorting(tmpdir: tempfile::TempDir) { } #[rstest] #[tokio::test] +// This test just needs the config to not change while it is running. +#[serial_test::serial(config)] async fn test_layer_manifests(tmpdir: tempfile::TempDir) { let a_dir = tmpdir.path().join("a"); ensure(a_dir.join("a.txt"), "a"); From a704938441c2c4e610739fd57e4ee794684967c5 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 17 Jun 2024 16:28:34 -0700 Subject: [PATCH 5/5] More test serialization Signed-off-by: J Robert Ray --- crates/spfs/src/sync_test.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/spfs/src/sync_test.rs b/crates/spfs/src/sync_test.rs index 719f95784b..3af465e32d 100644 --- a/crates/spfs/src/sync_test.rs +++ b/crates/spfs/src/sync_test.rs @@ -77,6 +77,8 @@ async fn test_push_ref(#[future] config: (tempfile::TempDir, Config)) { #[case::tar(tmprepo("tar"), tmprepo("tar"))] #[cfg_attr(feature = "server", case::rpc(tmprepo("rpc"), tmprepo("rpc")))] #[tokio::test] +// This test just needs the config to not change while it is running. +#[serial_test::serial(config)] async fn test_sync_ref( #[case] #[future]