Use insta for snapshot testing of formatter output (#856)

Signed-off-by: John Kastner <[email protected]>

Author: john-h-kastner-aws

cedar-policy-formatter/Cargo.toml

@@ -17,3 +17,6 @@ itertools = "0.12"
 smol_str = { version = "0.2", features = ["serde"] }
 regex = { version= "1.9.1", features = ["unicode"] }
 miette = { version = "7.1.0" }
+
+[dev-dependencies]
+insta = { version = "1.38.0", features = ["glob"] }

cedar-policy-formatter/README.md

@@ -34,3 +34,18 @@ cedar format -h
 
 Generated documentation for the latest version can be accessed on
 [docs.rs](https://docs.rs/cedar-policy-formatter).
+
+## Testing
+
+Tests for this package use [`insta`](https://insta.rs/) for snapshot testing.
+Tests are executed with the usual `cargo test`, but, if your changes introduce
+a difference in how we format any of the test policies, the tests will fail,
+printing a nicely formatted diff as the error message. You can then run `cargo insta review`
+to review how formatting has changed. If the change is expected, accept it
+and commit the updated snapshot file. Otherwise, reject the change, and fix it
+as you would any other failing test case.
+
+You can add new test cases just just by placing a `.cedar` file in the `tests`
+directory. The next run of `cargo test` will fail because there is no snapshot
+file. Run `cargo insta review` to review the formatted output for the new
+tests. Accept and commit the snapshot if it is correct.

cedar-policy-formatter/src/pprint/fmt.rs

@@ -153,42 +153,10 @@ pub fn policies_str_to_pretty(ps: &str, config: &Config) -> Result<String> {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
-    const TEST_CONFIG: &Config = &Config {
-        line_width: 40,
-        indent_width: 2,
-    };
-
-    #[test]
-    fn trivial_permit() {
-        let policy = r#"permit (principal, action, resource);"#;
-        assert_eq!(policies_str_to_pretty(policy, TEST_CONFIG).unwrap(), policy);
-    }
+    use insta::{assert_snapshot, glob, with_settings};
+    use std::fs;
 
-    #[test]
-    fn trivial_forbid() {
-        let policy = r#"forbid (principal, action, resource);"#;
-        assert_eq!(policies_str_to_pretty(policy, TEST_CONFIG).unwrap(), policy);
-    }
-
-    #[test]
-    fn action_in_set() {
-        let policy = r#"permit (
-        principal in UserGroup::"abc",
-        action in [Action::"viewPhoto", Action::"viewComments"],
-        resource in Album::"one"
-      );"#;
-        assert_eq!(
-            policies_str_to_pretty(policy, TEST_CONFIG).unwrap(),
-            r#"permit (
-  principal in UserGroup::"abc",
-  action in
-    [Action::"viewPhoto",
-     Action::"viewComments"],
-  resource in Album::"one"
-);"#
-        );
-    }
+    use super::*;
 
     #[test]
     fn test_soundness_check() {
@@ -235,30 +203,44 @@ mod tests {
 
     #[test]
     fn test_format_files() {
-        use std::fs::read_to_string;
-        use std::path::Path;
-
         let config = Config {
             line_width: 80,
             indent_width: 2,
         };
-        let dir_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests");
-        let pairs = vec![
-            ("test.cedar", "test_formatted.cedar"),
-            ("policies.cedar", "policies_formatted.cedar"),
-            ("is_policies.cedar", "is_policies_formatted.cedar"),
-        ];
-        for (pf, ef) in pairs {
-            // editors or cargo run try to append a newline at the end of files
-            // we should remove them for equality testing
-            assert_eq!(
-                policies_str_to_pretty(&read_to_string(dir_path.join(pf)).unwrap(), &config)
-                    .unwrap()
-                    .trim_end_matches('\n'),
-                read_to_string(dir_path.join(ef))
-                    .unwrap()
-                    .trim_end_matches('\n')
-            );
-        }
+
+        // This test uses `insta` to test the current output of the formatter
+        // against the output from prior versions. Run the test as usual with
+        // `cargo test`.
+        //
+        // If it fails, then use `cargo insta review` to review the diff between
+        // the current output and the snapshot. If the change is expected, you
+        // can accept the changes to make `insta` update the snapshot which you
+        // should the commit to the repository.
+        //
+        // Add new tests by placing a `.cedar` file in the test directory. The
+        // next run of `cargo test` will fail. Use `cargo insta review` to check
+        // the formatted output is expected.
+        with_settings!(
+            { snapshot_path => "../../tests/snapshots/" },
+            {
+                glob!("../../tests", "*.cedar", |path| {
+                    let cedar_source = fs::read_to_string(path).unwrap();
+                    let formatted = policies_str_to_pretty(&cedar_source, &config).unwrap();
+                    assert_snapshot!(formatted);
+                });
+            }
+        );
+
+        // Also check the CLI sample files.
+        with_settings!(
+            { snapshot_path => "../../tests/cli-snapshots/" },
+            {
+                glob!("../../../cedar-policy-cli/sample-data", "**/*.cedar", |path| {
+                    let cedar_source = fs::read_to_string(path).unwrap();
+                    let formatted = policies_str_to_pretty(&cedar_source, &config).unwrap();
+                    assert_snapshot!(formatted);
+                });
+            }
+        )
     }
 }

cedar-policy-formatter/tests/action_in_set.cedar

@@ -0,0 +1,5 @@
+permit (
+        principal in UserGroup::"abc",
+        action in [Action::"viewPhoto", Action::"viewComments"],
+        resource in Album::"one"
+      );

cedar-policy-formatter/tests/cli-snapshots/cedar_policy_formatter__pprint__fmt__tests__cli_sample_data_format@sandbox_a__policies_1.cedar.snap

@@ -0,0 +1,20 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-cli/sample-data/sandbox_a/policies_1.cedar
+---
+// Everyone in the group UserGroup::"jane_friends" can view this specific photo
+@id("jane's friends view-permission policy")
+permit (
+  principal in UserGroup::"jane_friends",
+  action == Action::"view",
+  resource == Photo::"VacationPhoto94.jpg"
+);
+
+// but Tim is disallowed from viewing the photo
+@id("disallow tim policy")
+forbid (
+  principal == User::"tim",
+  action,
+  resource == Photo::"VacationPhoto94.jpg"
+);

cedar-policy-formatter/tests/cli-snapshots/cedar_policy_formatter__pprint__fmt__tests__cli_sample_data_format@sandbox_a__policies_1_bad.cedar.snap

@@ -0,0 +1,20 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-cli/sample-data/sandbox_a/policies_1_bad.cedar
+---
+// Everyone in the group UserGroup::"jane_friends" can view this specific photo
+@id("jane's friends view-permission policy")
+permit (
+  principal in UsrGroup::"jane_friends",
+  action == Action::"view",
+  resource == Photo::"VacationPhoto94.jpg"
+);
+
+// but Tim is disallowed from viewing the photo
+@id("disallow tim policy")
+forbid (
+  principal == User::"tim",
+  action,
+  resource == Photo::"VacationPhoto94.jpg"
+);

cedar-policy-formatter/tests/cli-snapshots/cedar_policy_formatter__pprint__fmt__tests__cli_sample_data_format@sandbox_a__policies_2.cedar.snap

@@ -0,0 +1,20 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-cli/sample-data/sandbox_a/policies_2.cedar
+---
+// Alice can view, edit, or delete any photo in the "jane_vacation" album
+@id("alice's access policy")
+permit (
+  principal == User::"alice",
+  action in [Action::"view", Action::"edit", Action::"delete"],
+  resource in Album::"jane_vacation"
+);
+
+// Bob can only view things in the "jane_vacation" album
+@id("bob's view policy")
+permit (
+  principal == User::"bob",
+  action == Action::"view",
+  resource in Album::"jane_vacation"
+);

cedar-policy-formatter/tests/cli-snapshots/cedar_policy_formatter__pprint__fmt__tests__cli_sample_data_format@sandbox_a__policies_3.cedar.snap

@@ -0,0 +1,13 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-cli/sample-data/sandbox_a/policies_3.cedar
+---
+// Everyone can view the photos in the "jane_vacation" album
+// (and list the photos in the album)
+@id("jane_vacation public")
+permit (
+  principal,
+  action in [Action::"view", Action::"listPhotos"],
+  resource in Album::"jane_vacation"
+);

cedar-policy-formatter/tests/cli-snapshots/cedar_policy_formatter__pprint__fmt__tests__cli_sample_data_format@sandbox_b__policies_4.cedar.snap

@@ -0,0 +1,15 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-cli/sample-data/sandbox_b/policies_4.cedar
+---
+// Only members of the HardwareEngineering department with job level >= 5 can
+// view photos in device_prototypes
+@id("prototypes access policy")
+permit (
+  principal,
+  action == Action::"view",
+  resource in Album::"device_prototypes"
+)
+when
+{ principal.department == "HardwareEngineering" && principal.jobLevel >= 5 };

cedar-policy-formatter/tests/cli-snapshots/cedar_policy_formatter__pprint__fmt__tests__cli_sample_data_format@sandbox_b__policies_5.cedar.snap

@@ -0,0 +1,19 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-cli/sample-data/sandbox_b/policies_5.cedar
+---
+// Alice's friends can view all of her photos
+@id("alice's friends view policy")
+permit (
+  principal in UserGroup::"alice_friends",
+  action == Action::"view",
+  resource in Account::"alice"
+);
+
+// but, as a general rule, anything marked private can only be viewed by the
+// account owner
+@id("privacy rule")
+forbid (principal, action, resource)
+when { resource.private }
+unless { resource.account has owner && resource.account.owner == principal };