GTTF: Make net_unittests run successfully with parallel test launcher

GTTF: Make net_unittests run successfully with parallel test launcher
by using a unique directory for disk cache tests.

BUG=54098
TEST=run net_unittests using tools/parallel_launcher/parallel_launcher.py

[Paweł Hajdan Jr., 2010-09-16 00:13:53]

With the parallel test launcher net_unittests run in under a minute on my Linux
machine.

[rvargas (doing something else), 2010-09-16 01:06:36]

I'm really not a fan of this change. I dislike the multitude of old test folders
that are left behind on the temp directory (I know that this is not supposed to
happen, but it happens a lot with other tests that used scoped folders).

There are other tools that depend on this code... and the fact that now one can
run a test and simply go to the cache_test folder and inspect the status of the
files after the last test is helpful during debugging.

Maybe if you could print out the folder name used for a test it could be better
(but that's a lot of noise for the common case).

http://codereview.chromium.org/3410008/diff/1/4
File net/disk_cache/disk_cache_test_base.cc (right):

http://codereview.chromium.org/3410008/diff/1/4#newcode76
net/disk_cache/disk_cache_test_base.cc:76: test_cache_.path(), force_creation_,
size_, type_,
nit: wrong indentation

http://codereview.chromium.org/3410008/diff/1/5
File net/disk_cache/disk_cache_test_base.h (right):

http://codereview.chromium.org/3410008/diff/1/5#newcode96
net/disk_cache/disk_cache_test_base.h:96: FilePath path() const { return
test_cache_.path(); }
nit: keep the style of the rest of the file (code on the next line)

http://codereview.chromium.org/3410008/diff/1/7
File net/disk_cache/disk_cache_test_util.h (left):

http://codereview.chromium.org/3410008/diff/1/7#oldcode27
net/disk_cache/disk_cache_test_util.h:27: FilePath GetCacheFilePath();
Did you build the whole solution? I believe this is used for more than unit
tests.

[Paweł Hajdan Jr., 2010-09-17 01:00:38]

Please note that in cases ScopedTempDir would leak the directory, the solution
that re-uses the same directory for all tests would also fail to delete the
cache directory, leading to intermittent problems.

I can add some debug log messages about cache directory, just let me know if you
have a preferred way to do that.

http://codereview.chromium.org/3410008/diff/1/4
File net/disk_cache/disk_cache_test_base.cc (right):

http://codereview.chromium.org/3410008/diff/1/4#newcode76
net/disk_cache/disk_cache_test_base.cc:76: test_cache_.path(), force_creation_,
size_, type_,
On 2010/09/16 01:06:36, rvargas wrote:
> nit: wrong indentation

Done.

http://codereview.chromium.org/3410008/diff/1/5
File net/disk_cache/disk_cache_test_base.h (right):

http://codereview.chromium.org/3410008/diff/1/5#newcode96
net/disk_cache/disk_cache_test_base.h:96: FilePath path() const { return
test_cache_.path(); }
On 2010/09/16 01:06:36, rvargas wrote:
> nit: keep the style of the rest of the file (code on the next line)

Do you mean to use GetCachePath instead of path?

http://codereview.chromium.org/3410008/diff/1/7
File net/disk_cache/disk_cache_test_util.h (left):

http://codereview.chromium.org/3410008/diff/1/7#oldcode27
net/disk_cache/disk_cache_test_util.h:27: FilePath GetCacheFilePath();
On 2010/09/16 01:06:36, rvargas wrote:
> Did you build the whole solution? I believe this is used for more than unit
> tests.

Done.

[rvargas (doing something else), 2010-09-17 21:19:40]

On 2010/09/17 01:00:38, Paweł Hajdan Jr. wrote:
> Please note that in cases ScopedTempDir would leak the directory, the solution
> that re-uses the same directory for all tests would also fail to delete the
> cache directory, leading to intermittent problems.

I don't think that's the case. The current code idea is that tests that need a
clean cache perform the cleanup before running the test (note that not all tests
do this), and we don't see failures doing it.

I know what would make me really happy: if there is a way to set the folder from
the outside. In other words, if test_cache_ is initialized with the well known
folder, and we only override that when running tests in parallel.

> 
> I can add some debug log messages about cache directory, just let me know if
you
> have a preferred way to do that.

I guess it's not just a matter of knowing the name, but not deleting the folder
at the end so that it can be inspected.

Another option is to not parallelize everything, but only Disk* against the rest
of the tests. Note that Disk* runs in under 40 secs on my system so that would
still give you less that one minute total. Specifying the name from the runner
is still a better idea. 

> 
> http://codereview.chromium.org/3410008/diff/1/4
> File net/disk_cache/disk_cache_test_base.cc (right):
> 
> http://codereview.chromium.org/3410008/diff/1/4#newcode76
> net/disk_cache/disk_cache_test_base.cc:76: test_cache_.path(),
force_creation_,
> size_, type_,
> On 2010/09/16 01:06:36, rvargas wrote:
> > nit: wrong indentation
> 
> Done.
> 
> http://codereview.chromium.org/3410008/diff/1/5
> File net/disk_cache/disk_cache_test_base.h (right):
> 
> http://codereview.chromium.org/3410008/diff/1/5#newcode96
> net/disk_cache/disk_cache_test_base.h:96: FilePath path() const { return
> test_cache_.path(); }
> On 2010/09/16 01:06:36, rvargas wrote:
> > nit: keep the style of the rest of the file (code on the next line)
> 
> Do you mean to use GetCachePath instead of path?
> 
> http://codereview.chromium.org/3410008/diff/1/7
> File net/disk_cache/disk_cache_test_util.h (left):
> 
> http://codereview.chromium.org/3410008/diff/1/7#oldcode27
> net/disk_cache/disk_cache_test_util.h:27: FilePath GetCacheFilePath();
> On 2010/09/16 01:06:36, rvargas wrote:
> > Did you build the whole solution? I believe this is used for more than unit
> > tests.
> 
> Done.

[rvargas (doing something else), 2010-09-17 21:20:07]

http://codereview.chromium.org/3410008/diff/1/4
File net/disk_cache/disk_cache_test_base.cc (right):

http://codereview.chromium.org/3410008/diff/1/4#newcode76
net/disk_cache/disk_cache_test_base.cc:76: test_cache_.path(), force_creation_,
size_, type_,
On 2010/09/17 01:00:38, Paweł Hajdan Jr. wrote:
> On 2010/09/16 01:06:36, rvargas wrote:
> > nit: wrong indentation
> 
> Done.

Note that I really prefer the old indentation (and elsewhere)... but yours is
not wrong.

http://codereview.chromium.org/3410008/diff/1/5
File net/disk_cache/disk_cache_test_base.h (right):

http://codereview.chromium.org/3410008/diff/1/5#newcode96
net/disk_cache/disk_cache_test_base.h:96: FilePath path() const { return
test_cache_.path(); }
On 2010/09/17 01:00:38, Paweł Hajdan Jr. wrote:
> On 2010/09/16 01:06:36, rvargas wrote:
> > nit: keep the style of the rest of the file (code on the next line)
> 
> Do you mean to use GetCachePath instead of path?

Nope. I meant to do 
const {
  return...
}

http://codereview.chromium.org/3410008/diff/6001/7009
File net/disk_cache/stress_cache.cc (right):

http://codereview.chromium.org/3410008/diff/6001/7009#newcode57
net/disk_cache/stress_cache.cc:57: path = path.AppendASCII("cache_test");
nit: merge this with the code at line 106

[Paweł Hajdan Jr., 2010-09-20 14:41:14]

Interesting. Disk* tests run in 12 seconds on my machine, and 5 seconds when
using parallel test launcher. Seems like it's worth to parallelize here.

I think you can preserve the directory contents by using
--gtest_break_on_failure.

Please take another look.

[rvargas (doing something else), 2010-09-20 18:56:36]

On 2010/09/20 14:41:14, Paweł Hajdan Jr. wrote:
> Interesting. Disk* tests run in 12 seconds on my machine, and 5 seconds when
> using parallel test launcher. Seems like it's worth to parallelize here.
> 
> I think you can preserve the directory contents by using
> --gtest_break_on_failure.

It's not necessarily about failures in the test: the test may run without
failures and still it may be worth going to the test folder. And with the code
as is, there's still the problem of finding out the name of the test folder. The
way to generate some of the data files used for tests is to actually run other
tests and grab the output folder... and yes, those tests could be modified to
break, and or print out the test folder, but my point is that the process
becomes more complex because you are removing code that makes that easy.

I'm not familiar with your tool to run tests in parallel (is it an external py
launcher?), but if I were writing this patch I would leave the original
GetCacheFilePath() function and write a wrapper in DiskCacheTestWithCache that
could be easily overrided to do what I want so that at least there is just only
one place where to look for things to change and recompile. As I mentioned, if
there is a way to know that the tests are running in parallel it would be better
because the code could select the right behavior by itself.

