For unit tests, track additions to AtExitManager and warn.

For unit tests, track additions to AtExitManager and warn.

While trying to bullet proof a unit test, I had trouble getting very far when
running all tests in shuffle mode. Tracked that back to a few other tests doing
stuff that accessed Singleton()s outside of a test-scoped
ShadowingAtExitManager. Seemed to me that should be an invariant around any
unit test, so created this towards that end, hopefully helping stabilize out
unit_tests a bit more.

BUG=133403
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144488

Reverted in https://chromiumcodereview.appspot.com/10834010/ - it was just causing noise, and really isn't the right answer. The right answer is to debug and finish up http://codereview.chromium.org/10735063/

[Scott Byer, 2012-06-18 23:45:57]

Looking for some thoughts here before I pursue this any further. While I could
clean up what I've seen, I think problems would creep back in without a warning
like this. The plan would be to clean everything up then make it an error.

http://codereview.chromium.org/10582012/diff/1/base/test/unit_test.h
File base/test/unit_test.h (right):

http://codereview.chromium.org/10582012/diff/1/base/test/unit_test.h#newcode17
base/test/unit_test.h:17: class UnitTest : public testing::Test {
I'm not wedded to the class name here, but this seemed better than just 'Test'.

http://codereview.chromium.org/10582012/diff/1/content/public/test/test_rende...
File content/public/test/test_renderer_host.h (right):

http://codereview.chromium.org/10582012/diff/1/content/public/test/test_rende...
content/public/test/test_renderer_host.h:121: class RenderViewHostTestHarness :
public base::UnitTest {
This is the test harness that surrounds both the new unit test I was working on
and one of the unit_tests I saw a problem with. I suspect there are other
subclasses of testing::Test that should also be lifted up and adhere to the
invariant.

[brettw, 2012-06-19 04:06:13]

I guess I don't get when I should derive from UnitTest and when I shouldn't. Are
there any ideas here?

[Paweł Hajdan Jr., 2012-06-19 18:42:09]

1. I've tried a very similar thing 1-2 years ago and hit problems with tests
crashing/misbehaving (Singleton-related failures).

2. Use gtest listeners instead of requiring people to derive from UnitTest
class.

3. Can we just ask AtExitManager how many callbacks it has? We can even check if
it's still the same AtExitManager (it should be).

4. This is very good thing to work on, and is important for test stability. I
appreciate this effort very much!

[Scott Byer, 2012-06-19 21:57:29]

On 2012/06/19 18:42:09, Paweł Hajdan Jr. wrote:
> 1. I've tried a very similar thing 1-2 years ago and hit problems with tests
> crashing/misbehaving (Singleton-related failures).

Let's see how far I get.

> 2. Use gtest listeners instead of requiring people to derive from UnitTest
> class.

Thank you! Much cleaner this way.

> 3. Can we just ask AtExitManager how many callbacks it has? We can even check
if
> it's still the same AtExitManager (it should be).

I wanted to avoid adding any code to AtExitManager since it would end up in
Chromium itself.


> 4. This is very good thing to work on, and is important for test stability. I
> appreciate this effort very much!

[Paweł Hajdan Jr., 2012-06-20 07:47:13]

Please don't use ShadowingAtExitManager, it's very likely to cause weirdness. I
think it's fine to modify AtExitManager.

By the way, I think we should also hide ShadowingAtExitManager, it's too
tempting to misuse.

http://codereview.chromium.org/10582012/diff/3005/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10582012/diff/3005/base/test/test_suite.cc#new...
base/test/test_suite.cc:214: LOG(INFO) << "Adding watcher for AtExitManager.";
This shouldn't be needed.

http://codereview.chromium.org/10582012/diff/3005/content/test/unittest_test_...
File content/test/unittest_test_suite.cc (right):

http://codereview.chromium.org/10582012/diff/3005/content/test/unittest_test_...
content/test/unittest_test_suite.cc:44: test_suite_->WatchAtExitManager();
This should always be run, for all test suites (make the suite in base/ do
that).

[Scott Byer, 2012-06-21 00:05:41]

Ok, modified AtExitManager, it is cleaner.

http://codereview.chromium.org/10582012/diff/3005/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10582012/diff/3005/base/test/test_suite.cc#new...
base/test/test_suite.cc:214: LOG(INFO) << "Adding watcher for AtExitManager.";
On 2012/06/20 07:47:13, Paweł Hajdan Jr. wrote:
> This shouldn't be needed.

Oops. Was doing local diagnostics and left it in. Done.

http://codereview.chromium.org/10582012/diff/3005/content/test/unittest_test_...
File content/test/unittest_test_suite.cc (right):

http://codereview.chromium.org/10582012/diff/3005/content/test/unittest_test_...
content/test/unittest_test_suite.cc:44: test_suite_->WatchAtExitManager();
On 2012/06/20 07:47:13, Paweł Hajdan Jr. wrote:
> This should always be run, for all test suites (make the suite in base/ do
> that).

I was trying to be conservative and not spew warnings all over the place - I had
it there to begin with.

I've fixed it so just browser_tests and interactive_ui_tests don't check since
those tests get run in a sub-process, adding exit manager callbacks / using
singletons seems like something they should be able to do.

content_unittests seems to have the warning fire on an unusually large number of
tests, I'm going to look into that. But it doesn't seem unmanageable.

[Paweł Hajdan Jr., 2012-06-21 08:45:24]

Minor comments.

http://codereview.chromium.org/10582012/diff/11006/base/at_exit.h
File base/at_exit.h (right):

http://codereview.chromium.org/10582012/diff/11006/base/at_exit.h#newcode65
base/at_exit.h:65: static void* CurrentAtExitManager();
nit: Let's not repeat the name, just static AtExitManager* current();

http://codereview.chromium.org/10582012/diff/11006/base/at_exit.h#newcode66
base/at_exit.h:66: static size_t CurrentAtExitCallbackStackSize();
nit: Let's remove the static and just name this CallbackStackSize() const;

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc#ne...
base/test/test_suite.cc:89: LOG(ERROR) << "AtExitManager: stack changed depth
across test " <<
This should also have a TODO above.

The error message should be more clear: that the AtExitManager is different,
"stack changed depth" is confusing (what stack?).

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc#ne...
base/test/test_suite.cc:98: ". Test should be surrounded by a
ShadowingAtExitManager.";
ShadowingAtExitManager is not always the right solution. Better to say something
about cleaning up global state before test exits.

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc#ne...
base/test/test_suite.cc:337: if
(!CommandLine::ForCurrentProcess()->HasSwitch("single_process") &&
Don't hardcode command-line flags this way, we always have to use switches:: for
that. If needed, move the relevant switch constants to base/

[Scott Byer, 2012-06-21 21:35:01]

http://codereview.chromium.org/10582012/diff/11006/base/at_exit.h
File base/at_exit.h (right):

http://codereview.chromium.org/10582012/diff/11006/base/at_exit.h#newcode65
base/at_exit.h:65: static void* CurrentAtExitManager();
On 2012/06/21 08:45:24, Paweł Hajdan Jr. wrote:
> nit: Let's not repeat the name, just static AtExitManager* current();

Done.

http://codereview.chromium.org/10582012/diff/11006/base/at_exit.h#newcode66
base/at_exit.h:66: static size_t CurrentAtExitCallbackStackSize();
On 2012/06/21 08:45:24, Paweł Hajdan Jr. wrote:
> nit: Let's remove the static and just name this CallbackStackSize() const;

Done.

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc#ne...
base/test/test_suite.cc:89: LOG(ERROR) << "AtExitManager: stack changed depth
across test " <<
On 2012/06/21 08:45:24, Paweł Hajdan Jr. wrote:
> This should also have a TODO above.
> 
> The error message should be more clear: that the AtExitManager is different,
> "stack changed depth" is confusing (what stack?).

Made it an error (doesn't currently occur anywhere), worked on the wording.

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc#ne...
base/test/test_suite.cc:98: ". Test should be surrounded by a
ShadowingAtExitManager.";
On 2012/06/21 08:45:24, Paweł Hajdan Jr. wrote:
> ShadowingAtExitManager is not always the right solution. Better to say
something
> about cleaning up global state before test exits.

New wording, but I'm still trying to think through if there's some way of saying
something more actionable.

http://codereview.chromium.org/10582012/diff/11006/base/test/test_suite.cc#ne...
base/test/test_suite.cc:337: if
(!CommandLine::ForCurrentProcess()->HasSwitch("single_process") &&
On 2012/06/21 08:45:24, Paweł Hajdan Jr. wrote:
> Don't hardcode command-line flags this way, we always have to use switches::
for
> that. If needed, move the relevant switch constants to base/

Done.

[Paweł Hajdan Jr., 2012-06-22 07:52:42]

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.cc
File base/at_exit.cc (right):

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.cc#newcode90
base/at_exit.cc:90: 
nit: Remove redundant empty line.

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.h
File base/at_exit.h (right):

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.h#newcode66
base/at_exit.h:66: size_t CallbackStackSize();
nit: CallbackStackSize() const;

http://codereview.chromium.org/10582012/diff/26001/base/test/test_switches.cc
File base/test/test_switches.cc (right):

http://codereview.chromium.org/10582012/diff/26001/base/test/test_switches.cc...
base/test/test_switches.cc:13: // Run the tests and the launcher in the same
process, and Chromium in single
This is not true. It runs Chrome in single-process mode. Please also rename the
flag name to kSingleProcessChromeFlag.

http://codereview.chromium.org/10582012/diff/26001/content/test/test_launcher.cc
File content/test/test_launcher.cc (left):

http://codereview.chromium.org/10582012/diff/26001/content/test/test_launcher...
content/test/test_launcher.cc:344:
new_cmd_line.AppendSwitch(kSingleProcessTestsFlag);
Also remove the declaration of kSingleProcessTestsFlag and
kSingleProcessTestsAndChromeFlag from content/public/test/test_luncher.h

[Scott Byer, 2012-06-22 22:02:55]

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.cc
File base/at_exit.cc (right):

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.cc#newcode90
base/at_exit.cc:90: 
On 2012/06/22 07:52:42, Paweł Hajdan Jr. wrote:
> nit: Remove redundant empty line.

Done.

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.h
File base/at_exit.h (right):

http://codereview.chromium.org/10582012/diff/26001/base/at_exit.h#newcode66
base/at_exit.h:66: size_t CallbackStackSize();
On 2012/06/22 07:52:42, Paweł Hajdan Jr. wrote:
> nit: CallbackStackSize() const;

Done.

http://codereview.chromium.org/10582012/diff/26001/base/test/test_switches.cc
File base/test/test_switches.cc (right):

http://codereview.chromium.org/10582012/diff/26001/base/test/test_switches.cc...
base/test/test_switches.cc:13: // Run the tests and the launcher in the same
process, and Chromium in single
On 2012/06/22 07:52:42, Paweł Hajdan Jr. wrote:
> This is not true. It runs Chrome in single-process mode. Please also rename
the
> flag name to kSingleProcessChromeFlag.

Done.

http://codereview.chromium.org/10582012/diff/26001/content/test/test_launcher.cc
File content/test/test_launcher.cc (left):

http://codereview.chromium.org/10582012/diff/26001/content/test/test_launcher...
content/test/test_launcher.cc:344:
new_cmd_line.AppendSwitch(kSingleProcessTestsFlag);
On 2012/06/22 07:52:42, Paweł Hajdan Jr. wrote:
> Also remove the declaration of kSingleProcessTestsFlag and
> kSingleProcessTestsAndChromeFlag from content/public/test/test_luncher.h

Done.

[Paweł Hajdan Jr., 2012-06-23 07:17:30]

LGTM

base/ owners, please take a look at base/at_exit changes

http://codereview.chromium.org/10582012/diff/37001/base/test/test_switches.cc
File base/test/test_switches.cc (right):

http://codereview.chromium.org/10582012/diff/37001/base/test/test_switches.cc...
base/test/test_switches.cc:14: const char kSingleProcessChromeFlag[]   =
"single-process";
nit: When sorting the header, make the order here match it.

http://codereview.chromium.org/10582012/diff/37001/base/test/test_switches.h
File base/test/test_switches.h (right):

http://codereview.chromium.org/10582012/diff/37001/base/test/test_switches.h#...
base/test/test_switches.h:13: extern const char kSingleProcessChromeFlag[];
nit: Sort.

[brettw, 2012-06-27 16:23:13]

lgtm

[commit-bot: I haz the power, 2012-06-27 16:25:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/10582012/43002

[commit-bot: I haz the power, 2012-06-27 17:55:00]

Change committed as 144488