Errors from builtins in tests are not reported as documented

selaux commented 3 months ago

Short description

I am currently trying to trigger an error in a specific rule in rego in order to ensure that this specific codepath is only evaluated when it is actually needed.

While implementing this I noticed that errors in tests dont get reported as documented. They are instead reported as regular failures, even for the basic example shown in the documentation.

OPA version:

Version: 0.65.0
Build Commit: 
Build Timestamp: 
Build Hostname: 
Go Version: go1.22.3
Platform: darwin/arm64
WebAssembly: unavailable

Steps To Reproduce

Create pass_fail_error_test.rego as specified in the documentation:

package example_test

import rego.v1

import data.example

# This test will pass.
test_ok if true

# This test will fail.
test_failure if 1 == 2

# This test will error.
test_error if 1 / 0

# This test will be skipped.
todo_test_missing_implementation if {
    example.allow with data.roles as ["not", "implemented"]
}

Running the test as documented (opa test pass_fail_error_test.rego) results in:

pass_fail_error_test.rego:
data.example_test.test_failure: FAIL (116.834µs)
data.example_test.test_error: FAIL (114.958µs)
data.example_test.todo_test_missing_implementation: SKIPPED
--------------------------------------------------------------------------------
PASS: 1/4
FAIL: 2/4
SKIPPED: 1/4

Expected behavior

The documentation mentions that this should be the result (although that is probably wrong as well, as its missing the skipped test):

data.example_test.test_failure: FAIL (253ns)
data.example_test.test_error: ERROR (289ns)
  pass_fail_error_test.rego:15: eval_builtin_error: div: divide by zero
--------------------------------------------------------------------------------
PASS: 1/3
FAIL: 1/3
ERROR: 1/3

Additional context

Not sure if the documentation, or the implementation is the correct behavior here. For my use-case the documented behavior would be preferrable though, as I want to test that a rule is not entered in some specific cases (to avoid a http request).

anderseknert commented 3 months ago

Hi there, and thanks for filing this!

Wow, I'm surprised no one reported that before, because those docs are absolutely ancient. The behavior of error handling from built-in functions changed 2020(!) with this PR: https://github.com/open-policy-agent/opa/pull/2717

Commands like opa eval can be run with --show-builtin-errors to have errors from built-in functions reported as errors, but while the test runner supports this in the Go API (and Conftest makes use of that), there doesn't seem to be an equivalent command line flag to have it enabled. I don't see any reason why there couldn't be.

Either way, the docs definitely need an update.

selaux commented 3 months ago

Yes the missing --strict-builtin-errors for tests was something I wondered about as well when I noticed this behavior.

anderseknert commented 3 months ago

Any objections to adding that, @johanfylling, @srenatus ?

johanfylling commented 3 months ago

This is a surprising omission. I have no objections to adding a --strict-builtin-errors to opa test, @anderseknert.

anderseknert commented 3 months ago

From what I can tell, the reporter wasn't touched with the change in 2020, so I think adding that flag would be enough to get the same type of output that are shown in the docs. @selaux would you want to submit a fix? Should be a good first PR :)

selaux commented 3 months ago

Yes, I will attempt a fix over the weekend.

anderseknert commented 3 months ago

Awesome!

anderseknert commented 3 months ago

Just checking in @selaux — need any help to proceed here? I'd be happy to provide it if so!

selaux commented 3 months ago

Hi @anderseknert,

Thanks for checking in. I am a bit stuck in what is the best approach to implement this feature. While I would like to implement the --strict-builtin-errors (so it is consistent with opa eval), the current functionality that is supported by the test runner is slightly different as it will raise all builtin errors collected during a test run and not only the first one. So now there are three options:

Implementing the flag as --raise-builtin-errors and reuse the behavior already implemented in the runner. This would introduce some inconsistency beween the eval and test commands
Implementing the flag as --strict-builtin-errors and change the behavior in the runner to only raise the first builtin error. This would change the public golang API though, asRaiseBuiltinErrors would be renamed to StrictBuiltinErrors for the runner. This is what is currently implemented in the commit above.
Keeping RaiseBuiltinErrors as well as introduce StrictBuiltinErrors for the runner. As far as I can see, that would require a check that both are not used at the same time, similar to what is done in the eval command. It would probably also be a bit confusing from a usage perspective (why there are two functions that almost do the same).

I also still need to update the documentation.

charlieegan3 commented 2 months ago

Hey @selaux, sorry for the delay in getting back to you here, this one slipped though the cracks 😞

Keeping RaiseBuiltinErrors as well as introduce StrictBuiltinErrors for the runner. As far as I can see, that would require a check that both are not used at the same time, similar to what is done in the eval command. It would probably also be a bit confusing from a usage perspective (why there are two functions that almost do the same).

I think this is the best course of action, because:

A) We keep RaiseBuiltinErrors for anyone that depends on it currently in Go projects B) StrictBuiltinErrors then maps to the flag used by the user and has the same behaviour as the opa eval command.

StrictBuiltinErrors feels like the most useful behaviour for test runner purposes ✅

Please let me know if I can help at all get this ball rolling again this week.

open-policy-agent / opa