I'm sorry to insist so much about this. I could just let you commit the code and
then go and change it to what I think it should do, but that's not the spirit if
a code review.

By the way, my comment about not parallelize Disk* was not that there is no
gain, but that you should be able to get exactly the same total speedup without
running multiple Disk tests at the same time... because your total time is one
minute, and the serial time of all Disk* is 12 seconds (much better than the
number that I was considering). Seriously, same number just changing the script.

> 
> Please take another look.

[Paweł Hajdan Jr., 2010-09-21 15:39:16]

On Mon, Sep 20, 2010 at 20:56, <rvargas@chromium.org> wrote:

> On 2010/09/20 14:41:14, Paweł Hajdan Jr. wrote:
>
>> Interesting. Disk* tests run in 12 seconds on my machine, and 5 seconds
>> when
>> using parallel test launcher. Seems like it's worth to parallelize here.
>>
>
>  I think you can preserve the directory contents by using
>> --gtest_break_on_failure.
>>
>
> It's not necessarily about failures in the test: the test may run without
> failures and still it may be worth going to the test folder. And with the
> code
> as is, there's still the problem of finding out the name of the test
> folder.
>

I offered to print the folder name since the beginning.


> As I mentioned, if there is a way to know that the tests are running in
> parallel it would be better
> because the code could select the right behavior by itself.
>

