First cut at UI for saving net_logs data into a temporary file on

chrome/browser/net/net_log_temp_file_unittest.cc

Index: chrome/browser/net/net_log_temp_file_unittest.cc
===================================================================
--- chrome/browser/net/net_log_temp_file_unittest.cc	(revision 0)
+++ chrome/browser/net/net_log_temp_file_unittest.cc	(revision 0)
@@ -0,0 +1,221 @@
+// Copyright (c) 2013 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 "chrome/browser/net/net_log_temp_file.h"
+
+#include "base/basictypes.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/values.h"
+#include "build/build_config.h"
+#include "chrome/browser/net/chrome_net_log.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::BrowserThread;
+
+class CustomNetLogTempFile : public NetLogTempFile {

[mmenke, 2013/01/23 19:01:27]

nit:  Think "TestNetLogTempFile" or "NetLogTempFileForTesting" is clearer than
"CustomNetLogTempFile", particularly if we have to friend it in non-test code
(Which I don't think we do, see other comment about that).

[ramant (doing other things), 2013/01/24 03:23:55]

Done.

+ public:
+  void set_lie_about_file_existence(bool lie_about_file_existence) {
+    lie_about_file_existence_ = lie_about_file_existence;
+  }
+
+ protected:
+  explicit CustomNetLogTempFile(ChromeNetLog* chrome_net_log)
+      : NetLogTempFile(chrome_net_log),
+        lie_about_file_existence_(false) {
+  }
+
+  bool GetNetExportLog(FilePath* path) {
+    bool file_exists = NetLogTempFile::GetNetExportLog(path);
+    if (lie_about_file_existence_)
+      return false;
+    return file_exists;
+  }
+
+ private:
+  friend class NetLogTempFileTest;

[mmenke, 2013/01/23 19:01:27]

Don't think we need to friend the test, can just make the constructor public.

[ramant (doing other things), 2013/01/24 03:23:55]

Made constructor and NetExportLogExists public.

Done.

+
+  bool lie_about_file_existence_;
+};
+
+class NetLogTempFileTest : public ::testing::Test {
+ protected:
+  NetLogTempFileTest() {
+    net_log_.reset(new ChromeNetLog);
+
+    file_user_blocking_thread_.reset(
+        new content::TestBrowserThread(BrowserThread::FILE_USER_BLOCKING));
+    file_user_blocking_thread_->Start();
+
+    net_log_temp_file_ = new CustomNetLogTempFile(net_log_.get());
+  }
+
+  virtual void SetUp() OVERRIDE {
+    // Get a temporary file name for unit tests.
+    ASSERT_TRUE(file_util::CreateTemporaryFile(&net_export_log_));
+
+    FilePath net_export_log_filename = net_export_log_.BaseName();
+    net_log_temp_file_->log_filename_ = net_export_log_filename.value();

[mmenke, 2013/01/23 19:01:27]

nit:  Could just move this to the constructor, and move TearDown to the
destructor.  Each execution of a test creates a new test fixture.

[ramant (doing other things), 2013/01/24 03:23:55]

Would like to call ASSERT_TRUE to check the return value of CreateTemporaryFile.
Constructor doesn't seem to allow it.

[mmenke, 2013/01/24 03:55:34]

Didn't realize that, fine as-is, then.

+  }
+
+  virtual void TearDown() OVERRIDE {
+    // Delete the temporary file we have created.
+    ASSERT_TRUE(file_util::Delete(net_log_temp_file_->log_path(), false));

[mmenke, 2013/01/23 19:01:27]

Suggest we use |net_export_log_| instead of net_log_temp_file_->log_path()
everywhere, so we aren't depending on any NetLogTempFile state.

[ramant (doing other things), 2013/01/24 03:23:55]

Done.

+  }
+
+  CustomNetLogTempFile* net_log_temp_file_;
+  FilePath net_export_log_;
+
+ private:
+  scoped_ptr<ChromeNetLog> net_log_;
+  scoped_ptr<content::TestBrowserThread> file_user_blocking_thread_;
+};
+
+TEST_F(NetLogTempFileTest, InitAllowStart) {
+  net_log_temp_file_->set_lie_about_file_existence(true);
+  net_log_temp_file_->Init();
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START, net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path);
+}
+
+TEST_F(NetLogTempFileTest, InitAllowStartOrSend) {
+  net_log_temp_file_->Init();
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START_SEND", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path);
+  EXPECT_TRUE(file_util::PathExists(log_path));
+}
+
+TEST_F(NetLogTempFileTest, ProcessCommandDoStart) {

[mmenke, 2013/01/23 19:01:27]

Not sure this test really gets us anything that DoStop doesn't.

[ramant (doing other things), 2013/01/24 03:23:55]

Merged the tests into ProcessCommandDoStartAndStop.

Done.

+  // Execute DO_START command via ProcessCommand and verfiy that we have
+  // transitioned to STATE_ALLOW_STOP state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_STOP", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_STOP, net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path);
+  EXPECT_TRUE(file_util::PathExists(log_path));
+  int64 file_size;
+  EXPECT_TRUE(file_util::GetFileSize(log_path, &file_size));
+  EXPECT_GT(file_size, 0);
+
+  // Execute DO_STOP so that temporary file could be deleted.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+  // TODO(rtenneti): Add check for the contents of the file, by adding a random
+  // NetLog event and verify it is there.
+}
+
+TEST_F(NetLogTempFileTest, ProcessCommandDoStop) {
+  // Execute DO_START command via ProcessCommand and verfiy that we transition
+  // to ALLOW_STOP state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);

[mmenke, 2013/01/23 19:01:27]

Should use EXPECT_TRUE on calls to dict->GetString.

[ramant (doing other things), 2013/01/24 03:23:55]

Done.

+  EXPECT_EQ("ALLOW_STOP", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_STOP, net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path);
+  EXPECT_TRUE(file_util::PathExists(log_path));
+
+  // Execute DO_STOP command via ProcessCommand and verfiy that we have
+  // transitioned to STATE_ALLOW_START_SEND state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+  dict = net_log_temp_file_->GetState();
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START_SEND", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+  FilePath log_path1 = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path1);
+  EXPECT_TRUE(file_util::PathExists(log_path1));
+  // TODO(rtenneti): Add check for the contents of the file, by adding a random
+  // NetLog event and verify it is there.
+}
+
+TEST_F(NetLogTempFileTest, DoStartClearsFile) {
+  // Execute DO_START command via ProcessCommand and verfiy that we transition
+  // to ALLOW_STOP state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_STOP", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_STOP, net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path);
+  EXPECT_TRUE(file_util::PathExists(log_path));
+  int64 start_file_size;
+  EXPECT_TRUE(file_util::GetFileSize(log_path, &start_file_size));

[mmenke, 2013/01/23 19:01:27]

To keep the length of this test down, sugget deleting a bunch of these expects. 
Everything above this line, except perhaps the state check, GetState checks,
etc, would make this a much more focused test, and easier to follow.

[ramant (doing other things), 2013/01/24 03:23:55]

Done.

+
+  // Execute DO_STOP command via ProcessCommand and verfiy that we have
+  // transitioned to STATE_ALLOW_START_SEND state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+  dict = net_log_temp_file_->GetState();
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START_SEND", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+  FilePath log_path1 = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path1);
+  EXPECT_EQ(log_path, log_path1);
+  EXPECT_TRUE(file_util::PathExists(log_path1));
+  int64 stop_file_size;
+  EXPECT_TRUE(file_util::GetFileSize(log_path, &stop_file_size));
+  EXPECT_GE(stop_file_size, start_file_size);
+
+  // Add some junk at the end of the file.
+  std::string junk_data("Hello");
+  EXPECT_GT(file_util::AppendToFile(
+      log_path1, junk_data.c_str(), junk_data.size()), 0);

[mmenke, 2013/01/23 19:01:27]

Ahh, you weren't following me here...What I was suggesting was that, after the
next DO_START call, we do:

net_log_->AddEvent(net::NetLog::TYPE_CANCELLED);

And then make sure that, after we stop logging, the file is larger than a net
log created without that event.

Could do it in another test that's just a copy of this one.  This test makes
sure the sizes are the same when neither has any events, the other test would
make sure that when one log has events, it's larger (So presumably is correctly
logging events).

