[Offline Pages] Create a event/activity logger

components/offline_pages/background/request_coordinator_event_logger_unittest.cc

Index: components/offline_pages/background/request_coordinator_event_logger_unittest.cc
diff --git a/components/offline_pages/background/request_coordinator_event_logger_unittest.cc b/components/offline_pages/background/request_coordinator_event_logger_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6e82039280af62d8b972a3188565e85605c183c1
--- /dev/null
+++ b/components/offline_pages/background/request_coordinator_event_logger_unittest.cc
@@ -0,0 +1,56 @@
+// Copyright 2016 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 "components/offline_pages/background/request_coordinator_event_logger.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace offline_internals {

[fgorski, 2016/06/23 16:34:06]

I only noticed that now, but why did we go with a different namespace than
offline_pages?

If this is specific to this file, please fix it. I don't think we need a second
namespace for our stuff. (I think we only did that on Android).

[chili, 2016/06/24 02:45:50]

Oops. brainfart.  Fixed.

+
+namespace {

[fgorski, 2016/06/23 16:34:06]

I don't think we need an anonymous namespace here.

[chili, 2016/06/24 02:45:50]

Removed. A couple tests I was looking at had this.  Are there some rules
surrounding use of this?

+
+class RequestCoordinatorEventLoggerTest : public ::testing::Test {

[fgorski, 2016/06/23 16:34:06]

Since the fixture is empty you can use TEST() macro instead of TEST_F()
https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?rcl=...

[chili, 2016/06/24 02:45:51]

Done.

+ public:
+  RequestCoordinatorEventLoggerTest() {}
+  ~RequestCoordinatorEventLoggerTest() {}
+};
+
+TEST_F(RequestCoordinatorEventLoggerTest, RecordsWhenLoggingIsOn) {
+  OfflinePageModelEventLogger logger;

[Pete Williamson, 2016/06/24 00:38:32]

Did you mean RequestCoordinatorEventLogger instead of
OfflinePageModelEventLogger?  (I see it in other functions in this file too,
maybe a cut and paste didn't get fully customized).

[chili, 2016/06/24 02:45:51]

haha good catch
oops

+  std::vector<std::string> log;
+
+  logger.SetIsLogging(true);
+  logger.RecordSavePageRequestUpdated("last_n", "pending", "foo");

[Pete Williamson, 2016/06/24 00:38:32]

It might be nice to make last_n, pending, and foo into string constants at the
top of this file.

[chili, 2016/06/24 02:45:51]

Done.

+  logger.GetLogs(log);
+
+  EXPECT_EQ(1, log.size());

[Pete Williamson, 2016/06/24 00:38:32]

Can we also test that the content of the logs are what we expect?  Something
like:

EXPECT_EQ("2010:06:21:1230:45.0071 - last_n - pending - foo",
log[0].getString());

Doing it in one test should be enough, I don't see a need to do it for every
test.

If it is tough to get the time string to match what you expect, I'd be OK with
just checking the part after the time string.

Alternately, you could use a base::Clock interface to get your time from, and
have a test clock that outputs a time you expect on the test so you can test
against an exact string.  I think FGorski may have made a similar suggestion
elsewhere in the changelist.

[chili, 2016/06/24 02:45:51]

As explained in the other comment, I ultimately didn't end up testing for the
exact string because I didn't think test update should be necessary if the
logging string changes for any reason (better wording or typo/etc).  I'm happy
to add it in if you think otherwise.

[Pete Williamson, 2016/06/24 16:52:58]

OK, I understand that the test will break if we change the log string format,
but that just means we need to fix the test, and that's expected to fix tests
when we change code.  Let's add the test.

[chili, 2016/06/24 20:30:10]

Added the test.

+}
+
+TEST_F(RequestCoordinatorEventLoggerTest, RecordsWhenLoggingIsOff) {
+  OfflinePageModelEventLogger logger;
+  std::vector<std::string> log;
+
+  logger.SetIsLogging(false);
+  logger.RecordSavePageRequestUpdated("last_n", "pending", "foo");
+  logger.GetLogs(log);
+
+  EXPECT_EQ(1, log.size());
+}
+
+TEST_F(RequestCoordinatorEventLoggerTest, DoesNotExceedMaxSize) {
+  OfflinePageModelEventLogger logger;
+  std::vector<std::string> log;
+
+  logger.SetIsLogging(true);
+  for (int i = 0; i < 21; i++) {

[fgorski, 2016/06/23 16:34:06]

I am guessing your buffer size is 20.
Could you please document that better. Perhaps a const instead of a magic
number?

[Pete Williamson, 2016/06/24 00:38:31]

Maybe you could expose MAX_LOGGED_ACTIVITY_COUNT in offline_event_logger.h
(instead of in a private namespace) and use that here?

[chili, 2016/06/24 02:45:51]

Done.

+    logger.RecordSavePageRequestUpdated("last_n", "pending", "foo");
+  }
+  logger.GetLogs(log);
+
+  EXPECT_EQ(20, log.size());

[fgorski, 2016/06/23 16:34:06]

ditto

[chili, 2016/06/24 02:45:51]

Done.

+}
+
+} // namespace
+
+} // namespace offline_internals