[Local NTP] Add an integration test for the most visited iframe

chrome/browser/ui/search/local_ntp_browsertest.cc

 // Copyright 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 "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/search/instant_test_utils.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/omnibox/browser/omnibox_edit_model.h"
 #include "components/omnibox/browser/omnibox_view.h"
 #include "components/omnibox/common/omnibox_focus_state.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/vector2d.h"
 
 class LocalNTPTest : public InProcessBrowserTest,
                      public InstantTestBase {
  public:
@@ skipping 96 matching lines @@
   // The omnibox should have received visible focus.
   EXPECT_EQ(OMNIBOX_FOCUS_VISIBLE, omnibox()->model()->focus_state());
   // ...and the typed text should have arrived there.
   EXPECT_EQ("a", GetOmniboxText());
 
   // On the JS side, the fakebox should have been hidden.
   ASSERT_TRUE(GetBoolFromJS(active_tab, "!!fakeboxIsVisible()", &result));
   EXPECT_FALSE(result);
 }
 
+IN_PROC_BROWSER_TEST_F(LocalNTPTest, LoadsIframe) {
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
+  FocusOmnibox();
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), ntp_url(), WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB |
+          ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  content::WebContents* active_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(search::IsInstantNTP(active_tab));
+
+  content::DOMMessageQueue msg_queue;
+
+  bool result = false;
+  ASSERT_TRUE(GetBoolFromJS(active_tab, "!!setupAdvancedTest(true)", &result));
+  ASSERT_TRUE(result);
+
+  // Wait for the MV iframe to load.
+  std::string message;
+  // First get rid of the "true" message from the GetBoolFromJS call above.
+  msg_queue.PopMessage(&message);
+  ASSERT_EQ("true", message);
+  // Now wait for the "loaded" message.
+  msg_queue.WaitForMessage(&message);
+  ASSERT_EQ("\"loaded\"", message);
+
+  // Find the RenderFrameHost of the iframe.

[sfiera, 2017/02/17 17:38:12]

Break out a "GetIframe()" helper function?

[Marc Treib, 2017/02/17 18:15:23]

Done.

+  ASSERT_EQ(2, active_tab->GetAllFrames().size());
+  content::RenderFrameHost* iframe = nullptr;
+  for (content::RenderFrameHost* frame : active_tab->GetAllFrames()) {
+    if (frame != active_tab->GetMainFrame()) {
+      iframe = frame;
+      break;
+    }
+  }
+  CHECK(iframe);
+
+  // Check that there is at least one thumbnail, and that all of them loaded
+  // their images successfully.

[sfiera, 2017/02/17 17:38:12]

I'm not sure I follow this set of conditions.

[Marc Treib, 2017/02/17 18:15:23]

I've tried to make the comments clearer. Does this help? Otherwise let's maybe
discuss in person.

[sfiera, 2017/02/20 11:20:10]

I think I'm just not familiar enough with this test. The things I'm wondering
are:
- Why "at least one" tile? Where's it coming from? Why don't we know the real
number?
- What guarantees that we have thumbnails?

[Marc Treib, 2017/02/20 11:37:27]

The test runs in a non-signed-in, fresh profile, i.e. no history. That means
we'll get the two default TopSites tiles. testMostVisitedContents in the .js has
a comment about that; I've added one here too.

We do know the number, but it's not really relevant for this test, so I would
prefer not hardcoding it here. WDYT?
The baked-in sites should always have thumbnails, otherwise *something* is
broken.

+  int total_thumbs = 0;
+  ASSERT_TRUE(GetIntFromJS(
+      iframe, "document.querySelectorAll('.mv-thumb').length", &total_thumbs));
+  int succeeded_imgs = 0;
+  ASSERT_TRUE(GetIntFromJS(iframe,
+                           "document.querySelectorAll('.mv-thumb img').length",
+                           &succeeded_imgs));
+  int failed_imgs = 0;
+  ASSERT_TRUE(GetIntFromJS(
+      iframe, "document.querySelectorAll('.mv-thumb.failed-img').length",
+      &failed_imgs));
+
+  ASSERT_EQ(total_thumbs, succeeded_imgs + failed_imgs);

[sfiera, 2017/02/17 17:38:12]

This assert doesn't seem to add anything to the below checks.

[Marc Treib, 2017/02/17 18:15:23]

Well, true. It's supposed to be a sanity check - if, say, any of the css classes
were renamed, then IMO this assert failing is a bit clearer compared to some of
the below expectations failing.

In other words: This makes sure that the test's assumptions still hold. The
below are the actual test expectations.

Make sense?

[sfiera, 2017/02/20 11:20:10]

It still looks to me like it would make debugging harder, not easier. If this
assert fails, then the first thing I'll want to know is what succeeded_imgs and
failed_imgs are—but this assert serves to *hide* that information. Besides, if
.mv-thumb is renamed, the assert will trivially pass (0 == 0 + 0).

[Marc Treib, 2017/02/20 11:37:28]

But if e.g. only the ".failed-img" class is renamed, then one tile might fail to
load its thumbnail, and this test would never notice.

I've made it an EXPECT instead of ASSERT so the below checks will still trigger,
and I've added a comment to explain.

[sfiera, 2017/02/20 12:16:43]

The test would notice, because succeeded_imgs would no longer be equal to
total_thumbs. You're testing these three things:

x == y
x == y + z
z == 0

One of them is redundant. Any one of them can be derived from the other two. (x
= total_thumbs, y = succeeded_imgs, z = failed_imgs)
That's OK.

+
+  EXPECT_GT(total_thumbs, 0);
+  EXPECT_EQ(total_thumbs, succeeded_imgs);
+  EXPECT_EQ(0, failed_imgs);
+}
+
 IN_PROC_BROWSER_TEST_F(LocalNTPTest,
                        NTPRespectsBrowserLanguageSetting) {
   // Make sure the default language is not French.
   std::string default_locale = g_browser_process->GetApplicationLocale();
   EXPECT_NE("fr", default_locale);
 
   // Switch browser language to French.
   std::string loaded_locale =
       ui::ResourceBundle::GetSharedInstance().ReloadLocaleResources("fr");
 
@@ skipping 16 matching lines @@
       browser(), GURL(chrome::kChromeUINewTabURL),
       WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB |
           ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
 
   // Verify that the NTP is in French.
   content::WebContents* active_tab =
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_EQ(base::ASCIIToUTF16("Nouvel onglet"), active_tab->GetTitle());
 }