PushMessagingBrowserTest for registration success and failure

chrome/browser/services/gcm/push_messaging_browsertest.cc

+// Copyright 2014 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 <string>
+
+#include "base/command_line.h"
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/services/gcm/fake_gcm_profile_service.h"
+#include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
+#include "chrome/browser/services/gcm/push_messaging_application_id.h"
+#include "chrome/browser/services/gcm/push_messaging_constants.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/infobars/core/confirm_infobar_delegate.h"
+#include "components/infobars/core/infobar.h"
+#include "components/infobars/core/infobar_manager.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
+
+namespace {
+class InfoBarObserver : public infobars::InfoBarManager::Observer {
+ public:
+  InfoBarObserver(Browser* browser, bool accept)

[Peter Beverloo, 2014/10/20 12:45:22]

I don't really like the name of InfoBarObserver given that it also accepts or
cancels the permission request. Perhaps something like InfoBarPermissionGranter?

[Peter Beverloo, 2014/10/20 12:45:22]

Is it much more noise to pass in a WebContents* rather than a Browser*?

[Michael van Ouwerkerk, 2014/10/20 15:30:37]

InfoBarResponder then, as it can deny as well as grant.

[Michael van Ouwerkerk, 2014/10/20 15:30:37]

I think so.

[Peter Beverloo, 2014/10/20 16:35:52]

Acknowledged, that sounds better :-).

[Peter Beverloo, 2014/10/20 16:35:52]

Did you try this? The following does not look that much worse to me.

-----
  auto accepting_responder(browser()->tab_strip_model()->GetActiveWebContents(),
                           true);
-----

("I think so." is a very dismissive response.)

[Michael van Ouwerkerk, 2014/10/20 17:06:49]

I didn't mean to dismiss a serious concern, this looked like a minor nit to me,
sorry if it wasn't. I tried Peter, I do think it's needlessly verbose. The class
being instantiated is only used in this test, so passing in a Browser* is a
trivial requirement. Were it a general purpose class I would probably agree with
you.

Anyway, using auto does not work like that, you need to hint at the type a bit
more explicitly.

+      : infobar_service_(InfoBarService::FromWebContents(
+            browser->tab_strip_model()->GetActiveWebContents())),
+        accept_(accept) {
+    infobar_service_->AddObserver(this);
+  }
+
+  virtual ~InfoBarObserver() { infobar_service_->RemoveObserver(this); }
+
+  // infobars::InfoBarManager::Observer
+  virtual void OnInfoBarAdded(infobars::InfoBar* infobar) override {
+    ConfirmInfoBarDelegate* delegate =
+        infobar->delegate()->AsConfirmInfoBarDelegate();

[Peter Beverloo, 2014/10/20 12:45:22]

Can we check somehow if the infobar which you're dealing with here is the one
you're expecting it to be? Also, please add a DCHECK(delegate).

[Michael van Ouwerkerk, 2014/10/20 15:30:37]

That seems to require Run-Time Type Information. But, I've changed it to be a
one-shot, by removing itself when it does observe an infobar. This means that if
it somehow (not sure in what case) observes a different infobar than intended,
the intended infobar will not be handled and the test will fail.

[Peter Beverloo, 2014/10/20 16:35:52]

I was hoping there'd be some way to get the owner or some kind of non-textual,
consistent ID. If there isn't then that's OK.

[Michael van Ouwerkerk, 2014/10/20 17:06:49]

Acknowledged.

+    if (accept_)
+      delegate->Accept();
+    else
+      delegate->Cancel();
+  }
+
+ private:
+  InfoBarService* infobar_service_;
+  bool accept_;
+};
+}  // namespace
+
+class PushMessagingBrowserTest : public InProcessBrowserTest {
+ public:
+  PushMessagingBrowserTest() : gcm_service_(nullptr) {}
+  virtual ~PushMessagingBrowserTest() {}
+
+  // InProcessBrowserTest:
+  virtual void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(
+        switches::kEnableExperimentalWebPlatformFeatures);

[Peter Beverloo, 2014/10/20 12:45:22]

+ InProcessBrowserTest::SetUpCommandLine()

[Michael van Ouwerkerk, 2014/10/20 15:30:37]

Done.

+  }
+
+  // InProcessBrowserTest:
+  virtual void SetUp() override {
+    https_server_.reset(new net::SpawnedTestServer(
+        net::SpawnedTestServer::TYPE_HTTPS,
+        net::BaseTestServer::SSLOptions(
+            net::BaseTestServer::SSLOptions::CERT_OK),
+        base::FilePath(FILE_PATH_LITERAL("chrome/test/data/"))));
+    ASSERT_TRUE(https_server_->Start());
+
+    InProcessBrowserTest::SetUp();
+  }
+
+  // InProcessBrowserTest:
+  virtual void SetUpOnMainThread() override {
+    gcm_service_ = static_cast<gcm::FakeGCMProfileService*>(
+        gcm::GCMProfileServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+            browser()->profile(), &gcm::FakeGCMProfileService::Build));
+    gcm_service_->set_collect(true);
+
+    ui_test_utils::NavigateToURL(
+        browser(), https_server_->GetURL("files/push_messaging/test.html"));

