Isolate tests by running AtExit callbacks between them.

Isolate tests by running AtExit callbacks between them.

For now, this is only for base_unittests. The plan is to enable
it for all unit tests. This should finally fix mysterious
problems cause by Singletons surviving after one test etc.

This change also adapts LazyInstance so that it can be reused
after being destroyed. It is used very frequently, for example
each time a MessageLoop is used. It is also worth noting that
we had some problems in the past related to the MessageLoop
being destroyed and re-instantiated in the same test executable.
This patch should also fix that.

TEST=none
BUG=12710
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32507

[sky, 2009-11-09 17:45:31]

I'm not too familiar with these classes. I'm going to let Brett and Huan handle
this review.

[brettw, 2009-11-09 21:03:33]

I've never touched this code either. The changelog says cpu and mentovai are the
main authors. I'm adding them.

[cpu_(ooo_6.6-7.5), 2009-11-09 22:12:47]

I can review later today.

I think this is a good idea, but I've yet to see the code.

Afaik lazy_instance is the brainchild of Marc-Antoine, it might happen that I
deffer to him or you can add him to the review now.

[Paweł Hajdan Jr., 2009-11-10 22:25:47]

+M-A

[Paweł Hajdan Jr., 2009-11-10 22:26:06]

+M-A

http://codereview.chromium.org/372057

[cpu_(ooo_6.6-7.5), 2009-11-10 23:08:30]

Some questions below. Also I think you should do a test of this shadowing thing,
unless there is one already.

http://codereview.chromium.org/372057/diff/1/3
File base/at_exit.h (right):

http://codereview.chromium.org/372057/diff/1/3#newcode42
Line 42: DCHECK(shadow || !current_manager_);
shadown is not used, so this should ber DCHECK(current_manager_) ?

http://codereview.chromium.org/372057/diff/1/4
File base/lazy_instance.cc (right):

http://codereview.chromium.org/372057/diff/1/4#newcode32
Line 32: // makes possible calling all AtExit callbacks between tests. Assumes
that
shouldn't this block be under an #ifdef?

http://codereview.chromium.org/372057/diff/1/4#newcode37
Line 37: // Make sure that the lazily instantiated object will get destroyed at
exit.
we call RegisterCallback twice?

[cpu_(ooo_6.6-7.5), 2009-11-10 23:08:47]

Some questions below. Also I think you should do a test of this shadowing  
thing,
unless there is one already.




http://codereview.chromium.org/372057/diff/1/3
File base/at_exit.h (right):

http://codereview.chromium.org/372057/diff/1/3#newcode42
Line 42: DCHECK(shadow || !current_manager_);
shadown is not used, so this should ber DCHECK(current_manager_) ?

http://codereview.chromium.org/372057/diff/1/4
File base/lazy_instance.cc (right):

http://codereview.chromium.org/372057/diff/1/4#newcode32
Line 32: // makes possible calling all AtExit callbacks between tests.
Assumes that
shouldn't this block be under an #ifdef?

http://codereview.chromium.org/372057/diff/1/4#newcode37
Line 37: // Make sure that the lazily instantiated object will get
destroyed at exit.
we call RegisterCallback twice?

http://codereview.chromium.org/372057

[M-A Ruel, 2009-11-10 23:43:14]

Actually, Mike wrote LazyInstance.

http://codereview.chromium.org/372057/diff/1/3
File base/at_exit.h (right):

http://codereview.chromium.org/372057/diff/1/3#newcode12
Line 12: #include "base/logging.h"
much sadness.

http://codereview.chromium.org/372057/diff/1/3#newcode36
Line 36: #if defined(UNIT_TEST)
I'd rather leave it always available but private when defined(UNIT_TEST) is not
defined so you can leave the implementation in the source file. I bit hackish
but I think it's worth.

http://codereview.chromium.org/372057/diff/1/3#newcode42
Line 42: DCHECK(shadow || !current_manager_);
CHECK()?

http://codereview.chromium.org/372057/diff/1/3#newcode45
Line 45: #endif  // defined (UNIT_TEST)
defined(UNIT_TEST)

