Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485923002:
Add some more tests of PopularSites. (Closed)
|
Created:
4 years, 1 month ago by sfiera Modified:
4 years, 1 month ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd some more tests of PopularSites.
Committed: https://crrev.com/c49f0e4d6c1ff0776f244457f3693a289cc292ec
Cr-Commit-Position: refs/heads/master@{#430598}
Patch Set 1 #
Total comments: 9
Patch Set 2 : More tests broken down. #Patch Set 3 : Fix for compile error on some bots. #Messages
Total messages: 17 (11 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
The CQ bit was checked by sfiera@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 Nice! https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_unittest.cc (right): https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:92: {kTitle, "Wikipedia, fhta Ph'nglui mglw'nafh"}, I feel like I'm missing a reference here...? :D https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:146: std::vector<PopularSites::Site>* sites) { optional nit: You could return a base::Optional<vector<PopularSites::Site>> instead of using an output param? https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:178: base::FilePath cache_dir_; nit: cache_path_ ? https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:306: "https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json", {}); Is this actually needed? The same thing would happen if the fallback file didn't exist, right? (Actually even if it wasn't empty, the way the fallback works right now, correct?) https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:311: EXPECT_THAT(sites.size(), Eq(0u)); nit: IsEmpty?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_unittest.cc (right): https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:92: {kTitle, "Wikipedia, fhta Ph'nglui mglw'nafh"}, On 2016/11/08 11:27:30, Marc Treib wrote: > I feel like I'm missing a reference here...? :D I'm using the country code "ZZ" which is guaranteed not to be a real country (just as an extra precaution against using real data). Since it's not a real country, I grabbed some not-real-language from Lovecraft: …His worshippers chant "Ph'nglui mglw'nafh Cthulhu R'lyeh wgah'nagl fhtagn" ("In his house at R'lyeh, dead Cthulhu waits dreaming.")… (from the *English* Wikipedia) https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:178: base::FilePath cache_dir_; On 2016/11/08 11:27:30, Marc Treib wrote: > nit: cache_path_ ? I would say that the cache path is "/…whatever…/suggested_sites.json", whereas this is just the cache dir, the "/…whatever…/" part. https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:306: "https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json", {}); On 2016/11/08 11:27:30, Marc Treib wrote: > Is this actually needed? The same thing would happen if the fallback file didn't > exist, right? (Actually even if it wasn't empty, the way the fallback works > right now, correct?) Hmm. Well, the fetcher will hit the fallback URL here, so need a response (404 is fine, though). In this case, it's also interesting to verify that a cached fallback file doesn't prevent us from re-fetching the primary file, but I've now made that a separate test. And since caching an empty file is interesting too, that's now another test. https://codereview.chromium.org/2485923002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_unittest.cc:311: EXPECT_THAT(sites.size(), Eq(0u)); On 2016/11/08 11:27:30, Marc Treib wrote: > nit: IsEmpty? Done.
The CQ bit was checked by sfiera@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 sfiera@chromium.org
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2485923002/#ps40001 (title: "Fix for compile error on some bots.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add some more tests of PopularSites. ========== to ========== Add some more tests of PopularSites. Committed: https://crrev.com/c49f0e4d6c1ff0776f244457f3693a289cc292ec Cr-Commit-Position: refs/heads/master@{#430598} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c49f0e4d6c1ff0776f244457f3693a289cc292ec Cr-Commit-Position: refs/heads/master@{#430598} |
