Rework storage tests to test both encoding formats

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" }

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 }
 

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();

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,18 +88,29 @@ 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()
         .into_layer()
         .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

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();