Changed when the UMA metric Ash.NumberOfVisibleWindowsInPrimaryDisplay is recorded.

ash/metrics/user_metrics_recorder_unittest.cc

Index: ash/metrics/user_metrics_recorder_unittest.cc
diff --git a/ash/metrics/user_metrics_recorder_unittest.cc b/ash/metrics/user_metrics_recorder_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..34a4263a581b2a3eb09dc4e7b7fb55aa6fe9cb7e
--- /dev/null
+++ b/ash/metrics/user_metrics_recorder_unittest.cc
@@ -0,0 +1,188 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/metrics/user_metrics_recorder.h"
+
+#include "ash/shelf/shelf_layout_manager.h"
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/test/test_session_state_delegate.h"
+#include "base/test/histogram_tester.h"
+#include "ui/aura/window.h"
+
+namespace ash {
+namespace {
+
+const char kAsh_ShelfAlignmentOverTime[] = "Ash.ShelfAlignmentOverTime";
+
+const char kAsh_NumberOfVisibleWindowsInPrimaryDisplay[] =
+    "Ash.NumberOfVisibleWindowsInPrimaryDisplay";
+
+const char kAsh_ActiveWindowShowTypeOverTime[] =
+    "Ash.ActiveWindowShowTypeOverTime";
+
+// Test specific subclass of a UserMetricsRecorder that promotes the visibility
+// of some protected level functions to enable testing. The repeating timer of
+// the UserMetricsRecorder will NOT be started. Use the |RecordPeriodicMetrics|
+// method to trigger a recording.
+class TestUserMetricsRecorder : public UserMetricsRecorder {

[pkotwicz, 2015/04/17 13:45:38]

Extending the real class is a common pattern in Chrome for tests. However, I
don't think that it is necessary in this case.

If you make UserMetricsRecorder a friend of UserMetricsRecorderTest, I think you
can access all the privates that you need.

I think that it is preferable to get rid of the constructor with the boolean
parameter and call UserMetricsRecorder.timer_.Stop() after the
UserMetricsRecorder has been constructed. (I suspect that you will get different
opinions about this if you ask different devs). I think that we like to avoid
modifying the classes that we are testing for the sake of testing if at all
possible.
An argument can be made that you want TestUserMetricsRecorder to be generic and
used by other test classes. Chrome has a philosophy "Do not make something any
more generic than currently needed" and I agree with this philosophy.

[bruthig, 2015/04/17 14:41:23]

Out of curiosity when would it be necessary?
To be honest I'm not a big fan of either approach.  The reason is that they are
not scalable.  In either case, if you have another test fixture that want's to
use the UserMetricsRecorder it will require another friend to be declared or
another custom test specific subclass.  The only reason I chose this approach is
because I didn't want to challenge the whole world all at once ;) I am happy for
the time being to use whichever the owner's prefer.  I really don't want to have
to use the FRIEND_TEST_ALL_PREFIXES macro for each and every test though.

From a testability perspective the better approach would be to split the
UserMetricsRecorder class in to two classes.  The first class would be
responsible for "how the metrics are recorded".  More concretely it would
contain the RecordPeriodicMetrics and the RecordUserMetricsAction methods which
would have to both be public by necessity.  The second class would handle "when
the periodic metrics are recorded".  It would be responsible for owning and
managing the timer, connecting the timer to the RecordPeriodicMetrics method as
well as checking the "should I log conditions" like UserIsActive.
It is exactly this philosophy that I am challenging and I am trying to show the
benefits of the alternative :)  Testing is considered a second class citizen in
the world of chrome and currently is not 'important' enough to consider it as a
use case or client of the system under test and this is the same push back I
have received on multiple occasions. I am still searching for the underlying
reason of this approach so that we can evaluate the pros and cons of it.

By designing software for testability you inevitably give the test target two
(if not more) use cases and you are right, this will force it to be slightly
more generic.  This has the beneficial "side" effects of making that test target
better defined and more portable for reuse, not to mention it is well tested.

[pkotwicz, 2015/04/17 17:45:16]

Extending the real class is necessary when the test wants to change the behavior
of one of the functions of the real class. For instance, this is the case of
TestFaviconHandler w.r.t to the real class FaviconHandler. I am going to
completely rewrite favicon_handler_unittest.cc so lets not discuss whether
favicon_handler_unittest.cc requires this.

I think we want to avoid using the FRIEND_TEST_ALL_PREFIXES macro in new tests
and just make the test harness a friend of the class being tested. For instance,
ResourceBundleTest is a friend of ResourceBundle.

[sadrul, 2015/04/17 22:55:39]

In a number of places, we friend a TestApi class to expose the internal bits for
tests. The TestApi classes are reusable, so you don't need to either keep adding
new friends, or use FRIEND_TEST_ALL_PREFIXES, for new tests. (look for TestApi
for various examples)
I do not think this is right. We do want to make sure the production code is
easily testable, and make necessary changes to allow that, as long as it doesn't
hurt the code (maintainability/quality/performance etc. -wise). I suppose we
haven't necessarily always done a good job of it, or there isn't a universal
approach that everyone agrees with, so this is something that could be improved.

[bruthig, 2015/04/21 14:34:05]

I've reworked it to use the TestAPI approach and I'm quite happy with it. 
Thanks for pointing me in that direction sadrul@.

+ public:
+  TestUserMetricsRecorder();
+  ~TestUserMetricsRecorder() override;
+
+  using UserMetricsRecorder::RecordPeriodicMetrics;
+  using UserMetricsRecorder::UserIsActive;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestUserMetricsRecorder);
+};
+
+TestUserMetricsRecorder::TestUserMetricsRecorder()
+    : UserMetricsRecorder(false) {
+}
+
+TestUserMetricsRecorder::~TestUserMetricsRecorder() {
+}
+
+}  // namespace
+
+// Test fixture for the UserMetricsRecorder class.
+class UserMetricsRecorderTest : public test::AshTestBase {
+ public:
+  UserMetricsRecorderTest();
+  ~UserMetricsRecorderTest() override;
+
+  void SetUp() override;
+  void TearDown() override;
+
+  // Convenience function to trigger the periodic metrics recording of the test
+  // target.
+  void RecordPeriodicMetrics();
+
+  // Sets the current user session to be active.
+  void SetUserActive();
+
+  // Sets the current user session to be inactive.
+  void SetUserNotActive();
+
+  TestUserMetricsRecorder& user_metrics_recorder() {
+    return user_metrics_recorder_;
+  }
+
+  base::HistogramTester& histograms() { return histograms_; }
+
+  test::TestSessionStateDelegate* session_state_delegate() {
+    return session_state_delegate_;
+  }
+
+ private:
+  // The test target.
+  TestUserMetricsRecorder user_metrics_recorder_;
+
+  // Histogram value verifier.
+  base::HistogramTester histograms_;
+
+  // The current SessionStateDelegate used by the test fixture. Not owned.
+  test::TestSessionStateDelegate* session_state_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserMetricsRecorderTest);
+};
+
+UserMetricsRecorderTest::UserMetricsRecorderTest() {
+}
+
+UserMetricsRecorderTest::~UserMetricsRecorderTest() {
+}
+
+void UserMetricsRecorderTest::SetUp() {
+  test::AshTestBase::SetUp();
+
+  session_state_delegate_ = static_cast<ash::test::TestSessionStateDelegate*>(
+      ash::Shell::GetInstance()->session_state_delegate());
+}
+
+void UserMetricsRecorderTest::TearDown() {
+  session_state_delegate_ = nullptr;
+
+  test::AshTestBase::TearDown();
+}
+
+void UserMetricsRecorderTest::RecordPeriodicMetrics() {
+  user_metrics_recorder_.RecordPeriodicMetrics();
+}
+
+void UserMetricsRecorderTest::SetUserActive() {
+  SetSessionStarted(true);
+  session_state_delegate()->UnlockScreen();
+  ASSERT_TRUE(user_metrics_recorder().UserIsActive());
+}
+
+void UserMetricsRecorderTest::SetUserNotActive() {
+  SetSessionStarted(false);
+  session_state_delegate()->UnlockScreen();
+  ASSERT_FALSE(user_metrics_recorder().UserIsActive());
+}
+
+// Verify that the user is active when logged in and the screen is unlocked.
+TEST_F(UserMetricsRecorderTest, VerifyUserIsActiveWhenLoggedIn) {
+  SetSessionStarted(true);
+  session_state_delegate()->UnlockScreen();
+  EXPECT_TRUE(user_metrics_recorder().UserIsActive());
+}
+
+// Verify that the user is not active when not logged in.
+TEST_F(UserMetricsRecorderTest, VerifyUserIsNotActiveWhenNotLoggedIn) {
+  SetSessionStarted(false);
+  session_state_delegate()->UnlockScreen();
+  EXPECT_FALSE(user_metrics_recorder().UserIsActive());
+}
+
+// Verify that the user is not active when logged in but the screen is locked.
+TEST_F(UserMetricsRecorderTest, VerifyUserIsNotActiveWhenAccountIsLocked) {
+  SetSessionStarted(true);
+  session_state_delegate()->LockScreen();
+  EXPECT_FALSE(user_metrics_recorder().UserIsActive());
+}
+
+// Verifies that the stats dependent on the presence of a ShelfLayoutManager
+// are recorded when a ShelfLayoutManager exists.
+// TODO(bruthig): Add a test to verify the stats are not recorded when a
+// ShelfLayoutManager does not exist.

