-
Notifications
You must be signed in to change notification settings - Fork 107
Move tests out of api.rs #446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
LGTM. That being said, is there a convention where we want to put unit tests (e.g., a dedicated folder)? |
stupid question, did you do the simple check that the tests are still running? (like, just break one and check.) I'm a little surprised we didn't need |
I just followed e.g., https://github.com/rust-lang/rust/blob/master/compiler/rustc_graphviz/src/tests.rs here. I don't know if a one-size-fits-all strategy makes the most sense. |
We did in fact need a |
Fails correctly with broken unit test: https://github.com/cedar-policy/cedar/actions/runs/6897658266/job/18766238211?pr=446 |
This reverts commit 933a4ad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it.
Signed-off-by: John Kastner <[email protected]>
Description of changes
Move tests out of api.rs. IMO this is cleaner than having almost 3k lines of tests in the public API.
Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy-core
,cedar-validator
, etc.)I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec
(choose one, and delete the other options):Disclaimer
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.