Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1985973003:
Adding score field into jsons in unittests for ntp_snippets_service. (Closed)
|
Created:
4 years, 7 months ago by jkrcal Modified:
4 years, 7 months ago Reviewers:
Marc Treib CC:
chromium-reviews, mcwilliams+watch_chromium.org, dgn+watch_chromium.org, mastiz Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding score field into jsons in unittests for ntp_snippets_service.
Somewhat related, refactoring of how snippet JSONs are generated.
Committed: https://crrev.com/5c8feb3c074b599d1ac53aea12a9809b00f571b8
Cr-Commit-Position: refs/heads/master@{#395053}
Patch Set 1 #Patch Set 2 : A minor fix #
Total comments: 11
Patch Set 3 : After code review #Patch Set 4 : Rebase #Patch Set 5 : Fixing a merging error #Patch Set 6 : Fixing a merge error #Patch Set 7 : Massaging a new unit test in the same style #Patch Set 8 : Fixing a unittest failure #
Total comments: 1
Patch Set 9 : Fixing a build failure #Patch Set 10 : After code review #2 #Messages
Total messages: 29 (13 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
PTAL
+mastiz FYI LGTM with some comments below. Thanks for the cleanup! https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:73: char json_str_format[] = "{ \"recos\": [ %s]}"; Might as well just inline this below (and remove the extra spaces inside). https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:78: std::string GetSnippetWithSources(const std::string& url, GetSnippetWithTimesAndSources? If we have "With..." pseudo-overloads, then we should be consistent in naming them. https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:131: std::string GetSnippet(const std::string& url, Also here: GetSnippetWithUrlAndTimes https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:136: std::vector<std::string>({kSnippetUrl}), Is the std::vector<std::string>() required here? https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:321: EXPECT_EQ(snippet.best_source().amp_url.spec(), GURL(kSnippetAmpUrl).spec()); I think the ".spec()" isn't required here - GURLs should be comparable?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985973003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985973003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985973003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985973003/60001
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 tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985973003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985973003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985973003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985973003/130001
Thanks, Marc. I had to rebase, fix a few things. On top of it, I changed a newly added unittest to conform with the rest of the file (probably should have resisted the temptation and should have deferred it to another CL). https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:73: char json_str_format[] = "{ \"recos\": [ %s]}"; On 2016/05/17 12:38:45, Marc Treib wrote: > Might as well just inline this below (and remove the extra spaces inside). Done. https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:78: std::string GetSnippetWithSources(const std::string& url, On 2016/05/17 12:38:45, Marc Treib wrote: > GetSnippetWithTimesAndSources? If we have "With..." pseudo-overloads, then we > should be consistent in naming them. Done. https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:131: std::string GetSnippet(const std::string& url, On 2016/05/17 12:38:45, Marc Treib wrote: > Also here: GetSnippetWithUrlAndTimes Done. https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:136: std::vector<std::string>({kSnippetUrl}), On 2016/05/17 12:38:45, Marc Treib wrote: > Is the std::vector<std::string>() required here? You are right, not really. Though it seems that at least std::string is needed inside. https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:321: EXPECT_EQ(snippet.best_source().amp_url.spec(), GURL(kSnippetAmpUrl).spec()); On 2016/05/17 12:38:45, Marc Treib wrote: > I think the ".spec()" isn't required here - GURLs should be comparable? Done.
LGTM, thanks! https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1985973003/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:136: std::vector<std::string>({kSnippetUrl}), On 2016/05/20 09:50:50, jkrcal wrote: > On 2016/05/17 12:38:45, Marc Treib wrote: > > Is the std::vector<std::string>() required here? > > You are right, not really. Though it seems that at least std::string is needed > inside. Makes sense - without that, you'll probably get a vector<const string&> which doesn't work at all, or a vector<const char*> which is not what you want. Interesting. https://codereview.chromium.org/1985973003/diff/130001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1985973003/diff/130001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:77: std::string GetSnippetWithUrlTimesAndSources( nit: "WithUrlTimes" sounds weird.. "WithUrlAndTimesAndSources"?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985973003/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985973003/170001
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 jkrcal@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/1985973003/#ps170001 (title: "After code review #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985973003/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985973003/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Adding score field into jsons in unittests for ntp_snippets_service. Somewhat related, refactoring of how snippet JSONs are generated. ========== to ========== Adding score field into jsons in unittests for ntp_snippets_service. Somewhat related, refactoring of how snippet JSONs are generated. Committed: https://crrev.com/5c8feb3c074b599d1ac53aea12a9809b00f571b8 Cr-Commit-Position: refs/heads/master@{#395053} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5c8feb3c074b599d1ac53aea12a9809b00f571b8 Cr-Commit-Position: refs/heads/master@{#395053} |
