test: Add RVM compiler testing to ACI tests - #509
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances the ACI test suite to validate both the interpreter and the new RVM (Rego Virtual Machine) compiler implementation, ensuring they produce consistent results. The PR also includes two important compiler fixes: one for destructuring variable scoping and another for the RVM's handling of the not operator with undefined operands.
Key Changes
- Expanded ACI test harness to run test cases through both interpreter and RVM, comparing their outputs for consistency
- Fixed RVM's
notoperator to treat undefined operands as successful negations (returningtrue) instead of propagatingundefined, matching interpreter semantics - Fixed compiler's destructuring logic to only check for duplicate variable bindings within the innermost scope, preventing false positives from parent scopes
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aci/main.rs | Added RVM test execution alongside interpreter tests with timing instrumentation, result alignment logic, filter support, and round-trip serialization validation |
| tests/aci/aci.yaml | Updated test queries from data.policy.rule=x format to data.policy.rule format to work with eval_rule API |
| src/rvm/vm/dispatch.rs | Fixed not operator to return true for undefined operands, matching Rego semantics where negation succeeds when expression has no results |
| src/languages/rego/compiler/destructuring.rs | Changed variable binding check to use is_var_bound_in_current_scope instead of lookup_local_var |
| src/languages/rego/compiler/core.rs | Added is_var_bound_in_current_scope method to check variable binding only in the innermost scope |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| let mut engine = setup_engine(dir, case)?; | ||
| let setup_duration = setup_start.elapsed(); | ||
|
|
||
| // Use eval_rule instead of eval_query since we're evaluating specific rules |
There was a problem hiding this comment.
The comment says "Use eval_rule instead of eval_query since we're evaluating specific rules" but this doesn't fully explain why the change was made. The more important context is that the test cases now use data.policy.rule queries (without =x bindings) that return rule results directly, which is what eval_rule is designed for, versus the previous eval_query approach that returned query results with bindings.
| // Use eval_rule instead of eval_query since we're evaluating specific rules | |
| // The test cases now use `data.policy.rule` queries (without `=x` bindings) that return rule results directly. | |
| // This is what eval_rule is designed for, versus the previous eval_query approach that returned query results with bindings. |
| let input = case.input.clone(); | ||
| let data = case.data.clone(); |
There was a problem hiding this comment.
[nitpick] These clones on lines 114-115 are unnecessary. The input and data variables are cloned from case but they're already available via engine which was set up with them in setup_engine. You can directly use case.input.clone() and case.data.clone() on lines 140-141 where they're actually needed, or remove these intermediate variables entirely.
code fixes:
- compiler: add `is_var_bound_in_current_scope` and use it in destructuring so
only the innermost scope blocks rebinding while still catching duplicates
within that block.
- rvm: treat `not` over undefined operands as a successful negation to match
interpreter semantics.
tests/aci:
migrate YAML cases to `data.policy.rule` queries with `{x: …}`
bindings, expand the harness to run interpreter plus RVM (with optional
skipping), align results to the binding format, add readable timing output,
and support a `--filter` flag for targeting cases.
Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
0939a44 to
89c4d15
Compare
code fixes:
is_var_bound_in_current_scopeand use it in destructuring so only the innermost scope blocks rebinding while still catching duplicates within that block.notover undefined operands as a successful negation to match interpreter semantics.tests/aci:
migrate YAML cases to
data.policy.rulequeries with{x: …}bindings, expand the harness to run interpreter plus RVM (with optional
skipping), align results to the binding format, add readable timing output,
and support a
--filterflag for targeting cases.