Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695813012:
[Local NTP] Add an integration test for the most visited iframe (Closed)
|
Created:
3 years, 10 months ago by Marc Treib Modified:
3 years, 10 months ago Reviewers:
sfiera CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Local NTP] Add an integration test for the most visited iframe
BUG=692002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2695813012
Cr-Commit-Position: refs/heads/master@{#451617}
Committed: https://chromium.googlesource.com/chromium/src/+/be5176536dd8dafc48fb719ad718a16d84caad0e
Patch Set 1 #
Total comments: 14
Patch Set 2 : review #Patch Set 3 : review 2 #Patch Set 4 : rebase #
Messages
Total messages: 29 (21 generated)
Description was changed from ========== [Local NTP] Add an integration test for the most visited iframe BUG=692002 ========== to ========== [Local NTP] Add an integration test for the most visited iframe BUG=692002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
treib@chromium.org changed reviewers: + sfiera@chromium.org
Behold my black JS testing magic! https://codereview.chromium.org/2695813012/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:655: iframe.src = 'chrome-search://most-visited/single.html?' + args.join('&'); This change is necessary for the fake testing NTP to load the iframe (it's hosted on a test server on https://). But I see no downside; AFAIK protocol-relative URLs are mostly considered a Bad Thing anyway. https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_test_utils.h (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_test_utils.h:55: bool GetBoolFromJS(const content::ToRenderFrameHost& adapter, This is a Special Hack to that it's possible to pass in either a WebContents or a RenderFrameHost.
https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_test_utils.h (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_test_utils.h:55: bool GetBoolFromJS(const content::ToRenderFrameHost& adapter, On 2017/02/17 17:25:31, Marc Treib wrote: > This is a Special Hack to that it's possible to pass in either a WebContents or > a RenderFrameHost. Nice implicit conversions. https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:166: // Find the RenderFrameHost of the iframe. Break out a "GetIframe()" helper function? https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:178: // their images successfully. I'm not sure I follow this set of conditions. https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:191: ASSERT_EQ(total_thumbs, succeeded_imgs + failed_imgs); This assert doesn't seem to add anything to the below checks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:166: // Find the RenderFrameHost of the iframe. On 2017/02/17 17:38:12, sfiera wrote: > Break out a "GetIframe()" helper function? Done. https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:178: // their images successfully. On 2017/02/17 17:38:12, sfiera wrote: > I'm not sure I follow this set of conditions. I've tried to make the comments clearer. Does this help? Otherwise let's maybe discuss in person. https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:191: ASSERT_EQ(total_thumbs, succeeded_imgs + failed_imgs); On 2017/02/17 17:38:12, sfiera wrote: > This assert doesn't seem to add anything to the below checks. 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?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:178: // their images successfully. On 2017/02/17 18:15:23, Marc Treib wrote: > On 2017/02/17 17:38:12, sfiera wrote: > > I'm not sure I follow this set of conditions. > > I've tried to make the comments clearer. Does this help? Otherwise let's maybe > discuss in person. 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? https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:191: ASSERT_EQ(total_thumbs, succeeded_imgs + failed_imgs); On 2017/02/17 18:15:23, Marc Treib wrote: > On 2017/02/17 17:38:12, sfiera wrote: > > This assert doesn't seem to add anything to the below checks. > > 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? 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).
https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:178: // their images successfully. On 2017/02/20 11:20:10, sfiera wrote: > On 2017/02/17 18:15:23, Marc Treib wrote: > > On 2017/02/17 17:38:12, sfiera wrote: > > > I'm not sure I follow this set of conditions. > > > > I've tried to make the comments clearer. Does this help? Otherwise let's maybe > > discuss in person. > > 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? 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? > - What guarantees that we have thumbnails? The baked-in sites should always have thumbnails, otherwise *something* is broken. https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:191: ASSERT_EQ(total_thumbs, succeeded_imgs + failed_imgs); On 2017/02/20 11:20:10, sfiera wrote: > On 2017/02/17 18:15:23, Marc Treib wrote: > > On 2017/02/17 17:38:12, sfiera wrote: > > > This assert doesn't seem to add anything to the below checks. > > > > 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? > > 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). 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.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2695813012/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:191: ASSERT_EQ(total_thumbs, succeeded_imgs + failed_imgs); On 2017/02/20 11:37:28, Marc Treib wrote: > On 2017/02/20 11:20:10, sfiera wrote: > > On 2017/02/17 18:15:23, Marc Treib wrote: > > > On 2017/02/17 17:38:12, sfiera wrote: > > > > This assert doesn't seem to add anything to the below checks. > > > > > > 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? > > > > 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). > > 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. 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) > I've made it an EXPECT instead of ASSERT so the below checks will still trigger, > and I've added a comment to explain. That's OK.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487598122472620,
"parent_rev": "25a3df81a5b7b5a8684e7f5c423fb86d0776d986", "commit_rev":
"be5176536dd8dafc48fb719ad718a16d84caad0e"}
Message was sent while issue was closed.
Description was changed from ========== [Local NTP] Add an integration test for the most visited iframe BUG=692002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Local NTP] Add an integration test for the most visited iframe BUG=692002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2695813012 Cr-Commit-Position: refs/heads/master@{#451617} Committed: https://chromium.googlesource.com/chromium/src/+/be5176536dd8dafc48fb719ad718... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/be5176536dd8dafc48fb719ad718... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
