Test Parameters::import() with snapshot (MystenLabs#610)

* snapshot test Parameters::import() and fix typos

* ignore git precommit hook

* add instructions

narwhal/.gitignore

@@ -16,6 +16,9 @@ target/
 !/**/src/**/target/
 .test_*
 
+# Configuration for pre-commit / Git precommit hook.
+.pre-commit*
+
 # Generated by Cargo
 # will have compiled files and executables
 /target/

narwhal/CONTRIBUTING.md

@@ -18,11 +18,11 @@ on the Clippy repository. We make use of a cargo alias (defined in
 .cargo/config) which sets some project-wide lints. To run the linter locally
 you can use the following command:
 ```
-cargo xclippy 
+cargo xclippy
 ```
 
 ## Run the formatter
-To ensure that the codebase is following standard formatting properties, we use  
+To ensure that the codebase is following standard formatting properties, we use
 [Rustfmt](https://github.com/rust-lang/rustfmt). To run the formatter locally
 you can use the follow command:
 ```
@@ -39,11 +39,16 @@ The [derive_builder](https://crates.io/crates/derive_builder) crate has been use
 auto-generate builders (following the [builder design pattern](https://en.wikipedia.org/wiki/Builder_pattern)) for structs. Instead of having to write (lots) of boilerplate
 code to create a builder, this is offered by the derive_builder and is the recommended
 way to create builders for this repo. Examples can be found within the repo and on the
-crate docs as well. 
+crate docs as well.
+
+## Testing
+One thing to note is that Narwhal uses snapshot testing for configs. See the beginning of the file
+[config/tests/config_tests.rs](config/tests/config_tests.rs) for background and instructions if your PR breaks the test.
 
 ## Issues
 We use GitHub issues to track public bugs. Please ensure your description is
 clear and has sufficient instructions to be able to reproduce the issue.
+
 ## License
 By contributing to Narwhal and Tusk, you agree that your contributions will be licensed
 under the LICENSE file in the root directory of this source tree.

narwhal/config/src/lib.rs

@@ -47,7 +47,7 @@ pub enum ConfigError {
 }
 
 #[derive(Error, Debug)]
-pub enum ComitteeUpdateError {
+pub enum CommitteeUpdateError {
     #[error("Node {0} is not in the committee")]
     NotInCommittee(String),
 
@@ -98,7 +98,7 @@ pub type WorkerId = u32;
 /// Holds all the node properties. An example is provided to
 /// showcase the usage and deserialization from a json file.
 /// To define a Duration on the property file can use either
-/// miliseconds or seconds (e.x 5s, 10ms , 2000ms).
+/// milliseconds or seconds (e.x 5s, 10ms , 2000ms).
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct Parameters {
     /// The preferred header size. The primary creates a new header when it has enough parents and
@@ -458,7 +458,7 @@ impl Committee {
     pub fn update_primary_network_info(
         &mut self,
         mut new_info: BTreeMap<PublicKey, (Stake, PrimaryAddresses)>,
-    ) -> Result<(), Vec<ComitteeUpdateError>> {
+    ) -> Result<(), Vec<CommitteeUpdateError>> {
         let mut errors = None;
 
         let table = &self.authorities;
@@ -492,22 +492,22 @@ impl Committee {
                         // Stake does not match: create or append error
                         push_error_and_return(
                             acc,
-                            ComitteeUpdateError::DifferentStake(pk.to_string()),
+                            CommitteeUpdateError::DifferentStake(pk.to_string()),
                         )
                     }
                 } else {
                     // This key is absent from new information
                     push_error_and_return(
                         acc,
-                        ComitteeUpdateError::MissingFromUpdate(pk.to_string()),
+                        CommitteeUpdateError::MissingFromUpdate(pk.to_string()),
                     )
                 }
             });
 
         // If there are elements left in new_info, they are not in the original table
         // If new_info is empty, this is a no-op.
         let res = new_info.iter().fold(res, |acc, (pk, _)| {
-            push_error_and_return(acc, ComitteeUpdateError::NotInCommittee(pk.to_string()))
+            push_error_and_return(acc, CommitteeUpdateError::NotInCommittee(pk.to_string()))
         });
 
         match res {
@@ -523,99 +523,9 @@ impl Committee {
 
 #[cfg(test)]
 mod tests {
-    use crate::{Import, Parameters};
-    use std::{fs::File, io::Write};
-    use tempfile::tempdir;
+    use crate::Parameters;
     use tracing_test::traced_test;
 
-    #[test]
-    #[traced_test]
-    fn parse_properties() {
-        // GIVEN
-        let input = r#"{
-             "header_size": 1000,
-             "max_header_delay": "100ms",
-             "gc_depth": 50,
-             "sync_retry_delay": "5s",
-             "sync_retry_nodes": 3,
-             "batch_size": 500000,
-             "max_batch_delay": "100ms",
-             "block_synchronizer": {
-                 "certificates_synchronize_timeout": "2s",
-                 "payload_synchronize_timeout": "3_000ms",
-                 "payload_availability_timeout": "4_000ms",
-                 "handler_certificate_deliver_timeout": "1_000ms"
-             },
-             "consensus_api_grpc": {
-                 "socket_addr": "/ip4/127.0.0.1/tcp/0/http",
-                 "get_collections_timeout": "5_000ms",
-                 "remove_collections_timeout": "5_000ms"
-             },
-             "max_concurrent_requests": 500000,
-             "prometheus_metrics": {
-                 "socket_addr": "127.0.0.1:0"
-             }
-          }"#;
-
-        // AND temporary file
-        let dir = tempdir().expect("Couldn't create tempdir");
-
-        let file_path = dir.path().join("temp-properties.json");
-        let mut file = File::create(file_path.clone()).expect("Couldn't create temp file");
-
-        // AND write the json context
-        writeln!(file, "{input}").expect("Couldn't write to file");
-
-        // WHEN
-        let params = Parameters::import(file_path.to_str().unwrap()).expect("Error raised");
-
-        // THEN
-        assert_eq!(params.sync_retry_delay.as_millis(), 5_000);
-        assert_eq!(
-            params
-                .block_synchronizer
-                .certificates_synchronize_timeout
-                .as_millis(),
-            2_000
-        );
-        assert_eq!(
-            params
-                .block_synchronizer
-                .payload_synchronize_timeout
-                .as_millis(),
-            3_000
-        );
-        assert_eq!(
-            params
-                .block_synchronizer
-                .payload_availability_timeout
-                .as_millis(),
-            4_000
-        );
-        assert_eq!(
-            params.consensus_api_grpc.socket_addr,
-            "/ip4/127.0.0.1/tcp/0/http".parse().unwrap(),
-        );
-        assert_eq!(
-            params
-                .consensus_api_grpc
-                .get_collections_timeout
-                .as_millis(),
-            5_000
-        );
-        assert_eq!(
-            params
-                .consensus_api_grpc
-                .remove_collections_timeout
-                .as_millis(),
-            5_000
-        );
-        assert_eq!(
-            params.prometheus_metrics.socket_addr.to_string(),
-            "127.0.0.1:0",
-        );
-    }
-
     #[test]
     #[traced_test]
     fn tracing_should_print_parameters() {

narwhal/config/tests/config_tests.rs

@@ -1,15 +1,32 @@
 // Copyright (c) 2022, Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
+// This file contains tests that detect changes in Narwhal configs and parameters.
+// If a PR breaks one or more tests here, the PR probably has a real impact
+// on a Narwhal configuration file. When test failure happens, the PR should
+// be marked as a breaking change and reviewers should be aware of this.
+//
+// Owners and operators of production configuration files can add themselves to
+// .github/CODEOWNERS for the corresponding snapshot tests, so they can get notified
+// of changes. PRs that modifies snapshot files should wait for reviews from
+// code owners (if any) before merging.
+//
+// To review snapshot changes, and fix snapshot differences,
+// 0. Install cargo-insta
+// 1. Run `cargo insta test --review` under `./config`.
+// 2. Review, accept or reject changes.
+
 use std::collections::BTreeMap;
 
 use config::{
-    Committee, ConsensusAPIGrpcParameters, Epoch, Parameters, PrimaryAddresses,
+    Committee, ConsensusAPIGrpcParameters, Epoch, Import, Parameters, PrimaryAddresses,
     PrometheusMetricsParameters, Stake,
 };
 use crypto::{traits::KeyPair as _, PublicKey};
 use insta::assert_json_snapshot;
 use rand::seq::SliceRandom;
+use std::{fs::File, io::Write};
+use tempfile::tempdir;
 use test_utils::make_authority_with_port_getter;
 
 #[test]
@@ -22,7 +39,7 @@ fn update_primary_network_info_test() {
     for err in res {
         assert!(matches!(
             err,
-            config::ComitteeUpdateError::MissingFromUpdate(_)
+            config::CommitteeUpdateError::MissingFromUpdate(_)
         ))
     }
 
@@ -40,8 +57,8 @@ fn update_primary_network_info_test() {
         // we'll get the two collections reporting missing from each other
         assert!(matches!(
             err,
-            config::ComitteeUpdateError::NotInCommittee(_)
-                | config::ComitteeUpdateError::MissingFromUpdate(_)
+            config::CommitteeUpdateError::NotInCommittee(_)
+                | config::CommitteeUpdateError::MissingFromUpdate(_)
         ))
     }
 
@@ -59,7 +76,7 @@ fn update_primary_network_info_test() {
     for err in res2 {
         assert!(matches!(
             err,
-            config::ComitteeUpdateError::DifferentStake(_)
+            config::CommitteeUpdateError::DifferentStake(_)
         ))
     }
 
@@ -140,3 +157,47 @@ fn commmittee_snapshot_matches() {
     settings.set_sort_maps(true);
     settings.bind(|| assert_json_snapshot!("committee", committee));
 }
+
+#[test]
+fn parameters_import_snapshot_matches() {
+    // GIVEN
+    let input = r#"{
+         "header_size": 1000,
+         "max_header_delay": "100ms",
+         "gc_depth": 50,
+         "sync_retry_delay": "5s",
+         "sync_retry_nodes": 3,
+         "batch_size": 500000,
+         "max_batch_delay": "100ms",
+         "block_synchronizer": {
+             "certificates_synchronize_timeout": "2s",
+             "payload_synchronize_timeout": "3_000ms",
+             "payload_availability_timeout": "4_000ms",
+             "handler_certificate_deliver_timeout": "1_000ms"
+         },
+         "consensus_api_grpc": {
+             "socket_addr": "/ip4/127.0.0.1/tcp/0/http",
+             "get_collections_timeout": "5_000ms",
+             "remove_collections_timeout": "5_000ms"
+         },
+         "max_concurrent_requests": 500000,
+         "prometheus_metrics": {
+             "socket_addr": "127.0.0.1:0"
+         }
+      }"#;
+
+    // AND temporary file
+    let dir = tempdir().expect("Couldn't create tempdir");
+
+    let file_path = dir.path().join("temp-properties.json");
+    let mut file = File::create(file_path.clone()).expect("Couldn't create temp file");
+
+    // AND write the json context
+    writeln!(file, "{input}").expect("Couldn't write to file");
+
+    // WHEN
+    let params = Parameters::import(file_path.to_str().unwrap()).expect("Error raised");
+
+    // THEN
+    assert_json_snapshot!("parameters_import", params)
+}

narwhal/config/tests/snapshots/config_tests__parameters_import.snap

@@ -0,0 +1,28 @@
+---
+source: config/tests/config_tests.rs
+expression: params
+---
+{
+  "header_size": 1000,
+  "max_header_delay": "100ms",
+  "gc_depth": 50,
+  "sync_retry_delay": "5000ms",
+  "sync_retry_nodes": 3,
+  "batch_size": 500000,
+  "max_batch_delay": "100ms",
+  "block_synchronizer": {
+    "certificates_synchronize_timeout": "2000ms",
+    "payload_synchronize_timeout": "3000ms",
+    "payload_availability_timeout": "4000ms",
+    "handler_certificate_deliver_timeout": "1000ms"
+  },
+  "consensus_api_grpc": {
+    "socket_addr": "/ip4/127.0.0.1/tcp/0/http",
+    "get_collections_timeout": "5000ms",
+    "remove_collections_timeout": "5000ms"
+  },
+  "max_concurrent_requests": 500000,
+  "prometheus_metrics": {
+    "socket_addr": "127.0.0.1:0"
+  }
+}