http://codereview.chromium.org/372057/diff/1/4
File base/lazy_instance.cc (right):

http://codereview.chromium.org/372057/diff/1/4#newcode32
Line 32: // makes possible calling all AtExit callbacks between tests. Assumes
that
No since it's in the .cc no you can't.

http://codereview.chromium.org/372057/diff/1/7
File base/test/test_suite.h (right):

http://codereview.chromium.org/372057/diff/1/7#newcode37
Line 37: DCHECK(!exit_manager_.get());
Can you use EXPECT_FALSE() at that level? That would be better.

[M-A Ruel, 2009-11-10 23:43:42]

Actually, Mike wrote LazyInstance.


http://codereview.chromium.org/372057/diff/1/3
File base/at_exit.h (right):

http://codereview.chromium.org/372057/diff/1/3#newcode12
Line 12: #include "base/logging.h"
much sadness.

http://codereview.chromium.org/372057/diff/1/3#newcode36
Line 36: #if defined(UNIT_TEST)
I'd rather leave it always available but private when defined(UNIT_TEST)
is not defined so you can leave the implementation in the source file. I
bit hackish but I think it's worth.

http://codereview.chromium.org/372057/diff/1/3#newcode42
Line 42: DCHECK(shadow || !current_manager_);
CHECK()?

http://codereview.chromium.org/372057/diff/1/3#newcode45
Line 45: #endif  // defined (UNIT_TEST)
defined(UNIT_TEST)

http://codereview.chromium.org/372057/diff/1/4
File base/lazy_instance.cc (right):

http://codereview.chromium.org/372057/diff/1/4#newcode32
Line 32: // makes possible calling all AtExit callbacks between tests.
Assumes that
No since it's in the .cc no you can't.

http://codereview.chromium.org/372057/diff/1/7
File base/test/test_suite.h (right):

http://codereview.chromium.org/372057/diff/1/7#newcode37
Line 37: DCHECK(!exit_manager_.get());
Can you use EXPECT_FALSE() at that level? That would be better.

http://codereview.chromium.org/372057

[Paweł Hajdan Jr., 2009-11-11 11:25:07]

Patch updated, that should address most comments. Some answers below.

On 2009/11/10 23:08:47, cpu wrote:
> Some questions below. Also I think you should do a test of this shadowing  
> thing, unless there is one already.

I think it's sufficiently tested in at_exit_unittest.cc. Also note that it was
there before. I can think about some more evil tests though.

> http://codereview.chromium.org/372057/diff/1/4#newcode32
> Line 32: // makes possible calling all AtExit callbacks between tests.
> Assumes that
> shouldn't this block be under an #ifdef?

It doesn't seem good to make such a low-level, subtle piece of code behave
differently in tests and real-world scenarios.

> http://codereview.chromium.org/372057/diff/1/4#newcode37
> Line 37: // Make sure that the lazily instantiated object will get
> destroyed at exit.
> we call RegisterCallback twice?

Yes, once to reset the state of the LazyInstance itself, and then to destroy the
object stored inside the LazyInstance. It could be combined into one
RegisterCallback call, but it'd just obfuscate the code I think. I don't see
anything bad in calling RegisterCallback twice, provided it's the simplest way
to do it. What do you think?

[Paweł Hajdan Jr., 2009-11-11 11:25:30]

Patch updated, that should address most comments. Some answers below.

On 2009/11/10 23:08:47, cpu wrote:
> Some questions below. Also I think you should do a test of this shadowing
> thing, unless there is one already.

I think it's sufficiently tested in at_exit_unittest.cc. Also note that it  
was
there before. I can think about some more evil tests though.

> http://codereview.chromium.org/372057/diff/1/4#newcode32
> Line 32: // makes possible calling all AtExit callbacks between tests.
> Assumes that
> shouldn't this block be under an #ifdef?

It doesn't seem good to make such a low-level, subtle piece of code behave
differently in tests and real-world scenarios.

