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

chrome/browser/ui/search/local_ntp_browsertest.cc

Index: chrome/browser/ui/search/local_ntp_browsertest.cc
diff --git a/chrome/browser/ui/search/local_ntp_browsertest.cc b/chrome/browser/ui/search/local_ntp_browsertest.cc
index 97cd8e9c6bf80cda4b7c0dddd7605129a65e9849..15f9dfd85f37176c2ef740c0999c87f964b0c6d7 100644
--- a/chrome/browser/ui/search/local_ntp_browsertest.cc
+++ b/chrome/browser/ui/search/local_ntp_browsertest.cc
@@ -19,6 +19,7 @@
 #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"
@@ -135,6 +136,65 @@ IN_PROC_BROWSER_TEST_F(LocalNTPTest, FakeboxRedirectsToOmnibox) {
   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.