[ramant (doing other things), 2013/01/24 03:23:55]

Done.

+  int64 junk_file_size;
+  EXPECT_TRUE(file_util::GetFileSize(log_path1, &junk_file_size));
+  EXPECT_GT(junk_file_size, start_file_size);
+
+  // Execute DO_START command and make sure the file is back to the size before
+  // addition of junk data.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  dict = net_log_temp_file_->GetState();
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_STOP", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_STOP, net_log_temp_file_->state());
+  FilePath log_path2 = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path2);
+  EXPECT_TRUE(file_util::PathExists(log_path2));
+  int64 new_file_size;
+  EXPECT_TRUE(file_util::GetFileSize(log_path, &new_file_size));
+  EXPECT_EQ(new_file_size, start_file_size);
+
+  // Execute DO_STOP so that temporary file could be deleted.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+}
+
+TEST_F(NetLogTempFileTest, GetFilePath) {

[mmenke, 2013/01/23 19:01:27]

Suggest merging this with InitAllowStart / InitAllowStartStop, for the broadest
coverage.  Check the results of GetFilePath after init, after start, and after
stop.

Could either get rid of this test, and add checks in the other two tests, or
split this test in two, and have one lie about file's initial existence.

This also covers the transition from ALLOW_START to ALLOW_STOP, which none of
these tests currently do.

[ramant (doing other things), 2013/01/24 03:23:55]

Done.

+  // Execute DO_START and DO_STOP commands via ProcessCommand and verfiy that we
+  // have transitioned to STATE_ALLOW_START_SEND state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_EQ(net_export_log_, log_path);
+
+  FilePath net_export_file_path;
+  EXPECT_TRUE(net_log_temp_file_->GetFilePath(&net_export_file_path));
+  EXPECT_TRUE(file_util::PathExists(net_export_file_path));
+}