Timeout flag on individual test cases in opa test command #1851
Conversation
… to scale rego test when using the --timeout flag. Fixes open-policy-agent#1788 Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
|
Thanks for the PR! The changes look pretty good. The only thing I would request is that we adjust the behavior so that overall test running doesn't stop if a test case times out. As-is over here https://github.com/open-policy-agent/opa/blob/master/tester/runner.go#L333-L337 we will set ...
if err != nil {
tr.Error = err
if topdown.IsCancel(err) {
// Stop running tests if the context was canceled for any reason except a timeout
stop = !strings.Contains(ctx.Err().Error(), "context deadline exceeded")
}
...Then update the test case to ensure that |
tester/runner.go
Outdated
| runCtx, cancel := context.WithTimeout(ctx, r.timeout) | ||
| defer cancel() | ||
| tr, stop := r.runTest(runCtx, txn, module, rule) |
There was a problem hiding this comment.
We should call cancel as soon as the current test finishes to release the resources associated with the timeout. As is this will call cancel() after ALL tests have executed. I'd probably wrap this in an anonymous function.
There was a problem hiding this comment.
Good catch, I have wrapped the call into an anonymous function.
stop tests only on explicit cancel. Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
Set default timeout as default value for time.Duration is 0 (causing random test failures). Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
Wrap test run into anonymous function as to not leak timeout related resources. Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
Running 1.13 locally, errors.Is is only added in 1.13 Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
|
@patrick-east Good suggestion. It makes a lot more sense to have the tests individually fail if they take longer than expected. Also, I made a slight change: time.Duration defaults to 0s, so I included a default of 5 seconds in the runner. The other approach is to only return a context with timeout if the |
Implement the test timeout at individual test level to make it easier to scale rego test when using the --timeout flag.
Fixes #1788