Extract SAFE_AWAIT_TIMEOUT constant
#108554
Conversation
Mainly so we have a place to document why we have a 10s wait in these methods.
DaveCTurner
added
>test
|
Pinging @elastic/es-distributed (Team:Distributed) |
| * <p> | ||
| * A well-designed test should not need to wait for longer than this. If you think you need to do so, instead seek a better way to write | ||
| * the test such that it does not need to wait for so long. Tests that take 10s of seconds to complete are a big drag on CI times which | ||
| * slows everyone down. |
There was a problem hiding this comment.
I wonder if we could make it even more restrictive?
I would say a unit test should not last longer than a second even on possibly slow CI.
For integration tests 10s sounds more reasonable.
There was a problem hiding this comment.
Even integ tests generally should run faster than this IMO. I strengthened the wording a little in ca73c5b.
| * fairly relaxed definition of "immediately". | ||
| * <p> | ||
| * A well-designed test should not need to wait for longer than this. If you think you need to do so, instead seek a better way to write | ||
| * the test such that it does not need to wait for so long. Tests that take 10s of seconds to complete are a big drag on CI times which |
There was a problem hiding this comment.
the test such that it does not need to wait for so long
May be we should give a hint(s) on those?
For example when waiting for an es timeout to happen we should generally ensure the test configures a custom fairly short timeout rather than relying on generous elasticsearch default one.
There was a problem hiding this comment.
IMO tests which wait for a timeout should be using a fake clock so that we can set a realistic timeout duration without having to wait for that long in real time. I know we have quite a lot of tests that set, and wait for, a timeout in the region of ~100ms, but this is not really testing a realistic situation and might therefore miss some bugs, whilst still being an approach that does not scale well when we're talking about a test suite the size of ES's.
There was a problem hiding this comment.
... but I'm not going to write that here, it's not always reasonable to require this, and yet nor do I want to write here that setting a 100ms timeout is a recommended practice.
DaveCTurner
added
the
auto-merge
|
Extracted from (or at least inspired by) #108531 hence the co-authored-by attribution. |
Mainly so we have a place to document why we have a 10s wait in these
methods.
Co-authored-by: Mikhail Berezovskiy mikhail@elastic.co