[pkotwicz, 2015/04/17 13:45:38]

When does the ShelfLayoutManager not exist?

[bruthig, 2015/04/17 14:41:23]

I'm not sure, I'm not familiar with this and only added this test because of the
conditional logic in the test target.

[pkotwicz, 2015/04/17 17:45:16]

I think that the test would 10x more useful if it had a comment as to when the
ShelfLayouManager does not exist. We may find out that we can just nuke the
ShelfLayoutManager check

[bruthig, 2015/04/21 14:34:05]

To the best of my knowledge a ShelfLayoutManager may not exist in during Shell
initialization or destruction. I've update the TODO to consider investigating
this.  I don't want to include that as a part of this change because I feel it
is a higher risk and not actually tied to work encompassed by this review.

+TEST_F(UserMetricsRecorderTest,
+       VerifyStatsRecordedWhenAShelfLayoutManagerExists) {
+  SetUserActive();
+  ASSERT_TRUE(ShelfLayoutManager::ForShelf(Shell::GetPrimaryRootWindow()));
+  RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_ShelfAlignmentOverTime, 1);
+}
+
+// Verifies that the UserIsActive() dependent stats are not recorded when a
+// user is not active.
+TEST_F(UserMetricsRecorderTest, VerifyStatsRecordedWhenUserIsNotActive) {
+  SetUserNotActive();
+  RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_NumberOfVisibleWindowsInPrimaryDisplay, 0);
+}
+
+// Verifies that the UserIsActive() dependent stats are recorded when a user
+// is active.
+TEST_F(UserMetricsRecorderTest, VerifyStatsRecordedWhenUserIsActive) {
+  SetUserActive();
+  RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_NumberOfVisibleWindowsInPrimaryDisplay, 1);
+}
+
+// Verifies which stats are always recorded and are not dependent on conditional
+// logic for when they are recorded.
+TEST_F(UserMetricsRecorderTest, VerifyStatsRecordedByRecordPeriodicMetrics) {
+  SetUserActive();
+  RecordPeriodicMetrics();
+
+  histograms().ExpectTotalCount(kAsh_ActiveWindowShowTypeOverTime, 1);
+}
+
+}  // namespace ash