Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780873003:
Add a browser test for the chrome://site-engagement WebUI. (Closed)
DescriptionAdd a "smoke test" for the chrome://site-engagement WebUI.
This test just verifies that if a score is injected for a single URL then
that URL will appear in the site engagement WebUI, to protect against
changes which break some part of the pipeline from the scoring service
to the UI.
BUG=703848
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2780873003
Cr-Commit-Position: refs/heads/master@{#462723}
Committed: https://chromium.googlesource.com/chromium/src/+/367ede2073f00b84cc50c43f9282c65a79bf2d0a
Patch Set 1 #
Total comments: 1
Patch Set 2 : Tidy up test impl #
Total comments: 11
Patch Set 3 : Address review comments #
Total comments: 11
Patch Set 4 : Switch to Promise #
Total comments: 2
Patch Set 5 : Address review comments #Patch Set 6 : Address review comments #
Dependent Patchsets: Messages
Total messages: 40 (28 generated)
Description was changed from ========== Add a browser test for the chrome://site-engagement WebUI. BUG=703848 ========== to ========== Add a browser test for the chrome://site-engagement WebUI. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by wez@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...
wez@chromium.org changed reviewers: + dominickn@chromium.org
PTAL; ugly first draft of a basic browser test. - Introduce a "page populated" callback for tests to set. - Use the callback to know when the page has been populated with site engagement data. - Examine the data to verify the expected fields are there. This is fragile in that it relies on specifics of the page layout, but with some refactoring (e.g. moving origin-cell class name to a constant & moving bits into helper functions on a test base) it can be made relatively easy to update if the page impl gets changed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
This approach seems fine to me. We should hopefully not change the layout too often, and having the table cell layout in a constant that vaguely resembles the layout in the webui.js should make this easy enough to keep updated. Thanks for this! https://codereview.chromium.org/2780873003/diff/1/chrome/browser/ui/webui/eng... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/1/chrome/browser/ui/webui/eng... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 for a new file? :)
The CQ bit was checked by wez@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...
OK, I think it's best to stick with this smoke-test for now; writing something more complete will take a bit more time and I'd like to get back to the changes SiteEngagementScore etc changes. :)
lgtm % nits. Thanks, I think this will work quite well! https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:7: #include "chrome/browser/ui/browser_commands.h" Not sure you need this one? https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:20: const GURL kExampleUrl = GURL("https://www.example.com/"); I'm surprised this doesn't fail some sort of static initialization check... can you just inline it in the test body? https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:32: void ResetBaseScore(GURL url, double score) { Nit: const GURL& https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:35: void ResetBaseScoreToMax(GURL url) { Nit: const GURL& https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:69: void ExpectPageContainsUrl(GURL url) { Nit: const GURL&
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 wez@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...
Description was changed from ========== Add a browser test for the chrome://site-engagement WebUI. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a "smoke test" for the chrome://site-engagement WebUI. This test just verifies that if a score is injected for a single URL then that URL will appear in the site engagement WebUI, to protect against changes which break some part of the pipeline from the scoring service to the UI. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
wez@chromium.org changed reviewers: + bauerb@chromium.org
bauerb: OWNERS for c/b/resources and c/b/ui/webui, plz! calamity: FYI, ugly C++ browser-test - feel free to replace with lean, mean JS. :) https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:7: #include "chrome/browser/ui/browser_commands.h" On 2017/03/30 06:05:24, dominickn wrote: > Not sure you need this one? You're right, I think; I'd left this because of the Navigate... but that's the ui_test_utils' version so we don't need this header as well. https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:20: const GURL kExampleUrl = GURL("https://www.example.com/"); On 2017/03/30 06:05:24, dominickn wrote: > I'm surprised this doesn't fail some sort of static initialization check... can > you just inline it in the test body? Static initializers are only checked on the perf bots, I think, and only for production binaries, not tests, I think. Rationale for putting it here was just that any tests added later should be able to use the same constants, e.g. the cell name constants would live up here as well. https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:32: void ResetBaseScore(GURL url, double score) { On 2017/03/30 06:05:24, dominickn wrote: > Nit: const GURL& Done. https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:35: void ResetBaseScoreToMax(GURL url) { On 2017/03/30 06:05:24, dominickn wrote: > Nit: const GURL& Done. https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:69: void ExpectPageContainsUrl(GURL url) { On 2017/03/30 06:05:24, dominickn wrote: > Nit: const GURL& Done. https://codereview.chromium.org/2780873003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:69: void ExpectPageContainsUrl(GURL url) { On 2017/03/30 06:05:24, dominickn wrote: > Nit: const GURL& Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... File chrome/browser/resources/engagement/site_engagement.js (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:12: function setPagePopulatedCallbackForTest(callback) { This looks like you could use Promises (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects... https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:44: const GURL web_ui_url(std::string(content::kChromeUIScheme) + "://" + Can you use JoinString here as well? Just in terms of characters, saving two `std::string()` calls might already be worth it...
calamity@chromium.org changed reviewers: + calamity@chromium.org
https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... File chrome/browser/resources/engagement/site_engagement.js (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:12: function setPagePopulatedCallbackForTest(callback) { On 2017/03/31 13:04:59, Bernhard (slow until Apr 6) wrote: > This looks like you could use Promises > (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...) Indeed! I think something like var populatedResolve = null; var populatedPromise = new Promise(function(resolve, reject) { populatedResolve = resolve; }); function waitForPagePopulated() { return populatedPromise; } function notifyPagePopulated() { populatedResolve(); } would work well here. https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:180: notifyPagePopulated(); I think this should move to line 191 so that the page is considered populated exactly after the response from getSiteEngagementInfo() is received. https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:61: " window.domAutomationController.send(true);" And then here: window.waitForPagePopulated().then(() => { window.domAutomationController.send(true); });
The CQ bit was checked by wez@chromium.org to run a CQ dry run
https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... File chrome/browser/resources/engagement/site_engagement.js (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:12: function setPagePopulatedCallbackForTest(callback) { On 2017/04/03 04:38:52, calamity wrote: > On 2017/03/31 13:04:59, Bernhard (slow until Apr 6) wrote: > > This looks like you could use Promises > > > (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...) > > Indeed! > > I think something like > > var populatedResolve = null; > var populatedPromise = new Promise(function(resolve, reject) { > populatedResolve = resolve; > }); > > function waitForPagePopulated() { > return populatedPromise; > } > > function notifyPagePopulated() { > populatedResolve(); > } > > would work well here. Done. Note that the downside is that we're allocating a Promise in production that is only actually used for testing, which does not seem desirable - although it's only a single Promise, all these little bits of waste add up. https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:180: notifyPagePopulated(); On 2017/04/03 04:38:52, calamity wrote: > I think this should move to line 191 so that the page is considered populated > exactly after the response from getSiteEngagementInfo() is received. The browser test is actually verifying that there is a table element containing the expected data, so that we catch the case where something is broken between the response and the table being populated, so it needs to be signalled only after the table has been updated. https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:44: const GURL web_ui_url(std::string(content::kChromeUIScheme) + "://" + On 2017/03/31 13:05:00, Bernhard (slow until Apr 6) wrote: > Can you use JoinString here as well? Just in terms of characters, saving two > `std::string()` calls might already be worth it... Done. https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:61: " window.domAutomationController.send(true);" On 2017/04/03 04:38:52, calamity wrote: > And then here: > > window.waitForPagePopulated().then(() => { > window.domAutomationController.send(true); > }); Done.
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.
LGTM with a nit: https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... File chrome/browser/resources/engagement/site_engagement.js (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:12: function setPagePopulatedCallbackForTest(callback) { On 2017/04/05 20:49:37, Wez wrote: > On 2017/04/03 04:38:52, calamity wrote: > > On 2017/03/31 13:04:59, Bernhard (slow until Apr 6) wrote: > > > This looks like you could use Promises > > > > > > (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...) > > > > Indeed! > > > > I think something like > > > > var populatedResolve = null; > > var populatedPromise = new Promise(function(resolve, reject) { > > populatedResolve = resolve; > > }); > > > > function waitForPagePopulated() { > > return populatedPromise; > > } > > > > function notifyPagePopulated() { > > populatedResolve(); > > } > > > > would work well here. > > Done. > > Note that the downside is that we're allocating a Promise in production that is > only actually used for testing, which does not seem desirable - although it's > only a single Promise, all these little bits of waste add up. I personally don't think it's a big issue, but you could always create the promise lazily. https://codereview.chromium.org/2780873003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:61: "window.pageIsPopulatedPromiseForTest().then(function() {" Nit: I realize that technically the function does return a promise, but calamity's suggestion of waitForPagePopulated() does read more intuitive. And I would also use arrow notation for the function. (Potentially also in the JS file; it's not super common yet in our WebUI code, but it's definitely allowed, and it is more concise.)
The CQ bit was checked by wez@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...
FYI: I've made the suggested changes and re-named things, so will land since those were nits - I'll CQ it now but please reply here or comment on the CL if you would like any other tweaks. https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... File chrome/browser/resources/engagement/site_engagement.js (right): https://codereview.chromium.org/2780873003/diff/40001/chrome/browser/resource... chrome/browser/resources/engagement/site_engagement.js:12: function setPagePopulatedCallbackForTest(callback) { On 2017/04/06 15:20:47, Bernhard Bauer wrote: > On 2017/04/05 20:49:37, Wez wrote: > > On 2017/04/03 04:38:52, calamity wrote: > > > On 2017/03/31 13:04:59, Bernhard (slow until Apr 6) wrote: > > > > This looks like you could use Promises > > > > > > > > > > (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...) > > > > > > Indeed! > > > > > > I think something like > > > > > > var populatedResolve = null; > > > var populatedPromise = new Promise(function(resolve, reject) { > > > populatedResolve = resolve; > > > }); > > > > > > function waitForPagePopulated() { > > > return populatedPromise; > > > } > > > > > > function notifyPagePopulated() { > > > populatedResolve(); > > > } > > > > > > would work well here. > > > > Done. > > > > Note that the downside is that we're allocating a Promise in production that > is > > only actually used for testing, which does not seem desirable - although it's > > only a single Promise, all these little bits of waste add up. > > I personally don't think it's a big issue, but you could always create the > promise lazily. The problem is then that the page might have completed loading data before the test registers interest, so you end up replicating this old-style flag+callback logic. I'll stick with the always-created Promise. https://codereview.chromium.org/2780873003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2780873003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:61: "window.pageIsPopulatedPromiseForTest().then(function() {" On 2017/04/06 15:20:47, Bernhard Bauer wrote: > Nit: I realize that technically the function does return a promise, but > calamity's suggestion of waitForPagePopulated() does read more intuitive. Ah, I see your point; you want the call site to read naturally with the then() syntax. Calling the function whenPageIsPopulated() seems the most appropriate naming; wait...() implies that the function itself won't return until the page has been populated. I'd prefer to keep ForTest() as the suffix, since this is not intended for general use. I've also added Promise to the var name, to better disambiguate it from the getter - though perhaps we don't need the getter function at all, and can just let the test grab the Promise var directly - WDYT? > And I > would also use arrow notation for the function. (Potentially also in the JS > file; it's not super common yet in our WebUI code, but it's definitely allowed, > and it is more concise.) Done, here and in the JS.
The CQ bit was unchecked by wez@chromium.org
The CQ bit was checked by wez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2780873003/#ps100001 (title: "Address review comments")
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": 100001, "attempt_start_ts": 1491522497196810,
"parent_rev": "261a0b276f6d7725877f735cc516d95182e5e360", "commit_rev":
"367ede2073f00b84cc50c43f9282c65a79bf2d0a"}
Message was sent while issue was closed.
Description was changed from ========== Add a "smoke test" for the chrome://site-engagement WebUI. This test just verifies that if a score is injected for a single URL then that URL will appear in the site engagement WebUI, to protect against changes which break some part of the pipeline from the scoring service to the UI. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a "smoke test" for the chrome://site-engagement WebUI. This test just verifies that if a score is injected for a single URL then that URL will appear in the site engagement WebUI, to protect against changes which break some part of the pipeline from the scoring service to the UI. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2780873003 Cr-Commit-Position: refs/heads/master@{#462723} Committed: https://chromium.googlesource.com/chromium/src/+/367ede2073f00b84cc50c43f9282... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/367ede2073f00b84cc50c43f9282... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
