Repository: cedar-policy/cedar
Pull request #1519 (2025-03-03 19:05:23)

Fix warnings in tests due to camel case name
## Description of changes

## Issue #, if available

## Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

- [ ] A breaking change requiring a major version bump to `cedar-policy` (e.g., changes to the signature of an existing API).
- [ ] A backwards-compatible change requiring a minor version bump to `cedar-policy` (e.g., addition of a new API).
- [ ] A bug fix or other functionality change requiring a patch to `cedar-policy`.
- [x] A change "invisible" to users (e.g., documentation, changes to "internal" crates like `cedar-policy-core`, `cedar-validator`, etc.)
- [ ] A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

- [ ] Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).
- [x] Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that [`cedar-spec`](https://github.com/cedar-policy/cedar-spec) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.
- [ ] Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in `cedar-spec`, and how you have tested that your updates are correct.)
- [ ] Requires updates, but I do not plan to make them in the near future. (Make sure that your changes are hidden behind a feature flag to mark them as experimental.)
- [ ] I'm not sure how my change impacts `cedar-spec`. (Post your PR anyways, and we'll discuss in the comments.)

I confirm that [`docs.cedarpolicy.com`](https://docs.cedarpolicy.com/) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar language specification.
- [ ] Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in [`cedar-docs`](https://github.com/cedar-policy/cedar-docs). PRs should be targeted at a `staging-X.Y` branch, not `main`.)
- [ ] I'm not sure how my change impacts the documentation. (Post your PR anyways, and we'll discuss in the comments.)



Patch:
diff --git a/cedar-policy-core/src/ast/entity.rs b/cedar-policy-core/src/ast/entity.rs
index eef8f397..39988d50 100644
--- a/cedar-policy-core/src/ast/entity.rs
+++ b/cedar-policy-core/src/ast/entity.rs
@@ -244,10 +244,10 @@ impl StaticallyTyped for EntityUID {
     }
 }
 
