Defensive programming around test case race condition to address #776

[d-cameron]

Description

AsyncBufferedIteratorTest.testBackgroundBlocks() is subject to a race condition as it is a testing the async loading of records in AsyncBufferedIterator, but is not privy to the internal state of the iterator. Additional logic has been added such that the test case race condition is now extremely unlikely to be encountered.

[codecov-io]

Current coverage is 64.544% (diff: 100%)

Merging #790 into master will not change coverage

@@             master       #790   diff @@
==========================================
  Files           523        523          
  Lines         31614      31614          
  Methods           0          0          
  Messages          0          0          
  Branches       6769       6769          
==========================================
  Hits          20405      20405          
  Misses         9063       9063          
  Partials       2146       2146          

Powered by Codecov. Last update 19bd848...c4ba257

[d-cameron]

@lbergelson do you think the codecov failure here due to the non-deterministic nature of the test case as it is now, or is there something else I'm missing?

[lbergelson]

@d-cameron I'm honestly not quite sure what's going on with that. We've been seeing it a lot. I've been attributing it to the pull request not being rebased on the head of master, but yours is. It shows you gaining a few lines in AsyncBlockCompressedInputStream and losing Md5CalculatingOutputStream.java. Both seem plausibly caused by test non-determinism.

I'll just disable that check since we're basically treating it as a false positive and I'm not sure it would ever have added value over the total project coverage test.

[d-cameron]

Is there any way to retrigger the coverage checks? As it stand, it's causing the PR to incorrectly appear as if it's not suitable for merging yet.

[lbergelson]

@d-cameron Sorry, I keep meaning to reply to this and then getting distracted. Could we change this to use a CountDownLatch instead of the extended sleep and check? It would be cleaner and avoid any sleeping.

[yfarjoun]

I think that this PR is fine as is. 👍