Do not include thread start time in test timeout measurements. #962
Conversation
The time it takes to start a thread can be surprisingly large. Especially in virtualized cloud environments where noisy neighbours. This change reduces the probability of non-deterministic failures of tests with timeouts (`@Test(timeout=…)`) by not beginning the timeout clock until we have observed the starting of the task thread – the thread that runs the actual test. This should make tests with small timeout values more reliable in general, and especially in cloud CI environments. No tests added for this because of the probabilistic nature of the problem, and the small scope of the change.
|
LGTM! @stefanbirkner Can you please take a look? |
|
The code LGTM, but I think we should not merge this pull request. IMHO the purpose of the timeout is to terminate tests in case of an unexpected behaviour that would otherwise lead to a long running test. If you encounter trouble because a timeout terminates your test too early, then you should increase the timeout. This is no problem, because normally the timeout does not interrupt your test. I know that the timeout can be used for performance tests. But I think that you should use some other mechanism for such tests, because JUnit gives you no guarantee about the test execution overhead that is part of the duration which is limited by the timeout. |
|
@stefanbirkner while I can see your point, I don't see any actual disadvantages to merging this pull. Are there non-philosophical concerns? |
|
IMHO it adds complexity that is not needed. But I'm not dogmatic about it. |
|
I think the added complexity is quite low, and I don't see how it can cause problems. It's possible that this change might slow down some tests due to the extra time waiting for the "parent" thread to be notified about the "child" thread starting, but I don't think it's a problem. @dsaff do you have any thoughts? |
|
I like the intent here, LGTM. |
Do not include thread start time in test timeout measurements.
The time it takes to start a thread can be surprisingly large.
Especially in virtualized cloud environments where noisy neighbours.
This change reduces the probability of non-deterministic failures of
tests with timeouts (
@Test(timeout=…)) by not beginning the timeoutclock until we have observed the starting of the task thread – the
thread that runs the actual test. This should make tests with small
timeout values more reliable in general, and especially in cloud CI
environments.
No tests added for this because of the probabilistic nature of the
problem, and the small scope of the change.