Added test support to observe and verify UMA user actions during tests.

Added test support to observe and verify UMA user actions during tests.

There are currently a few custom user action observers to record and verify
UMA user actions during tests.  This patch adds that support to a common location.

TEST=UserActionTesterTest.GetActionCountWhenNoActionsHaveBeenRecorded
TEST=UserActionTesterTest.GetActionCountWhenActionsHaveBeenRecorded
TEST=UserActionTesterTest.ResetCountsWhenNoActionsHaveBeenRecorded
TEST=UserActionTesterTest.ResetCountsWhenActionsHaveBeenRecorded
TEST=UserActionTesterTest.VerifyUserActionTesterListensForUserActions

BUG=475181
Committed: https://crrev.com/965a17ddac517f913ef767454521727ac6f5107d
Cr-Commit-Position: refs/heads/master@{#324953}

[bruthig, 2015-04-08 20:34:19]

bruthig@chromium.org changed reviewers:
+ danakj@chromium.org, rkaplow@chromium.org

[bruthig, 2015-04-08 20:34:19]

rkaplow@: Can you please take a look at base/test/user_action_tester* with an
UMA expert perspective?

danakj@: Can you please take a look at everything from an ownership point of
view?

[Ilya Sherman, 2015-04-08 20:49:18]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-04-08 20:49:19]

Thanks!  Could you please also migrate some or all of the custom observers over
to using this implementation?

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_tester.h
File base/test/user_action_tester.h (right):

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester.h:20: virtual ~UserActionTester();
nit: Does this really need to be virtual?  That is, is this class subclassable?

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
File base/test/user_action_tester_unittest.cc (right):

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester_unittest.cc:13: typedef testing::Test
UserActionTesterTest;
nit: Please omit this, and just use TEST rather than TEST_F

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester_unittest.cc:31:
user_action_tester.OnUserAction_for_test(kUserAction2);
Rather than calling into the private method, please test the public interface,
i.e. recording user actions via base::RecordAction() and
base::RecordComputedAction().

[bruthig, 2015-04-09 23:17:52]

I've updated the CL as per isherman@'s comments.

isherman@: Can you please have another look?

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_tester.h
File base/test/user_action_tester.h (right):

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester.h:20: virtual ~UserActionTester();
On 2015/04/08 20:49:19, Ilya Sherman wrote:
> nit: Does this really need to be virtual?  That is, is this class
subclassable?

Done.

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
File base/test/user_action_tester_unittest.cc (right):

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester_unittest.cc:13: typedef testing::Test
UserActionTesterTest;
On 2015/04/08 20:49:19, Ilya Sherman wrote:
> nit: Please omit this, and just use TEST rather than TEST_F

Done.

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester_unittest.cc:31:
user_action_tester.OnUserAction_for_test(kUserAction2);
On 2015/04/08 20:49:19, Ilya Sherman wrote:
> Rather than calling into the private method, please test the public interface,
> i.e. recording user actions via base::RecordAction() and
> base::RecordComputedAction().

Done.

In this particular case I am somewhat indifferent about including the
base::RecordAction(...) in the scope of each and every test simply because the
the added code/time is minimal/negligible.  However, I'm not sure I would
totally agree with the statement 'only test the public interface'.  Perhaps we
could have a friendly debate offline on the subject?

[Ilya Sherman, 2015-04-09 23:49:35]

Thanks!  As I wrote in my previous review, could you please also migrate some or
all of the custom observers over to using this implementation?

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
File base/test/user_action_tester_unittest.cc (right):

https://codereview.chromium.org/1073613002/diff/1/base/test/user_action_teste...
base/test/user_action_tester_unittest.cc:31:
user_action_tester.OnUserAction_for_test(kUserAction2);
On 2015/04/09 23:17:52, bruthig wrote:
> On 2015/04/08 20:49:19, Ilya Sherman wrote:
> > Rather than calling into the private method, please test the public
interface,
> > i.e. recording user actions via base::RecordAction() and
> > base::RecordComputedAction().
> 
> Done.
> 
> In this particular case I am somewhat indifferent about including the
> base::RecordAction(...) in the scope of each and every test simply because the
> the added code/time is minimal/negligible.  However, I'm not sure I would
> totally agree with the statement 'only test the public interface'.  Perhaps we
> could have a friendly debate offline on the subject?