I think that changing the behavior of the tests based on that is a recipe
for weirdness. The parallel launcher should be transparent provided the
tests are really isolated from each other. And they should really be, not
just because of the launcher.

[rvargas (doing something else), 2010-09-21 18:05:46]

OK

On 2010/09/21 15:39:16, Paweł Hajdan Jr. wrote:
> On Mon, Sep 20, 2010 at 20:56, <mailto:rvargas@chromium.org> wrote:
> 
> > On 2010/09/20 14:41:14, Paweł Hajdan Jr. wrote:
> >
> >> Interesting. Disk* tests run in 12 seconds on my machine, and 5 seconds
> >> when
> >> using parallel test launcher. Seems like it's worth to parallelize here.
> >>
> >
> >  I think you can preserve the directory contents by using
> >> --gtest_break_on_failure.
> >>
> >
> > It's not necessarily about failures in the test: the test may run without
> > failures and still it may be worth going to the test folder. And with the
> > code
> > as is, there's still the problem of finding out the name of the test
> > folder.
> >
> 
> I offered to print the folder name since the beginning.

I think that printing the name is more noise than useful for the common case. I
wouldn't want to see each test spitting out a log that I wouldn't need 99% of
the time.

> > As I mentioned, if there is a way to know that the tests are running in
> > parallel it would be better
> > because the code could select the right behavior by itself.
> >
> 
> I think that changing the behavior of the tests based on that is a recipe
> for weirdness. The parallel launcher should be transparent provided the
> tests are really isolated from each other. And they should really be, not
> just because of the launcher.
> 

I think that test independence is not the same as being able to run all test at
the same time.

I think that improving the time needed to run tests is a worthy endevour, but
you are missing the big picture by making a requirement that every single test
should be able to run at the same time with any other test... we have more that
1600 tests in that exe, so unless you can run hundreds of tests at the same time
in the machines that really count (out bots), that is not really a requirement.

I think that having a way for a test to know if it is running in parallel with
other tests or not is not undesirable... it is better if the code itself is
smart enough to adjust to the environment instead of needing a hand to fine tune
something, as long as the test still does its work of making sure that the
actual shipping code behaves properly.

And finally, I think we are wasting too much time in something so simple, so go
ahead and commit the change, and I'll fix it later.

[Paweł Hajdan Jr., 2010-09-22 11:48:37]

Oops, I got a weird test failure (Windows-only):

DiskCacheTest.DeleteOld: 
.\disk_cache\backend_unittest.cc(1279): error: Value of: cb.GetResult(rv)
  Actual: -2
Expected: net::OK
Which is: 0

Do you know what might cause it?

[rvargas (doing something else), 2010-09-22 17:33:18]

On 2010/09/22 11:48:37, Paweł Hajdan Jr. wrote:
> Oops, I got a weird test failure (Windows-only):
> 
> DiskCacheTest.DeleteOld: 
> .\disk_cache\backend_unittest.cc(1279): error: Value of: cb.GetResult(rv)
>   Actual: -2
> Expected: net::OK
> Which is: 0
> 
> Do you know what might cause it?

nope