[ChromeDriver] Subscribe PerformanceLogger to CommandListener interface

chrome/test/chromedriver/commands_unittest.cc

Index: chrome/test/chromedriver/commands_unittest.cc
diff --git a/chrome/test/chromedriver/commands_unittest.cc b/chrome/test/chromedriver/commands_unittest.cc
index 44571863054bbd1a38e1ca25808019295033843a..bf219118e9cb69b92d2ca8fe57a8a26c76c78a6a 100644
--- a/chrome/test/chromedriver/commands_unittest.cc
+++ b/chrome/test/chromedriver/commands_unittest.cc
@@ -546,3 +546,124 @@ TEST(CommandsTest, ErrorFindChildElement) {
       ExecuteFindChildElements(
           1, &session, &web_view, element_id, params, &result).code());
 }
+
+namespace {
+
+class MockCommandListener : public CommandListener {

[stgao, 2014/07/01 19:02:27]

duplicate code, how about extracting it out as a command_listener_stub.cc/h as I
suggested last time?

[johnmoore, 2014/07/02 22:17:44]

I have removed session->AddListener, so the duplicate code no longer exists.

[stgao, 2014/07/11 21:36:07]

It seems there is another MockCommandListener in
chrome/test/chromedriver/command_listener_proxy_unittest.cc

It would be better if it could be extracted into separate
command_listener_stub.cc/h files.

+ public:
+  MockCommandListener() : called_(false) {}
+  virtual ~MockCommandListener() {}
+
+  virtual Status BeforeCommand(const std::string& command_name) OVERRIDE {
+    called_ = true;
+    EXPECT_STREQ("cmd", command_name.c_str());
+    return Status(kOk);
+  }
+
+  void VerifyCalled() {
+    EXPECT_TRUE(called_);
+  }
+
+  void VerifyNotCalled() {
+    EXPECT_FALSE(called_);
+  }
+
+ private:
+  bool called_;
+};
+
+Status ExecuteAddListenerToSession(
+    MockCommandListener* listener,
+    Session* session,
+    const base::DictionaryValue& params,
+    scoped_ptr<base::Value>* return_value) {
+  session->AddListener(listener);
+  listener->VerifyNotCalled();
+  return Status(kOk);
+}
+
+Status ExecuteTestListener(

[stgao, 2014/07/01 19:02:27]

The name is not that clear for setting session->quit = true.

[johnmoore, 2014/07/02 22:17:44]

I have updated the command and callback names to match the style of the rest of
the tests.

+    Session* session,
+    const base::DictionaryValue& params,
+    scoped_ptr<base::Value>* return_value) {
+  session->quit = true;
+  return Status(kOk);
+}
+
+void VerifyListenerNotNotifiedAfterCommandRun(
+    MockCommandListener* listener,
+    base::RunLoop* run_loop,
+    const Status& status,
+    scoped_ptr<base::Value> value,
+    const std::string& session_id) {

[stgao, 2014/07/01 19:02:27]

Seems some parameters are not used. Could they be removed?

[johnmoore, 2014/07/02 22:17:44]

They could be removed if I re-implemented ExecuteSessionCommand in my test
method, such that it didn't pass the extra values. I don't like that idea since
it kind of defeats the purpose of the test, which is to mimic actual program
execution as closely as possible. Alternatively I could use the extra parameters
(i.e. perform checks on them), but they really aren't part of my test, so if
they somehow cause the test to fail, it won't be as clear why this particular
test is failing. Neither solution seems ideal, but let me know if you'd still
like me to address this issue.

[stgao, 2014/07/11 21:36:06]

OK. That's fine.

+  ASSERT_EQ(kOk, status.code());
+  listener->VerifyNotCalled();
+  run_loop->Quit();
+}
+
+
+void VerifyListenerNotifiedBeforeCommandRun(
+    MockCommandListener* listener,
+    base::RunLoop* run_loop,
+    const Status& status,
+    scoped_ptr<base::Value> value,
+    const std::string& session_id) {
+  ASSERT_EQ(kOk, status.code());
+  listener->VerifyCalled();
+  run_loop->Quit();
+}
+
+}  // namespace
+
+TEST(CommandsTest, SessionNotifiedOfCommand) {
+  SessionThreadMap map;
+  linked_ptr<base::Thread> thread(new base::Thread("1"));
+  ASSERT_TRUE(thread->Start());
+  std::string id("id");
+  thread->message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&internal::CreateSessionOnSessionThreadForTesting, id));
+  map[id] = thread;
+
+  base::DictionaryValue params;
+  // |listener| is owned by and will be destroyed by |session|
+  MockCommandListener* listener = new MockCommandListener();

[stgao, 2014/07/01 19:02:27]

As we have a reference to this |listener|, we could verify whether it is called
or not here before and after an execution of cmd.

Could the test be simplified a little bit? Or I misunderstand something here?

If a function above is used as a command, please postfix it with "Command" like
"abcCommand".

[johnmoore, 2014/07/02 22:17:44]

I have fixed the command and callback method names.

Regarding your other question:

Before the first command is executed, the listener has not been added to
|session|, and cannot be added to it because we do not have access to |session|
directly from the test. 

So, we must add the listener to |session| within the command itself, which has
access to |session|.

Recall that BeforeCommand is called before a command is executed. So when we
execute the command to add the listener to |session|, |session|'s listeners
(there aren't any of them yet) have already been notified of BeforeCommand.

So now we need to execute a second command so that the listener we just added in
the previous command can be notified of BeforeCommand. This second command is
basically a no-op, and just quits the session as a formality. Then in the
callback for the second command, we make sure the listener was notified.

I could try to re-write the test so that we had access to |session| from within
the test method, but I think that would defeat the purpose of the test.

[stgao, 2014/07/11 21:36:06]

You are right. We can't do something like ErrorFindChildElement here.
Thanks for the detailed explanation.

+  SessionCommand cmd = base::Bind(
+      &ExecuteAddListenerToSession, listener);
+
+  base::MessageLoop loop;
+  base::RunLoop run_loop_addlistener;
+
+  // |CommandListener|s are notified immediately before commands are run.
+  // Here, the command adds |listener| to the session, so |listener|
+  // should not be notified since it will not have been added yet.
+  ExecuteSessionCommand(
+      &map,
+      "cmd",
+      cmd,
+      false,
+      params,
+      id,
+      base::Bind(&VerifyListenerNotNotifiedAfterCommandRun, listener,
+                 &run_loop_addlistener));
+
+  run_loop_addlistener.Run();
+
+  base::RunLoop run_loop_testlistener;
+  cmd = base::Bind(
+        &ExecuteTestListener);
+
+  // |listener| has been added to the session and should be notified of
+  // the ExecuteTestListener command.
+  ExecuteSessionCommand(
+      &map,
+      "cmd",
+      cmd,
+      false,
+      params,
+      id,
+      base::Bind(&VerifyListenerNotifiedBeforeCommandRun, listener,
+                 &run_loop_testlistener));
+
+  run_loop_testlistener.Run();
+}