Happy to discuss offline, though I'm not sure either of us will really convince
the other to change our views ;)

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
File base/test/user_action_tester_unittest.cc (right):

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
base/test/user_action_tester_unittest.cc:21:
base::RecordAction(base::UserMetricsAction(user_action));
Could you please run tools/metrics/actions/extract_actions.py and verify whether
it outputs a warning for this call to base::RecordAction with a non-literal
string?

[bruthig, 2015-04-10 00:05:26]

I've created www.crbug.com/475747 to track the work of actually migrating the
tests to use the UserActionTester.

I've verified no warnings from extract_actions.py.

danakj@: Can you please have a look from an owner's point of view?

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
File base/test/user_action_tester_unittest.cc (right):

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
base/test/user_action_tester_unittest.cc:21:
base::RecordAction(base::UserMetricsAction(user_action));
On 2015/04/09 23:49:35, Ilya Sherman wrote:
> Could you please run tools/metrics/actions/extract_actions.py and verify
whether
> it outputs a warning for this call to base::RecordAction with a non-literal
> string?

Verified, no Warning output.

[Ilya Sherman, 2015-04-10 01:02:27]

LGTM, thanks.

[rkaplow, 2015-04-10 15:39:01]

lgtm

[bruthig, 2015-04-13 16:46:35]

bruthig@chromium.org changed reviewers:
+ mark@chromium.org
- danakj@chromium.org

[bruthig, 2015-04-13 16:47:27]

Replacing danakj@ with mark@ because danajk@ is away until April 19th. 

mark@chromium.org: Can you please have a look?

[Mark Mentovai, 2015-04-13 17:30:00]

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
File base/test/user_action_tester.cc (right):

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
base/test/user_action_tester.cc:7: #include <map>
The .h already brought in <map>, <string>, and "base/metrics/user_metrics.h", so
you don’t need them here.

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
File base/test/user_action_tester.h (right):

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
base/test/user_action_tester.h:16: // by the tests, so that they can be examined
afterwards for correctness.
Is this thread-safe? Does it need to be?

[bruthig, 2015-04-13 19:21:46]

I've updated the review as per mark@'s comments.

mark@, can you please have another look?

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
File base/test/user_action_tester.cc (right):

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
base/test/user_action_tester.cc:7: #include <map>
On 2015/04/13 17:30:00, Mark Mentovai wrote:
> The .h already brought in <map>, <string>, and "base/metrics/user_metrics.h",
so
> you don’t need them here.

Done.

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
File base/test/user_action_tester.h (right):

https://codereview.chromium.org/1073613002/diff/20001/base/test/user_action_t...
base/test/user_action_tester.h:16: // by the tests, so that they can be examined
afterwards for correctness.
On 2015/04/13 17:30:00, Mark Mentovai wrote:
> Is this thread-safe? Does it need to be?

Updated documentation.

There isn't currently a use case that requires this to be thread-safe.

[Mark Mentovai, 2015-04-13 20:08:37]

LGTM otherwis

https://codereview.chromium.org/1073613002/diff/40001/base/test/user_action_t...
File base/test/user_action_tester.cc (right):

https://codereview.chromium.org/1073613002/diff/40001/base/test/user_action_t...
base/test/user_action_tester.cc:9: #include "base/metrics/user_metrics.h"
Me too.

[bruthig, 2015-04-13 20:28:28]

Removed unnecessary #include from user_action_tester.cc.

No need to review.

[bruthig, 2015-04-13 20:28:36]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-04-13 20:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, isherman@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1073613002/#ps60001
(title: "Removed unnecessary #include from user_action_tester.cc")

[commit-bot: I haz the power, 2015-04-13 20:29:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073613002/60001

[commit-bot: I haz the power, 2015-04-14 00:01:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-14 00:02:31]

Patchset 4 (id:??) landed as
https://crrev.com/965a17ddac517f913ef767454521727ac6f5107d
Cr-Commit-Position: refs/heads/master@{#324953}