Mojo Log service and a thread-safe client library.

services/log/log_impl_unittest.cc

+// 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 <stdio.h>
+#include <unistd.h>
+
+#include <utility>
+
+#include "base/message_loop/message_loop.h"
+#include "mojo/public/c/environment/logger.h"
+#include "mojo/public/cpp/application/application_impl.h"
+#include "mojo/public/cpp/application/application_test_base.h"
+#include "mojo/public/cpp/system/functions.h"
+#include "mojo/services/log/interfaces/entry.mojom.h"
+#include "mojo/services/log/interfaces/log.mojom.h"
+#include "services/log/log_impl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace log {
+namespace {
+
+using base::MessageLoop;
+using LogImplTest = mojo::test::ApplicationTestBase;
+
+// We need to supply a ApplicationConnection to LogImpl::Create().
+class TestApplicationConnection : public ApplicationConnection {
+ public:
+  const std::string& GetConnectionURL() override {
+    static std::string kConnectionURL = "mojo:log";
+    return kConnectionURL;
+  }
+
+  const std::string& GetRemoteApplicationURL() override {
+    static std::string kRemoteApplicationURL = "mojo:log_impl_unittest";
+    return kRemoteApplicationURL;
+  }
+
+  ServiceProvider* GetServiceProvider() override { return nullptr; }
+  void SetServiceConnectorForName(ServiceConnector* service_connector,
+                                  const std::string& name) override {}
+};
+
+// Tests the Log service implementation by calling its AddEntry and verifying
+// the log message it prints to the supplied FILE stream.
+TEST_F(LogImplTest, AddEntryOutput) {
+  // LogImpl will write to p[1] (which we wrap into a FILE*), we will EXPECT
+  // what's written by reading in p[0].
+  int p[2];
+  pipe(p);

[viettrungluu, 2015/12/15 18:33:08]

You should, say, ASSERT_EQ(0, pipe(p));.

[vardhan, 2015/12/16 18:29:05]

Done.

+
+  FILE* output_file = fdopen(p[1], "w");

[viettrungluu, 2015/12/15 18:33:08]

Probably should use ScopedFILE (base/files/scoped_file.h).

[vardhan, 2015/12/16 18:29:05]

I figured it wasn't necessary since i'm closing it manually below, but I guess
its better to be defensive

+  ASSERT_NE(nullptr, output_file);
+
+  LogPtr log;
+  auto log_request = GetProxy(&log);
+  TestApplicationConnection app_connection;
+  LogImpl::Create(&app_connection, std::move(log_request), output_file);

[viettrungluu, 2015/12/15 18:33:08]

Why don't you just directly put GetProxy(&log) in here, instead of using a
temporary + std::move?

[vardhan, 2015/12/16 18:29:05]

Done.

+
+  Entry entry;
+  entry.log_level = MOJO_LOG_LEVEL_INFO;
+  entry.timestamp = GetTimeTicksNow();
+  entry.source_file = "file.ext";
+  entry.source_line = 0;
+  entry.message = "1234567890";
+  log->AddEntry(entry.Clone());
+
+  entry.source_line = 1;
+  log->AddEntry(entry.Clone());
+
+  entry.source_file.reset();
+  log->AddEntry(entry.Clone());
+
+  entry.message.reset();
+  log->AddEntry(entry.Clone());
+
+  log.reset();
+
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE, MessageLoop::QuitWhenIdleClosure(),
+      base::TimeDelta::FromMilliseconds(10));

[viettrungluu, 2015/12/15 18:33:08]

Probably should use TestTimeouts::tiny_timeout(), from
base/test/test_timeouts.h.

[vardhan, 2015/12/16 18:29:05]

Done.

+
+  MessageLoop::current()->Run();
+  fclose(output_file);  // This closes p[1].
+
+  const char* expected_string =

[viettrungluu, 2015/12/15 18:33:08]

const char expected_string[] = ...

[vardhan, 2015/12/16 18:29:05]

Done.

+      "<mojo:log_impl_unittest> [INFO] file.ext: 1234567890\n"
+      "<mojo:log_impl_unittest> [INFO] file.ext:1: 1234567890\n"
+      "<mojo:log_impl_unittest> [INFO] 1234567890\n"
+      "<mojo:log_impl_unittest> [INFO] <no message>\n";
+  char actual_string[1024 * 10];
+  size_t nbytes = read(p[0], actual_string, strlen(expected_string));

[viettrungluu, 2015/12/15 18:33:08]

Theoretically, this should be wrapped in HANDLE_EINTR, but ... <shrug>.

Also, read() returns an ssize_t, not a size_t; you should assert that what it
returns is nonnegative.

Also, you should include <string.h> for strlen().

[vardhan, 2015/12/16 18:29:05]

Done.  (also, im thinking the EXPECT_EQ() is sufficient and don't need the
additional non-negative assert)

+  EXPECT_EQ(static_cast<size_t>(strlen(expected_string)), nbytes);
+
+  close(p[0]);
+
+  actual_string[nbytes] = '\0';
+  EXPECT_STREQ(expected_string, actual_string);
+}
+
+}  // namespace
+}  // namespace log
+}  // namespace mojo