Add timeout configuration support #262
Conversation
op-acceptor/config.go
Outdated
| @@ -20,6 +20,7 @@ type Config struct { | |||
| RunInterval time.Duration // Interval between test runs | |||
| RunOnce bool // Indicates if the service should exit after one test run | |||
| AllowSkips bool // Allow tests to be skipped instead of failing when preconditions are not met | |||
| DefaultTimeout time.Duration // Default timeout for individual tests, can be overriden by test config | |||
There was a problem hiding this comment.
Tiniest of things - why not just Timeout?
There was a problem hiding this comment.
Just adding my opinion to this: if we can override it per-test/suite/gate then I like having 'Default' in the name. Because if you ran it with op-acceptor --timeout 5m and then a test timed out after 1m (because it has an override), that'd be confusing. Whereas if it were labelled --default-timeout then that would explain things.
There was a problem hiding this comment.
@scharissis explained well
| @@ -53,6 +54,12 @@ var ( | |||
| EnvVars: opservice.PrefixEnvVar(EnvVarPrefix, "ALLOW_SKIPS"), | |||
| Usage: "Allow tests to be skipped instead of failing when preconditions are not met.", | |||
| } | |||
| DefaultTimeout = &cli.DurationFlag{ | |||
There was a problem hiding this comment.
The same from above - the sorta standard across all frameworks is --timeout, i'd just name it that
There was a problem hiding this comment.
@scharissis covered well my thoughts #262 (comment)
There was a problem hiding this comment.
Awesome, thank you!
A few notes/questions:
- How would one override the timeout of an individual test?
- Can you please add some tests showing this all works?
- CI is failing due to lint errors. (To check them locally run
golangci-lint run -E goimports,sqlclosecheck,bodyclose,asciicheck,misspell,errorlint --timeout 5m -e "errors.As" -e "errors.Is" ./...)
teddyknox
force-pushed
the
op-acceptor-timeout
branch
from
03e5642 to
f369dd4
Compare
teddyknox
force-pushed
the
op-acceptor-timeout
branch
from
f369dd4 to
5f285a1
Compare
|
Thanks for your comments @scharissis, re-requesting! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 53.60% 53.76% +0.16%
==========================================
Files 67 67
Lines 7886 7923 +37
==========================================
+ Hits 4227 4260 +33
- Misses 3378 3381 +3
- Partials 281 282 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Description
Adds the ability to configure the timeout of an individual test or test package, and also adds the ability to update the default test timeout for an op-acceptor startup invocation via CLI.
Tests
I've tested the command building.
Additional context
Add any other context about the problem you're solving.
Metadata