Add more property tests (#22)

* Add more property tests.

Use the test_strategy::proptest macro for nice async support.

Upstream proptest+tokio issue:
proptest-rs/proptest#179

While there is a workaround there, the `prop_assert!` macro didn't
seem to work properly. The `assert!` macro does work, but then we see
all the output when proptest tries to shrink a failing example.

* More unwraps to assert

* Add some array tests

* hypothesis style naming

* fix comment

* Better array metadata strategy

* Use Arbitrary

* rename

* Add regression file

* bump version

* Option<DimensionNames>

* More feedbaclk

* Move strategies module under tests

* Make test-strategy dev dep only

* Revert "Make test-strategy dev dep only"

This reverts commit 93f5ed5.

* cleanup

* Tweak one flatmap

* Move strategies to new module

* Align dimensionname type with spec

* Check error types where we can

* Implement fmt::Debug trait on Storage

* update cargo.toml

Cargo.toml

@@ -20,12 +20,13 @@ thiserror = "1.0.63"
 serde_json = "1.0.125"
 serde = { version = "1.0.208", features = ["derive"] }
 serde_with = "3.9.0"
+test-strategy = "0.4.0"
+proptest = "1.5.0"
 
 [profile.release-with-debug]
 inherits = "release"
 debug = true
 
 [dev-dependencies]
 pretty_assertions = "1.4.0"
-proptest = "1.0.0"
 tokio = { version = "1.39.2", features = ["rt-multi-thread", "macros"] }

proptest-regressions/dataset.txt

@@ -0,0 +1,7 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc 10b7f8cfd2afb88ddee1d4abf0d6f1bb9e6c6bc87af8c2c0d5e90e49d50dede5 # shrinks to path = ""

src/dataset.rs

@@ -31,8 +31,6 @@ impl ChangeSet {
         path: Path,
         node_id: NodeId,
     ) -> Result<(), DeleteNodeError> {
-        // TODO: test delete a deleted group.
-        // TODO: test delete a non-existent group
         let was_new = self.new_groups.remove(&path).is_some();
         self.updated_attributes.remove(&path);
         if !was_new {
@@ -722,10 +720,11 @@ pub enum FlushError {
 
 #[cfg(test)]
 mod tests {
+
     use std::{error::Error, num::NonZeroU64, path::PathBuf};
 
     use crate::{
-        manifest::mk_manifests_table, storage::InMemoryStorage,
+        manifest::mk_manifests_table, storage::InMemoryStorage, strategies::*,
         structure::mk_structure_table, ChunkInfo, ChunkKeyEncoding, ChunkRef, ChunkShape,
         Codec, DataType, FillValue, Flags, ManifestExtents, StorageTransformer,
         TableRegion,
@@ -734,6 +733,110 @@ mod tests {
     use super::*;
     use itertools::Itertools;
     use pretty_assertions::assert_eq;
+    use proptest::prelude::{prop_assert, prop_assert_eq};
+    use test_strategy::proptest;
+
+    #[proptest(async = "tokio")]
+    async fn test_add_delete_group(
+        #[strategy(node_paths())] path: Path,
+        #[strategy(empty_datasets())] mut dataset: Dataset,
+    ) {
+        // getting any path from an empty dataset must fail
+        prop_assert!(dataset.get_node(&path).await.is_err());
+
+        // adding a new group must succeed
+        prop_assert!(dataset.add_group(path.clone()).await.is_ok());
+
+        // Getting a group just added must succeed
+        let node = dataset.get_node(&path).await;
+        prop_assert!(node.is_ok());
+
+        // Getting the group twice must be equal
+        prop_assert_eq!(node.unwrap(), dataset.get_node(&path).await.unwrap());
+
+        // adding an existing group fails
+        prop_assert_eq!(
+            dataset.add_group(path.clone()).await.unwrap_err(),
+            AddNodeError::AlreadyExists(path.clone())
+        );
+
+        // deleting the added group must succeed
+        prop_assert!(dataset.delete_group(path.clone()).await.is_ok());
+
+        // deleting twice must fail
+        prop_assert_eq!(
+            dataset.delete_group(path.clone()).await.unwrap_err(),
+            DeleteNodeError::NotFound(path.clone())
+        );
+
+        // getting a deleted group must fail
+        prop_assert!(dataset.get_node(&path).await.is_err());
+
+        // adding again must succeed
+        prop_assert!(dataset.add_group(path.clone()).await.is_ok());
+
+        // deleting again must succeed
+        prop_assert!(dataset.delete_group(path.clone()).await.is_ok());
+    }
+
+    #[proptest(async = "tokio")]
+    async fn test_add_delete_array(
+        #[strategy(node_paths())] path: Path,
+        #[strategy(zarr_array_metadata())] metadata: ZarrArrayMetadata,
+        #[strategy(empty_datasets())] mut dataset: Dataset,
+    ) {
+        // new array must always succeed
+        prop_assert!(dataset.add_array(path.clone(), metadata.clone()).await.is_ok());
+
+        // adding to the same path must fail
+        prop_assert!(dataset.add_array(path.clone(), metadata.clone()).await.is_err());
+
+        // first delete must succeed
+        prop_assert!(dataset.delete_array(path.clone()).await.is_ok());
+
+        // deleting twice must fail
+        prop_assert_eq!(
+            dataset.delete_array(path.clone()).await.unwrap_err(),
+            DeleteNodeError::NotFound(path.clone())
+        );
+
+        // adding again must succeed
+        prop_assert!(dataset.add_array(path.clone(), metadata.clone()).await.is_ok());
+
+        // deleting again must succeed
+        prop_assert!(dataset.delete_array(path.clone()).await.is_ok());
+    }
+
+    #[proptest(async = "tokio")]
+    async fn test_add_array_group_clash(
+        #[strategy(node_paths())] path: Path,
+        #[strategy(zarr_array_metadata())] metadata: ZarrArrayMetadata,
+        #[strategy(empty_datasets())] mut dataset: Dataset,
+    ) {
+        // adding a group at an existing array node must fail
+        prop_assert!(dataset.add_array(path.clone(), metadata.clone()).await.is_ok());
+        prop_assert_eq!(
+            dataset.add_group(path.clone()).await.unwrap_err(),
+            AddNodeError::AlreadyExists(path.clone())
+        );
+        prop_assert_eq!(
+            dataset.delete_group(path.clone()).await.unwrap_err(),
+            DeleteNodeError::NotAGroup(path.clone())
+        );
+        prop_assert!(dataset.delete_array(path.clone()).await.is_ok());
+
+        // adding an array at an existing group node must fail
+        prop_assert!(dataset.add_group(path.clone()).await.is_ok());
+        prop_assert_eq!(
+            dataset.add_array(path.clone(), metadata.clone()).await.unwrap_err(),
+            AddNodeError::AlreadyExists(path.clone())
+        );
+        prop_assert_eq!(
+            dataset.delete_array(path.clone()).await.unwrap_err(),
+            DeleteNodeError::NotAnArray(path.clone())
+        );
+        prop_assert!(dataset.delete_group(path.clone()).await.is_ok());
+    }
 
     #[tokio::test(flavor = "multi_thread")]
     async fn test_dataset_with_updates() -> Result<(), Box<dyn Error>> {

src/lib.rs

@@ -24,6 +24,7 @@
 pub mod dataset;
 pub mod manifest;
 pub mod storage;
+pub mod strategies;
 pub mod structure;
 pub mod zarr;
 
@@ -44,6 +45,7 @@ use std::{
     sync::Arc,
 };
 use structure::StructureTable;
+use test_strategy::Arbitrary;
 use thiserror::Error;
 
 #[derive(Debug, Clone)]
@@ -59,6 +61,8 @@ pub struct ArrayIndices(pub Vec<u64>);
 /// The shape of an array.
 /// 0 is a valid shape member
 pub type ArrayShape = Vec<u64>;
+pub type DimensionName = Option<String>;
+pub type DimensionNames = Vec<DimensionName>;
 
 pub type Path = PathBuf;
 
@@ -197,7 +201,7 @@ struct NameConfigSerializer {
     configuration: serde_json::Value,
 }
 
-#[derive(Copy, Clone, Eq, PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Arbitrary, Copy, Clone, Eq, PartialEq, Debug, Serialize, Deserialize)]
 pub enum ChunkKeyEncoding {
     Slash,
     Dot,
@@ -263,7 +267,7 @@ impl TryFrom<NameConfigSerializer> for ChunkKeyEncoding {
     }
 }
 
-#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+#[derive(Arbitrary, Clone, Debug, PartialEq, Serialize, Deserialize)]
 #[serde(untagged)]
 pub enum FillValue {
     // FIXME: test all json (de)serializations
@@ -503,8 +507,6 @@ pub struct StorageTransformer {
     pub configuration: Option<HashMap<String, serde_json::Value>>,
 }
 
-pub type DimensionName = String;
-
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub struct UserAttributes {
     #[serde(flatten)]
@@ -608,7 +610,7 @@ pub struct ZarrArrayMetadata {
     pub codecs: Vec<Codec>,
     pub storage_transformers: Option<Vec<StorageTransformer>>,
     // each dimension name can be null in Zarr
-    pub dimension_names: Option<Vec<Option<DimensionName>>>,
+    pub dimension_names: Option<DimensionNames>,
 }
 
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]
@@ -728,7 +730,7 @@ pub enum StorageError {
 /// Different implementation can cache the files differently, or not at all.
 /// Implementations are free to assume files are never overwritten.
 #[async_trait]
-pub trait Storage {
+pub trait Storage: fmt::Debug {
     async fn fetch_structure(
         &self,
         id: &ObjectId,
@@ -765,7 +767,7 @@ pub trait Storage {
     async fn write_chunk(&self, id: ObjectId, bytes: Bytes) -> Result<(), StorageError>;
 }
 
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Dataset {
     storage: Arc<dyn Storage + Send + Sync>,
     structure_id: Option<ObjectId>,

src/storage.rs

@@ -1,6 +1,7 @@
 use base64::{engine::general_purpose::URL_SAFE as BASE64_URL_SAFE, Engine as _};
 use std::{
     collections::HashMap,
+    fmt,
     fs::create_dir_all,
     ops::Range,
     sync::{Arc, RwLock},
@@ -60,7 +61,7 @@ impl ObjectStorage {
     ) -> Result<ObjectStorage, StorageError> {
         use object_store::aws::AmazonS3Builder;
         let store = AmazonS3Builder::new()
-            // TODO: Generalize the auth config
+            // FIXME: Generalize the auth config
             .with_access_key_id("minio123")
             .with_secret_access_key("minio123")
             .with_endpoint("http://localhost:9000")
@@ -108,6 +109,11 @@ impl ObjectStorage {
     }
 }
 
+impl fmt::Debug for ObjectStorage {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "ObjectStorage, prefix={}, store={}", self.prefix, self.store)
+    }
+}
 #[async_trait]
 impl Storage for ObjectStorage {
     async fn fetch_structure(
@@ -221,6 +227,12 @@ impl InMemoryStorage {
     }
 }
 
+impl fmt::Debug for InMemoryStorage {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "InMemoryStorage at {:p}", self)
+    }
+}
+
 #[async_trait]
 impl Storage for InMemoryStorage {
     async fn fetch_structure(