[Peter Beverloo, 2014/10/20 12:45:22]

+ InProcessBrowserTest::SetUpOnMainThread()

[Michael van Ouwerkerk, 2014/10/20 15:30:37]

Done.

+  }
+
+  void RunScript(const std::string& script, std::string* result) {
+    ASSERT_TRUE(content::ExecuteScriptAndExtractString(

[Peter Beverloo, 2014/10/20 12:45:22]

This still has the problem that an ASSERT_*() in a called function will not
actually stop the test from running. At the very least, this should be an
EXPECT_TRUE(), but you could also opt to return the boolean instead and
ASSERT_TRUE on RunScript() in the test body.

[Michael van Ouwerkerk, 2014/10/20 15:30:37]

It should be an ASSERT, as it would not make sense to continue running the test
if js execution failed. Still, pulling it up to the test level makes sense, so I
did that.

[Peter Beverloo, 2014/10/20 16:35:52]

My point is that ASSERTs don't work in called methods :-).

[Michael van Ouwerkerk, 2014/10/20 17:06:49]

Acknowledged.

+        browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(),
+        script,
+        result));
+  }
+
+  void RegisterServiceWorker(std::string* result) {
+    RunScript("registerServiceWorker()", result);
+  }
+
+  void RegisterPush(std::string* result) {
+    RunScript("registerPush('1234567890')", result);
+  }
+
+ protected:
+  scoped_ptr<net::SpawnedTestServer> https_server_;
+  gcm::FakeGCMProfileService* gcm_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(PushMessagingBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterSuccess) {
+  std::string register_worker_result;
+  RegisterServiceWorker(&register_worker_result);
+  EXPECT_EQ("ok", register_worker_result);
+
+  InfoBarObserver accepting_observer(browser(), true);
+
+  std::string register_push_result;
+  RegisterPush(&register_push_result);
+  EXPECT_EQ(std::string(gcm::kPushMessagingEndpoint) + " - 1",
+            register_push_result);
+
+  gcm::PushMessagingApplicationId expected_id(https_server_->GetURL(""), 0L);
+  EXPECT_EQ(gcm_service_->last_registered_app_id(), expected_id.ToString());
+  EXPECT_EQ(gcm_service_->last_registered_sender_ids()[0], "1234567890");
+}
+
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, RegisterFailureNoPermission) {
+  std::string register_worker_result;
+  RegisterServiceWorker(&register_worker_result);
+  EXPECT_EQ("ok", register_worker_result);
+
+  InfoBarObserver cancelling_observer(browser(), false);
+
+  std::string register_push_result;
+  RegisterPush(&register_push_result);
+  EXPECT_EQ("AbortError - Registration failed - permission denied",
+            register_push_result);
+}