Storage: Add TestUtil.await #487
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/await
branch
from
3ca14a5 to
9c734b1
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/await
branch
from
9c734b1 to
447449e
Compare
| return; | ||
| } | ||
| Thread.sleep(1); | ||
| TestUtil.await(task, 300, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Note: Some of the timeouts are VERY questionable, but I kept them as is to make this purely a refactor.
There was a problem hiding this comment.
Yeah, this looks a little insane. However, this test looks to be exercising network failures, so, as an integration test, it might not be unreasonable for this to take multiple minutes to fail. But it's a unit test; so hopefully it doesn't actually take anywhere near that long. (Maybe 5m was used when writing the test against the real network? Then it was converted to a unit test, but the timeout wasn't fixed?)
Regardless, keeping it the same to keep this PR as a refactor sounds reasonable. You could optionally add a TODO to investigate whether this value still makes sense. (If you do, write the todo to apply to all non default timeouts.)
There was a problem hiding this comment.
I added TODOs to all timeouts greater than 5 seconds with the exception of those that were already commented.
schmidt-sebastian
changed the title
| Thread.sleep(1); | ||
| } | ||
|
|
||
| org.junit.Assert.fail(); |
There was a problem hiding this comment.
how about org.junit.Assert.fail("Timeout occurred"); or similar?
| Thread.sleep(1); | ||
| } | ||
|
|
||
| org.junit.Assert.fail(); |
There was a problem hiding this comment.
Since you've imported org.junit.Assert, I think you could just write this as Assert.fail();
There was a problem hiding this comment.
... and done!
| return; | ||
| } | ||
| Thread.sleep(1); | ||
| TestUtil.await(task, 300, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Yeah, this looks a little insane. However, this test looks to be exercising network failures, so, as an integration test, it might not be unreasonable for this to take multiple minutes to fail. But it's a unit test; so hopefully it doesn't actually take anywhere near that long. (Maybe 5m was used when writing the test against the real network? Then it was converted to a unit test, but the timeout wasn't fixed?)
Regardless, keeping it the same to keep this PR as a refactor sounds reasonable. You could optionally add a TODO to investigate whether this value still makes sense. (If you do, write the todo to apply to all non default timeouts.)
| Assert.fail(); | ||
| } catch (RuntimeExecutionException e) { | ||
| Assert.assertEquals(taskException.get().getCause(), e.getCause().getCause()); | ||
| // Task failed. |
There was a problem hiding this comment.
nit: I'd probably get rid of these 'task failed' comments (here and below). Optional.
There was a problem hiding this comment.
But.... how will anyone understand this sophisticated logic? :) Removed.
| Thread.sleep(1); | ||
| } | ||
| Assert.fail(); | ||
| TestUtil.await(task, 20, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Yeah this ^. This non-default timeout is documented, so we know it's deliberate and meaningful. We should (eventually) do that for all of them. You could even enforce that by changing TestUtil.await(Task, int, TimeUnit) to TestUtil.await(Task, int, TimeUnit, String reasonWhyWereNotUsingTheDefault) (which is a little odd, but would be pretty effective.) Or, since I see this same reason is used again below, TestUtil.awaitFairnessBug(task)
(Aside: 20s timeout in a "unit" test is questionable.)
No action required.
There was a problem hiding this comment.
Acknowledged, but left the timeouts and the comment as is for now.
Addresses some leftover feedback from #458 and adds a shared TestUtil.await() that mimics Task.await() and removes a lot of duplicated code in our unit tests.