SimpleCache: have tests run on all three streams

SimpleCache: have tests run on all three streams

Many unit tests only run on the first stream. With the upcoming changes to
SimpleCache, each stream will have a different implementation. This CL ensures
test are ran on all streams.

BUG=173398
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243170

[clamy, 2013-09-09 14:46:04]

pasko, gavinp: PTAL.

This is intended to go along with
https://chromiumcodereview.appspot.com/23983005/ (the stream merge).

[pasko, 2013-09-10 14:35:53]

Thanks, Camille, this looks useful.

Running CleanupCacheDir() in many tests is a good choice, since it would avoid
breaking tests that could assume some entries are not created.

Though a cache backend could remain some leftover entries _after_ we cleaned up
the cache directory. This could result in flakiness, which would be hard to
debug.

How about adding DoomAllEntries? Its flakiness is soon going away. We can add it
just before CleanupCacheDir() to be sure.

Here is my random thoughts on what we could do instead:
1. destroy the backend between invocations (with the current testing
infrastructure I believe this would require adding an extra TEST for each stream
index, which is an overkill
2. wait until all backend operations are finished (there was a hackish flaky way
to make almost that: helper.WaitUntilCacheIoFinished(1);)
3. run using a different backend instance with a newly created directory (this
would require replacing simple_cache_impl_ and cache_ with a new instance)
looking hackish as well
4. manually run TearDown() and SetUp() inside a test? I believe nobody is doing
that, consequences unknown.

More ideas are welcome.

[clamy, 2013-09-10 14:45:44]

On 2013/09/10 14:35:53, pasko wrote:
> Thanks, Camille, this looks useful.
> 
> Running CleanupCacheDir() in many tests is a good choice, since it would avoid
> breaking tests that could assume some entries are not created.
> 
> Though a cache backend could remain some leftover entries _after_ we cleaned
up
> the cache directory. This could result in flakiness, which would be hard to
> debug.
> 
> How about adding DoomAllEntries? Its flakiness is soon going away. We can add
it
> just before CleanupCacheDir() to be sure.
> 
> Here is my random thoughts on what we could do instead:
> 1. destroy the backend between invocations (with the current testing
> infrastructure I believe this would require adding an extra TEST for each
stream
> index, which is an overkill
> 2. wait until all backend operations are finished (there was a hackish flaky
way
> to make almost that: helper.WaitUntilCacheIoFinished(1);)
> 3. run using a different backend instance with a newly created directory (this
> would require replacing simple_cache_impl_ and cache_ with a new instance)
> looking hackish as well
> 4. manually run TearDown() and SetUp() inside a test? I believe nobody is
doing
> that, consequences unknown.
> 
> More ideas are welcome.

Adding a DoomAllEntries seems fine to me. I decided on using CleanupCacheDir
after considering 1, 3 and 4 (and raising about the same points as you do).
2) I don't know about that one. If it is flaky, maybe we should use
DoomAllEntries instead.

[clamy, 2013-09-19 13:39:06]

egor, gavin: PTAL

In this version, SimpleCacheNonSequentialWrite seem to have no race condition -
and this is the test that caught the crc32 bug fixed in
https://chromiumcodereview.appspot.com/24251005/

I am working on a version of SimpleCacheNonSequentialWrite that has a race
conditions because of DoomAllEntries, and will upload that in another CL.

[pasko, 2013-10-29 18:53:46]

On 2013/09/19 13:39:06, clamy wrote:
> egor, gavin: PTAL
> 
> In this version, SimpleCacheNonSequentialWrite seem to have no race condition
-
> and this is the test that caught the crc32 bug fixed in
> https://chromiumcodereview.appspot.com/24251005/
> 
> I am working on a version of SimpleCacheNonSequentialWrite that has a race
> conditions because of DoomAllEntries, and will upload that in another CL.

FYI: some tests fail because writes on stream 2 now produce errors on doomed
entries

[clamy, 2013-11-20 13:50:08]

I stopped testing on stream 2 in the DoomdEntry test. The other ones seem ok.

[pasko, 2013-11-21 13:02:21]

lgtm given two comments below are addressed.

https://codereview.chromium.org/23702024/diff/31001/net/disk_cache/entry_unit...
File net/disk_cache/entry_unittest.cc (left):

https://codereview.chromium.org/23702024/diff/31001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3469: EXPECT_EQ(entry, entry2);
why removing this check? rebase gone out of control?

https://codereview.chromium.org/23702024/diff/31001/net/disk_cache/entry_unit...
File net/disk_cache/entry_unittest.cc (right):

https://codereview.chromium.org/23702024/diff/31001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:2461: // APP_CACHE does not use optimistic
operations, and this test needs non
// Proving that the test works well with optimistic operations enabled is
subtle, instead run only in APP_CACHE mode to disable optimistic operations.

And stream 0 is not special.

[clamy, 2013-11-21 18:55:04]

Thanks for the review! I'll be waiting on Gavin's one when he's back from
vacation.

https://chromiumcodereview.appspot.com/23702024/diff/31001/net/disk_cache/ent...
File net/disk_cache/entry_unittest.cc (left):

https://chromiumcodereview.appspot.com/23702024/diff/31001/net/disk_cache/ent...
net/disk_cache/entry_unittest.cc:3469: EXPECT_EQ(entry, entry2);
On 2013/11/21 13:02:21, pasko wrote:
> why removing this check? rebase gone out of control?

This check does not make any sense since entry has just been closed 3 lines
before. So it is effectively a dangling pointer. Also, the entry itself could
have been taken out of the list of active entry, so when queried again we would
get a new pointer and the check would fail.

https://chromiumcodereview.appspot.com/23702024/diff/31001/net/disk_cache/ent...
File net/disk_cache/entry_unittest.cc (right):

https://chromiumcodereview.appspot.com/23702024/diff/31001/net/disk_cache/ent...
net/disk_cache/entry_unittest.cc:2461: // APP_CACHE does not use optimistic
operations, and this test needs non
On 2013/11/21 13:02:21, pasko wrote:
> // Proving that the test works well with optimistic operations enabled is
> subtle, instead run only in APP_CACHE mode to disable optimistic operations.
> 
> And stream 0 is not special.

Done.

[pasko, 2013-11-21 19:22:54]

still lgtm.

[pasko, 2013-12-23 10:09:42]

gavinp: ping

[gavinp, 2013-12-30 20:57:45]

lgtm. Thanks for the ping!

[commit-bot: I haz the power, 2014-01-06 14:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/23702024/131001

[commit-bot: I haz the power, 2014-01-06 15:07:42]

Retried try job too often on linux_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-06 17:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/23702024/401001

[commit-bot: I haz the power, 2014-01-06 21:33:51]

Change committed as 243170