+#[cfg(test)]
 impl EntityUID {
     /// Create an `EntityUID` with the given string as its EID.
     /// Useful for testing.
-    #[cfg(test)]
     pub(crate) fn with_eid(eid: &str) -> Self {
         Self::EntityUID(EntityUIDImpl {
             ty: Self::test_entity_type(),
@@ -255,25 +255,16 @@ impl EntityUID {
             loc: None,
         })
     }
-    // by default, Coverlay does not track coverage for lines after a line
-    // containing #[cfg(test)].
-    // we use the following sentinel to "turn back on" coverage tracking for
-    // remaining lines of this file, until the next #[cfg(test)]
-    // GRCOV_BEGIN_COVERAGE
 
     /// The type of entities created with the above `with_eid()`.
-    #[cfg(test)]
     pub(crate) fn test_entity_type() -> EntityType {
         let name = Name::parse_unqualified_name("test_entity_type")
             .expect("test_entity_type should be a valid identifier");
         EntityType::EntityType(name)
     }
-    // by default, Coverlay does not track coverage for lines after a line
-    // containing #[cfg(test)].
-    // we use the following sentinel to "turn back on" coverage tracking for
-    // remaining lines of this file, until the next #[cfg(test)]
-    // GRCOV_BEGIN_COVERAGE
+}
 
+impl EntityUID {
     /// Create an `EntityUID` with the given (unqualified) typename, and the given string as its EID.
     pub fn with_eid_and_type(typename: &str, eid: &str) -> Result<Self, ParseErrors> {
         Ok(Self::EntityUID(EntityUIDImpl {
@@ -671,41 +662,6 @@ impl Entity {
                 == (other.ancestors().collect::<HashSet<_>>())
     }
 
-    /// Set the UID to the given value.
-    // Only used for convenience in some tests
-    #[cfg(test)]
-    pub fn set_uid(&mut self, uid: EntityUID) {
-        self.uid = uid;
-    }
-
-    /// Set the given attribute to the given value.
-    // Only used for convenience in some tests and when fuzzing
-    #[cfg(any(test, fuzzing))]
-    pub fn set_attr(
-        &mut self,
-        attr: SmolStr,
-        val: BorrowedRestrictedExpr<'_>,
-        extensions: &Extensions<'_>,
-    ) -> Result<(), EvaluationError> {
-        let val = RestrictedEvaluator::new(extensions).partial_interpret(val)?;
-        self.attrs.insert(attr, val.into());
-        Ok(())
-    }
-
-    /// Set the given tag to the given value.
-    // Only used for convenience in some tests and when fuzzing
-    #[cfg(any(test, fuzzing))]
-    pub fn set_tag(
-        &mut self,
-        tag: SmolStr,
-        val: BorrowedRestrictedExpr<'_>,
-        extensions: &Extensions<'_>,
-    ) -> Result<(), EvaluationError> {
-        let val = RestrictedEvaluator::new(extensions).partial_interpret(val)?;
-        self.tags.insert(tag, val.into());
-        Ok(())
-    }
-
     /// Mark the given `UID` as an indirect ancestor of this `Entity`
     ///
     /// The given `UID` will not be added as an indirecty ancestor if
diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs
index 63326dc9..348ebb97 100644
--- a/cedar-policy-core/src/ast/expr.rs
+++ b/cedar-policy-core/src/ast/expr.rs
@@ -1526,20 +1526,6 @@ pub enum Var {
     Context,
 }
 
-#[cfg(test)]
-mod var_generator {
-    use super::Var;
-    #[cfg(test)]
-    pub fn all_vars() -> impl Iterator<Item = Var> {
-        [Var::Principal, Var::Action, Var::Resource, Var::Context].into_iter()
-    }
-}
-// by default, Coverlay does not track coverage for lines after a line
-// containing #[cfg(test)].
-// we use the following sentinel to "turn back on" coverage tracking for
-// remaining lines of this file, until the next #[cfg(test)]
-// GRCOV_BEGIN_COVERAGE
-
 impl From<PrincipalOrResource> for Var {
     fn from(v: PrincipalOrResource) -> Self {
         match v {
@@ -1588,7 +1574,11 @@ mod test {
 
     use crate::expr_builder::ExprBuilder as _;
 
-    use super::{var_generator::all_vars, *};
+    use super::*;
+
+    pub fn all_vars() -> impl Iterator<Item = Var> {
+        [Var::Principal, Var::Action, Var::Resource, Var::Context].into_iter()
+    }
 
     // Tests that Var::Into never panics
     #[test]
diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs
index 152831e3..0cff6d10 100644
--- a/cedar-policy-core/src/ast/policy.rs
+++ b/cedar-policy-core/src/ast/policy.rs
@@ -56,20 +56,19 @@ impl From<Template> for TemplateBody {
 
 impl Template {
     /// Checks the invariant (slot cache correctness)
-    #[cfg(test)]
+    ///
+    /// This function is a no-op in release builds, but checks the invariant (and panics if it fails) in debug builds.
     pub fn check_invariant(&self) {
-        for slot in self.body.condition().slots() {
-            assert!(self.slots.contains(&slot));
-        }
-        for slot in self.slots() {
-            assert!(self.body.condition().slots().contains(slot));
+        #[cfg(debug_assertions)]
+        {
+            for slot in self.body.condition().slots() {
+                assert!(self.slots.contains(&slot));
+            }
+            for slot in self.slots() {
+                assert!(self.body.condition().slots().contains(slot));
+            }
         }
     }
-    // by default, Coverlay does not track coverage for lines after a line
-    // containing #[cfg(test)].
-    // we use the following sentinel to "turn back on" coverage tracking for
-    // remaining lines of this file, until the next #[cfg(test)]
-    // GRCOV_BEGIN_COVERAGE
 
     /// Construct a `Template` from its components
     #[allow(clippy::too_many_arguments)]
@@ -273,15 +272,7 @@ impl Template {
             body,
             slots: vec![],
         });
-        #[cfg(test)]
-        {
-            t.check_invariant();
-        }
-        // by default, Coverlay does not track coverage for lines after a line
-        // containing #[cfg(test)].
-        // we use the following sentinel to "turn back on" coverage tracking for
-        // remaining lines of this file, until the next #[cfg(test)]
-        // GRCOV_BEGIN_COVERAGE
+        t.check_invariant();
         let p = Policy::new(Arc::clone(&t), None, HashMap::new());
         (t, p)
     }
@@ -386,15 +377,12 @@ impl Policy {
     /// INVARIANT (values total map):
     /// `values` must bind every open slot in `template`
     fn new(template: Arc<Template>, link_id: Option<PolicyID>, values: SlotEnv) -> Self {
-        #[cfg(test)]
+        #[cfg(debug_assertions)]
         {
+            // PANIC SAFETY: asserts (value total map invariant) which is justified at call sites
+            #[allow(clippy::expect_used)]
             Template::check_binding(&template, &values).expect("(values total map) does not hold!");
         }
-        // by default, Coverlay does not track coverage for lines after a line
-        // containing #[cfg(test)].
-        // we use the following sentinel to "turn back on" coverage tracking for
-        // remaining lines of this file, until the next #[cfg(test)]
-        // GRCOV_BEGIN_COVERAGE
         Self {
             template,
             link: link_id,
@@ -687,11 +675,6 @@ mod hashing_tests {
         assert_eq!(compute_hash(&a), compute_hash(&b));
     }
 }
-// by default, Coverlay does not track coverage for lines after a line
-// containing #[cfg(test)].
-// we use the following sentinel to "turn back on" coverage tracking for
-// remaining lines of this file, until the next #[cfg(test)]
-// GRCOV_BEGIN_COVERAGE
 
 /// Errors that can happen during policy reification
 #[derive(Debug, Diagnostic, Error)]
diff --git a/cedar-policy-core/src/authorizer.rs b/cedar-policy-core/src/authorizer.rs
index 3a9ac155..de9bd596 100644
--- a/cedar-policy-core/src/authorizer.rs
+++ b/cedar-policy-core/src/authorizer.rs
@@ -640,11 +640,6 @@ mod test {
         assert!(r.residual_forbids.is_empty());
     }
 }
-// by default, Coverlay does not track coverage for lines after a line
-// containing #[cfg(test)].
-// we use the following sentinel to "turn back on" coverage tracking for
-// remaining lines of this file, until the next #[cfg(test)]
-// GRCOV_BEGIN_COVERAGE
 
 /// Authorization response returned from the `Authorizer`
 #[derive(Debug, PartialEq, Eq, Clone)]
diff --git a/cedar-policy-core/src/entities/err.rs b/cedar-policy-core/src/entities/err.rs
index 7e012698..95eed380 100644
--- a/cedar-policy-core/src/entities/err.rs
+++ b/cedar-policy-core/src/entities/err.rs
@@ -68,8 +68,8 @@ pub struct TransitiveClosureError {
     err: Box<transitive_closure::TcError<EntityUID>>,
 }
 
+#[cfg(test)]
 impl TransitiveClosureError {
-    #[cfg(test)]
     pub(crate) fn inner(&self) -> &transitive_closure::TcError<EntityUID> {
         self.err.as_ref()
     }
@@ -89,8 +89,8 @@ pub struct Duplicate {
     euid: EntityUID,
 }
 
+#[cfg(test)]
 impl Duplicate {
-    #[cfg(test)]
     pub(crate) fn euid(&self) -> &EntityUID {
         &self.euid
     }
diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs
index 691d2d31..6f3faaa5 100644
--- a/cedar-policy-core/src/evaluator.rs
+++ b/cedar-policy-core/src/evaluator.rs
@@ -932,33 +932,6 @@ impl<'e> Evaluator<'e> {
         }
     }
 
-    /// Interpret an `Expr` in an empty `SlotEnv`. Also checks that the source
-    /// location is propagated to the result.
-    #[cfg(test)]
-    pub fn interpret_inline_policy(&self, e: &Expr) -> Result<Value> {
-        use std::collections::HashMap;
-        match self.partial_interpret(e, &HashMap::new())? {
-            PartialValue::Value(v) => {
-                debug_assert!(e.source_loc().is_some() == v.source_loc().is_some());
-                Ok(v)
-            }
-            PartialValue::Residual(r) => {
-                debug_assert!(e.source_loc().is_some() == r.source_loc().is_some());
-                Err(err::EvaluationError::non_value(r))
-            }
-        }
-    }
-
-    /// Evaluate an expression, potentially leaving a residual
-    #[cfg(test)]
-    pub fn partial_eval_expr(&self, p: &Expr) -> Result<Either<Value, Expr>> {
-        let env = SlotEnv::new();
-        match self.partial_interpret(p, &env)? {
-            PartialValue::Value(v) => Ok(Either::Left(v)),
-            PartialValue::Residual(r) => Ok(Either::Right(r)),
-        }
-    }
-
     /// Evaluate a binary operation between a residual expression (left) and a value (right). If despite the unknown contained in the residual, concrete result
     /// can be obtained (using the type annotation on the residual), it is returned.
     fn short_circuit_residual_and_value(
@@ -1035,12 +1008,34 @@ impl<'e> Evaluator<'e> {
             _ => None,
         }
     }
+}
 
-    // by default, Coverlay does not track coverage for lines after a line
-    // containing #[cfg(test)].
-    // we use the following sentinel to "turn back on" coverage tracking for
-    // remaining lines of this file, until the next #[cfg(test)]
-    // GRCOV_BEGIN_COVERAGE
+#[cfg(test)]
+impl Evaluator<'_> {
+    /// Interpret an `Expr` in an empty `SlotEnv`. Also checks that the source
+    /// location is propagated to the result.
+    pub fn interpret_inline_policy(&self, e: &Expr) -> Result<Value> {
+        use std::collections::HashMap;
+        match self.partial_interpret(e, &HashMap::new())? {
+            PartialValue::Value(v) => {
+                debug_assert!(e.source_loc().is_some() == v.source_loc().is_some());
+                Ok(v)
+            }
+            PartialValue::Residual(r) => {
+                debug_assert!(e.source_loc().is_some() == r.source_loc().is_some());
+                Err(err::EvaluationError::non_value(r))
+            }
+        }
+    }
+
+    /// Evaluate an expression, potentially leaving a residual
+    pub fn partial_eval_expr(&self, p: &Expr) -> Result<Either<Value, Expr>> {
+        let env = SlotEnv::new();
+        match self.partial_interpret(p, &env)? {
+            PartialValue::Value(v) => Ok(Either::Left(v)),
+            PartialValue::Residual(r) => Ok(Either::Right(r)),
+        }
+    }
 }
 
 impl std::fmt::Debug for Evaluator<'_> {
@@ -1186,65 +1181,57 @@ pub(crate) mod test {
         let entity_no_attrs_no_parents =
             Entity::with_uid(EntityUID::with_eid("entity_no_attrs_no_parents"));
 
-        let mut entity_with_attrs = Entity::with_uid(EntityUID::with_eid("entity_with_attrs"));
-        entity_with_attrs
-            .set_attr(
-                "spoon".into(),
-                RestrictedExpr::val(787).as_borrowed(),
-                Extensions::none(),
-            )
-            .unwrap();
-        entity_with_attrs
-            .set_attr(
-                "fork".into(),
-                RestrictedExpr::val("spoon").as_borrowed(),
-                Extensions::none(),
-            )
-            .unwrap();
-        entity_with_attrs
-            .set_attr(
+        let attrs = HashMap::from([
+            ("spoon".into(), RestrictedExpr::val(787)),
+            ("fork".into(), RestrictedExpr::val("spoon")),
+            (
                 "tags".into(),
                 RestrictedExpr::set(vec![
                     RestrictedExpr::val("fun"),
                     RestrictedExpr::val("good"),
                     RestrictedExpr::val("useful"),
-                ])
-                .as_borrowed(),
-                Extensions::none(),
-            )
-            .unwrap();
-        entity_with_attrs
-            .set_attr(
+                ]),
+            ),
+            (
                 "address".into(),
                 RestrictedExpr::record(vec![
                     ("street".into(), RestrictedExpr::val("234 magnolia")),
                     ("town".into(), RestrictedExpr::val("barmstadt")),
                     ("country".into(), RestrictedExpr::val("amazonia")),
                 ])
-                .unwrap()
-                .as_borrowed(),
-                Extensions::none(),
-            )
-            .unwrap();
+                .unwrap(),
+            ),
+        ]);
+        let entity_with_attrs = Entity::new(
+            EntityUID::with_eid("entity_with_attrs"),
+            attrs.clone(),
+            HashSet::new(),
+            HashSet::new(),
+            HashMap::new(),
+            Extensions::none(),
+        )
+        .unwrap();
 
-        let mut entity_with_tags = Entity::with_uid(EntityUID::with_eid("entity_with_tags"));
-        entity_with_tags
-            .set_tag(
-                "spoon".into(),
-                RestrictedExpr::val(-121).as_borrowed(),
-                Extensions::none(),
-            )
-            .unwrap();
-
-        let mut entity_with_tags_and_attrs = entity_with_attrs.clone();
-        entity_with_tags_and_attrs.set_uid(EntityUID::with_eid("entity_with_tags_and_attrs"));
-        entity_with_tags_and_attrs
-            .set_tag(
-                "spoon".into(),
-                RestrictedExpr::val(-121).as_borrowed(),
-                Extensions::none(),
-            )
-            .unwrap();
+        let tags = HashMap::from([("spoon".into(), RestrictedExpr::val(-121))]);
+        let entity_with_tags = Entity::new(
+            EntityUID::with_eid("entity_with_tags"),
+            HashMap::new(),
+            HashSet::new(),
+            HashSet::new(),
+            tags.clone(),
+            Extensions::none(),
+        )
+        .unwrap();
+
+        let entity_with_tags_and_attrs = Entity::new(
+            EntityUID::with_eid("entity_with_tags_and_attrs"),
+            attrs,
+            HashSet::new(),
+            HashSet::new(),
+            tags,
+            Extensions::none(),
+        )
+        .unwrap();
 
         let mut child = Entity::with_uid(EntityUID::with_eid("child"));
         let mut parent = Entity::with_uid(EntityUID::with_eid("parent"));
diff --git a/cedar-policy-validator/src/cedar_schema/err.rs b/cedar-policy-validator/src/cedar_schema/err.rs
index fbd28325..2fe4d2f7 100644
--- a/cedar-policy-validator/src/cedar_schema/err.rs
+++ b/cedar-policy-validator/src/cedar_schema/err.rs
@@ -576,18 +576,11 @@ pub struct DuplicatePrincipalOrResource {
     loc2: Loc,
 }
 
-impl DuplicatePrincipalOrResource {
-    #[cfg(test)]
-    pub(crate) fn kind(&self) -> PR {
-        self.kind
-    }
-}
-
 impl Diagnostic for DuplicatePrincipalOrResource {
     impl_diagnostic_from_two_source_loc_fields!(loc1, loc2);
 
     fn help<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
-        let msg = format!("Actions may only have a single {} declaration. If you need it to apply to multiple types, try creating a parent type and using the `in` keyword", self.kind);
+        let msg = format!("Actions may only have a single {kind} declaration, but a {kind} declaration may specify a list of entity types like `{kind}: [X, Y, Z]`", kind=self.kind);
         Some(Box::new(msg))
     }
 }
diff --git a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs
index 74672faf..1b6e7c02 100644
--- a/cedar-policy-validator/src/cedar_schema/to_json_schema.rs
+++ b/cedar-policy-validator/src/cedar_schema/to_json_schema.rs
@@ -721,52 +721,52 @@ mod preserves_source_locations {
             .get(&Some(Name::parse_unqualified_name("NS").unwrap()))
             .expect("couldn't find namespace NS");
 
-        let entityA = ns
+        let entity_a = ns
             .entity_types
             .get(&"A".parse().unwrap())
             .expect("couldn't find entity A");
-        let entityB = ns
+        let entity_b = ns
             .entity_types
             .get(&"B".parse().unwrap())
             .expect("couldn't find entity B");
-        let entityC = ns
+        let entity_c = ns
             .entity_types
             .get(&"C".parse().unwrap())
             .expect("couldn't find entity C");
-        let ctypeS = ns
+        let ctype_s = ns
             .common_types
             .get(&json_schema::CommonTypeId::new("S".parse().unwrap()).unwrap())
             .expect("couldn't find common type S");
-        let ctypeAA = ns
+        let ctype_aa = ns
             .common_types
             .get(&json_schema::CommonTypeId::new("AA".parse().unwrap()).unwrap())
             .expect("couldn't find common type AA");
-        let actionRead = ns.actions.get("Read").expect("couldn't find action Read");
-        let actionWrite = ns.actions.get("Write").expect("couldn't find action Write");
-        let actionList = ns.actions.get("List").expect("couldn't find action List");
+        let action_read = ns.actions.get("Read").expect("couldn't find action Read");
+        let action_write = ns.actions.get("Write").expect("couldn't find action Write");
+        let action_list = ns.actions.get("List").expect("couldn't find action List");
 
-        assert_matches!(&entityA.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&entity_a.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("entity A;")
         ));
-        assert_matches!(&entityB.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&entity_b.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("entity B in A;")
         ));
-        assert_matches!(&entityC.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&entity_c.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("entity C in A {\n                bool: Bool,\n                s: S,\n                a: Set<A>,\n                b: { inner: B },\n            };")
         ));
-        assert_matches!(&ctypeS.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&ctype_s.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("type S = String;")
         ));
-        assert_matches!(&ctypeAA.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&ctype_aa.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("type AA = A;")
         ));
-        assert_matches!(&actionRead.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&action_read.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("action Read, Write;")
         ));
-        assert_matches!(&actionWrite.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&action_write.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("action Read, Write;")
         ));
-        assert_matches!(&actionList.loc, Some(loc) => assert_matches!(loc.snippet(),
+        assert_matches!(&action_list.loc, Some(loc) => assert_matches!(loc.snippet(),
             Some("action List in Read appliesTo {\n                principal: [A],\n                resource: [B, C],\n                context: {\n                    s: Set<S>,\n                    ab: { a: AA, b: B },\n                }\n            };")
         ));
     }
@@ -846,16 +846,16 @@ mod preserves_source_locations {
             });
         });});
 