> http://codereview.chromium.org/372057/diff/1/4#newcode37
> Line 37: // Make sure that the lazily instantiated object will get
> destroyed at exit.
> we call RegisterCallback twice?

Yes, once to reset the state of the LazyInstance itself, and then to  
destroy the
object stored inside the LazyInstance. It could be combined into one
RegisterCallback call, but it'd just obfuscate the code I think. I don't see
anything bad in calling RegisterCallback twice, provided it's the simplest  
way
to do it. What do you think?

http://codereview.chromium.org/372057

[Paweł Hajdan Jr., 2009-11-12 15:49:37]

ping!

[M-A Ruel, 2009-11-12 15:59:11]

http://codereview.chromium.org/372057/diff/5003/5007
File base/lazy_instance.h (right):

http://codereview.chromium.org/372057/diff/5003/5007#newcode79
Line 79: static void ResetState(void* helper);
Can you add a short description?

http://codereview.chromium.org/372057/diff/5003/5010
File base/test/run_all_unittests.cc (right):

http://codereview.chromium.org/372057/diff/5003/5010#newcode9
Line 9: test_suite.EnforceTestIsolation();
Shouldn't it always be enforced?

http://codereview.chromium.org/372057/diff/5003/5011
File base/test/test_suite.h (right):

http://codereview.chromium.org/372057/diff/5003/5011#newcode100
Line 100: void EnforceTestIsolation() {
I'd prefer to have it always enforced in Initialize().

[Paweł Hajdan Jr., 2009-11-12 17:05:26]

On 2009/11/12 15:59:11, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/372057/diff/5003/5007
> File base/lazy_instance.h (right):
> 
> http://codereview.chromium.org/372057/diff/5003/5007#newcode79
> Line 79: static void ResetState(void* helper);
> Can you add a short description?

Done.

> http://codereview.chromium.org/372057/diff/5003/5010
> File base/test/run_all_unittests.cc (right):
> 
> http://codereview.chromium.org/372057/diff/5003/5010#newcode9
> Line 9: test_suite.EnforceTestIsolation();
> Shouldn't it always be enforced?

Yes, that's the goal. However, I'd prefer to do it in small steps, to increase
chance of landing successfully. LazyInstance was quite difficult for me to
figure out (how to reset its state). There may be some more issues like that in
other test binaries (which for example might manifest only on the buildbot etc).
What do you think?

> http://codereview.chromium.org/372057/diff/5003/5011
> File base/test/test_suite.h (right):
> 
> http://codereview.chromium.org/372057/diff/5003/5011#newcode100
> Line 100: void EnforceTestIsolation() {
> I'd prefer to have it always enforced in Initialize().

Yes, I added a TODO.

[M-A Ruel, 2009-11-12 17:12:20]

lgtm but make sure it passes the try jobs.

[M-A Ruel, 2009-11-12 18:35:10]

On 2009/11/12 17:12:20, Marc-Antoine Ruel wrote:
> lgtm but make sure it passes the try jobs.

The base_unittests consistently fails on linux, please don't ignore.

[Paweł Hajdan Jr., 2009-11-12 18:37:53]

Will investigate that on Linux before landing, don't worry. :) One of the
reasons I really don't want to enable it for all tests at once.

On Thu, Nov 12, 2009 at 19:35, <maruel@chromium.org> wrote:

> On 2009/11/12 17:12:20, Marc-Antoine Ruel wrote:
>
>> lgtm but make sure it passes the try jobs.
>>
>
> The base_unittests consistently fails on linux, please don't ignore.
>
>
> http://codereview.chromium.org/372057
>

[cpu_(ooo_6.6-7.5), 2009-11-12 21:04:08]

lgtm

[Paweł Hajdan Jr., 2009-11-16 09:53:27]

This new version fixes a hang on Linux. The NSPR cleanup (when ran between
tests) waits for some threads, and never finishes.

wtc, could you comment on my comments about NSS and NSPR in test_suite.h? Or
maybe you'd have an idea how to do it better.

[M-A Ruel, 2009-11-16 17:25:13]

lgtm but the NSS stuff bothers me. :(