-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add basic EST round-trip testing #215
Conversation
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.
See also the tests and test utilities in est.rs
, which define three different kinds of roundtrips involving EST, one of which is always lossless.
// Panics if any step fails. The resulting policy may be different from the input | ||
// (since the roundtrip is lossy), so this function only checks that the round-trip | ||
// succeeds without error. |
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.
The lossy step is EST->AST or CST->AST, neither of which is included in this roundtrip I think? (I believe this takes AST input and goes AST->text->EST->JSON?) So I think we actually could check that the resulting policy matches the input
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.
EST->AST would be required here because check_policy_equivalence
expects two ASTs.
But it would be possible to define this function for ESTs too. I'll update this PR later to include more variants, inspired by the helper functions in est.rs
.
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.
Update: just looked at this again, and I think this step actually should be lossless. I assumed the translation was supposed to be lossy since DRT failed when I compare the outputs. But looking at the failure again, it looks like DRT was pointing out an actual error: cedar#618, which should be resolved by cedar#622. I'll add in the code to do the AST comparison, and test again once cedar#622 is finished.
Issue #, if available:
#204
Description of changes:
This PR renames the
pp
target toroundtrip
and adds two additional checks per policy:parser::parse_policy_or_template_to_est
and converting to JSON produces no errors.To test, I reverted cedar#601 and confirmed that the underlying bug was found quickly by check 1. Check 2 didn't turn up any bugs during an overnight run, but it seems useful(?)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.