-        let ctypeAA = ns
+        let ctype_aa = ns
             .common_types
             .get(&json_schema::CommonTypeId::new("AA".parse().unwrap()).unwrap())
             .expect("couldn't find common type AA");
-        assert_matches!(ctypeAA.ty.loc(), Some(loc) => {
+        assert_matches!(ctype_aa.ty.loc(), Some(loc) => {
             assert_matches!(loc.snippet(), Some("A"));
         });
 
-        let actionList = ns.actions.get("List").expect("couldn't find action List");
-        assert_matches!(&actionList.applies_to, Some(appliesto) => {
+        let action_list = ns.actions.get("List").expect("couldn't find action List");
+        assert_matches!(&action_list.applies_to, Some(appliesto) => {
             assert_matches!(appliesto.principal_types.first().expect("principal types were empty").loc(), Some(loc) => {
                 assert_matches!(loc.snippet(), Some("A"));
             });
diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs
index 63e592c3..3c1d5323 100644
--- a/cedar-policy-validator/src/json_schema.rs
+++ b/cedar-policy-validator/src/json_schema.rs
@@ -338,10 +338,10 @@ pub struct NamespaceDefinition<N> {
     pub annotations: Annotations,
 }
 
+#[cfg(test)]
 impl<N> NamespaceDefinition<N> {
     /// Create a new [`NamespaceDefinition`] with specified entity types and
     /// actions, and no common types or annotations
-    #[cfg(test)]
     pub fn new(
         entity_types: impl IntoIterator<Item = (UnreservedId, EntityType<N>)>,
         actions: impl IntoIterator<Item = (SmolStr, ActionType<N>)>,
diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs
index 6ca1bd8f..25adf254 100644
--- a/cedar-policy-validator/src/schema.rs
+++ b/cedar-policy-validator/src/schema.rs
@@ -1138,11 +1138,13 @@ impl AllDefs {
     fn entity_and_common_names(&self) -> impl Iterator<Item = &InternalName> {
         self.entity_defs.iter().chain(self.common_defs.iter())
     }
+}
 
+#[cfg(test)]
+impl AllDefs {
     /// Build an [`AllDefs`] that assumes the given fully-qualified
     /// [`InternalName`]s are defined (by the user) as entity types, and there
     /// are no defined common types or actions.
-    #[cfg(test)]
     pub(crate) fn from_entity_defs(names: impl IntoIterator<Item = InternalName>) -> Self {
         Self {
             entity_defs: names.into_iter().collect(),
diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs
index 11376cd4..9d84e4a0 100644
--- a/cedar-policy/src/api.rs
+++ b/cedar-policy/src/api.rs
@@ -3669,10 +3669,12 @@ impl Expression {
             vec![src_expr],
         ))
     }
+}
 
+#[cfg(test)]
+impl Expression {
     /// Deconstruct an [`Expression`] to get the internal type.
     /// This function is only intended to be used internally.
-    #[cfg(test)]
     pub(crate) fn into_inner(self) -> ast::Expr {
         self.0
     }
@@ -3774,10 +3776,12 @@ impl RestrictedExpression {
             name.as_ref(),
         )))
     }
+}
 
+#[cfg(test)]
+impl RestrictedExpression {
     /// Deconstruct an [`RestrictedExpression`] to get the internal type.
     /// This function is only intended to be used internally.
-    #[cfg(test)]
     pub(crate) fn into_inner(self) -> ast::RestrictedExpr {
         self.0
     }
diff --git a/cedar-policy/src/api/id.rs b/cedar-policy/src/api/id.rs
index 051fe6c1..949f4246 100644
--- a/cedar-policy/src/api/id.rs
+++ b/cedar-policy/src/api/id.rs
@@ -235,9 +235,11 @@ impl EntityUid {
             .into_euid(|| JsonDeserializationErrorContext::EntityUid)?
             .into())
     }
+}
 
+#[cfg(test)]
+impl EntityUid {
     /// Testing utility for creating `EntityUids` a bit easier
-    #[cfg(test)]
     pub(crate) fn from_strs(typename: &str, id: &str) -> Self {
         Self::from_type_name_and_id(
             EntityTypeName::from_str(typename).unwrap(),
diff --git a/cedar-policy/src/ffi/utils.rs b/cedar-policy/src/ffi/utils.rs
index 54f80220..fbbd530e 100644
--- a/cedar-policy/src/ffi/utils.rs
+++ b/cedar-policy/src/ffi/utils.rs
@@ -563,9 +563,11 @@ impl PolicySet {
         }
         Ok(policies)
     }
+}
 
+#[cfg(test)]
+impl PolicySet {
     /// Create an empty [`PolicySet`]
-    #[cfg(test)]
     pub(super) fn new() -> Self {
         Self {
             static_policies: StaticPolicySet::Set(Vec::new()),


Test patch:
diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs
index 3ca701d1..bbb181da 100644
--- a/cedar-policy-validator/src/cedar_schema/test.rs
+++ b/cedar-policy-validator/src/cedar_schema/test.rs
@@ -35,11 +35,7 @@ mod demo_tests {
     use smol_str::ToSmolStr;
 
     use crate::{
-        cedar_schema::{
-            self,
-            ast::PR,
-            err::{ToJsonSchemaError, NO_PR_HELP_MSG},
-        },
+        cedar_schema::{self, err::NO_PR_HELP_MSG},
         json_schema::{self, EntityType, EntityTypeKind},
         schema::test::utils::collect_warnings,
         CedarSchemaError, RawName,
@@ -277,18 +273,12 @@ mod demo_tests {
                 principal : [c]
             };
         "#;
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(crate::CedarSchemaError::Parsing(err)) => {
-            assert_matches!(err.errors(), cedar_schema::parser::CedarSchemaParseErrors::JsonError(json_errs) => {
-                assert!(json_errs
-                    .iter()
-                    .any(|err| {
-                        matches!(
-                            err,
-                            ToJsonSchemaError::DuplicatePrincipalOrResource(err) if err.kind() == PR::Principal
-                        )
-                    })
-                );
-            });
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
+            expect_err(src, &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(
+                r#"error parsing schema: duplicate `principal` declaration in action `Foo`"#
+            ).help(
+                "Actions may only have a single principal declaration, but a principal declaration may specify a list of entity types like `principal: [X, Y, Z]`"
+            ).exactly_two_underlines("principal: [a, b],", "principal : [c]").build());
         });
     }
 
@@ -304,17 +294,12 @@ mod demo_tests {
                 resource: [c]
             };
         "#;
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(crate::CedarSchemaError::Parsing(err)) => {
-            assert_matches!(err.errors(), cedar_schema::parser::CedarSchemaParseErrors::JsonError(json_errs) => {
-                assert!(json_errs
-                    .iter()
-                    .any(|err| {
-                        matches!(
-                            err,
-                            ToJsonSchemaError::DuplicatePrincipalOrResource(err) if err.kind() == PR::Resource
-                        )
-                    }));
-            });
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
+            expect_err(src, &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(
+                r#"error parsing schema: duplicate `resource` declaration in action `Foo`"#
+            ).help(
+                "Actions may only have a single resource declaration, but a resource declaration may specify a list of entity types like `resource: [X, Y, Z]`"
+            ).exactly_two_underlines("resource: [a, b],", "resource: [c]").build());
         });
     }
 
diff --git a/cedar-policy-validator/src/typecheck/test/expr.rs b/cedar-policy-validator/src/typecheck/test/expr.rs
index fa88f457..516c1cb1 100644
--- a/cedar-policy-validator/src/typecheck/test/expr.rs
+++ b/cedar-policy-validator/src/typecheck/test/expr.rs
@@ -16,7 +16,6 @@
 
 //! Contains tests for typechecking Cedar expressions outside of a larger
 //! policy and without a schema.
-// GRCOV_STOP_COVERAGE
 
 use std::{str::FromStr, vec};
 
diff --git a/cedar-policy-validator/src/typecheck/test/extensions.rs b/cedar-policy-validator/src/typecheck/test/extensions.rs
index e2eb6439..df544aaf 100644
--- a/cedar-policy-validator/src/typecheck/test/extensions.rs
+++ b/cedar-policy-validator/src/typecheck/test/extensions.rs
@@ -15,7 +15,6 @@
  */
 
 //! Contains tests for typechecking Cedar extensions
-// GRCOV_STOP_COVERAGE
 
 use crate::{diagnostics::ValidationError, types::Type};
 use cedar_policy_core::ast::Expr;
diff --git a/cedar-policy-validator/src/typecheck/test/namespace.rs b/cedar-policy-validator/src/typecheck/test/namespace.rs
index 9babfe70..b0e8c7c3 100644
--- a/cedar-policy-validator/src/typecheck/test/namespace.rs
+++ b/cedar-policy-validator/src/typecheck/test/namespace.rs
@@ -16,7 +16,6 @@
 
 //! Contains test for typechecking complete Cedar policies with namespaced
 //! schema files.
-// GRCOV_STOP_COVERAGE
 
 use cool_asserts::assert_matches;
 use serde_json::json;
diff --git a/cedar-policy-validator/src/typecheck/test/optional_attributes.rs b/cedar-policy-validator/src/typecheck/test/optional_attributes.rs
index a096c16a..bdf070a3 100644
--- a/cedar-policy-validator/src/typecheck/test/optional_attributes.rs
+++ b/cedar-policy-validator/src/typecheck/test/optional_attributes.rs
@@ -16,7 +16,6 @@
 
 //! Contains tests for defining optional attributes and typechecking their
 //! access using the ability added by capabilities.
-// GRCOV_STOP_COVERAGE
 
 use cedar_policy_core::{
     ast::{PolicyID, StaticPolicy},
diff --git a/cedar-policy-validator/src/typecheck/test/partial.rs b/cedar-policy-validator/src/typecheck/test/partial.rs
index 84d7cc69..09a1c842 100644
--- a/cedar-policy-validator/src/typecheck/test/partial.rs
+++ b/cedar-policy-validator/src/typecheck/test/partial.rs
@@ -15,7 +15,6 @@
  */
 
 //! Contains test for typechecking with partial schema files.
-// GRCOV_STOP_COVERAGE
 
 use std::collections::HashSet;
 
diff --git a/cedar-policy-validator/src/typecheck/test/policy.rs b/cedar-policy-validator/src/typecheck/test/policy.rs
index 237bb647..06cb2c06 100644
--- a/cedar-policy-validator/src/typecheck/test/policy.rs
+++ b/cedar-policy-validator/src/typecheck/test/policy.rs
@@ -16,7 +16,6 @@
 
 //! Contains test for typechecking complete Cedar policies with schema
 //! files.
-// GRCOV_STOP_COVERAGE
 
 use std::sync::Arc;
 
diff --git a/cedar-policy-validator/src/typecheck/test/strict.rs b/cedar-policy-validator/src/typecheck/test/strict.rs
index 2979ba41..dd2a751c 100644
--- a/cedar-policy-validator/src/typecheck/test/strict.rs
+++ b/cedar-policy-validator/src/typecheck/test/strict.rs
@@ -15,7 +15,6 @@
  */
 
 //! Contains test for strict typechecking.
-// GRCOV_STOP_COVERAGE
 
 use cool_asserts::assert_matches;
 use serde_json::json;
diff --git a/cedar-policy-validator/src/typecheck/test/tags.rs b/cedar-policy-validator/src/typecheck/test/tags.rs
index 8eace49a..f88070b1 100644
--- a/cedar-policy-validator/src/typecheck/test/tags.rs
+++ b/cedar-policy-validator/src/typecheck/test/tags.rs
@@ -16,7 +16,6 @@
 
 //! Contains tests for defining entity tags and typechecking their
 //! access using the ability added by capabilities.
-// GRCOV_STOP_COVERAGE
 
 use super::test_utils::{
     assert_exactly_one_diagnostic, assert_policy_typecheck_fails,
diff --git a/cedar-policy-validator/src/typecheck/test/test_utils.rs b/cedar-policy-validator/src/typecheck/test/test_utils.rs
index d000c936..44958130 100644
--- a/cedar-policy-validator/src/typecheck/test/test_utils.rs
+++ b/cedar-policy-validator/src/typecheck/test/test_utils.rs
@@ -16,7 +16,6 @@
 
 //! Contains utility functions for testing that expression typecheck or fail to
 //! typecheck correctly.
-// GRCOV_STOP_COVERAGE
 
 use cool_asserts::assert_matches;
 use itertools::Itertools;
diff --git a/cedar-policy-validator/src/typecheck/test/unspecified_entity.rs b/cedar-policy-validator/src/typecheck/test/unspecified_entity.rs
index 71c66894..ebd3f2af 100644
--- a/cedar-policy-validator/src/typecheck/test/unspecified_entity.rs
+++ b/cedar-policy-validator/src/typecheck/test/unspecified_entity.rs
@@ -14,8 +14,6 @@
  * limitations under the License.
  */
 
-// GRCOV_STOP_COVERAGE
-
 use crate::{json_schema, RawName};
 use cool_asserts::assert_matches;