Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
673783003:
BrowserTest for delivering push message to service worker. (Closed)
|
Created:
6 years, 2 months ago by Michael van Ouwerkerk Modified:
6 years, 1 month ago CC:
chromium-reviews, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@PushIntegrationTest2 Project:
chromium Visibility:
Public. |
DescriptionBrowserTest for delivering push message to service worker.
BUG=350377
Committed: https://crrev.com/092dc8bd2de3286a5b13893493e173be2578110d
Cr-Commit-Position: refs/heads/master@{#301376}
Patch Set 1 #Patch Set 2 : Minor cleanups. #
Total comments: 4
Patch Set 3 : Use generic data requesting class in js. #
Total comments: 16
Patch Set 4 : Rebase, address review comments. #
Total comments: 6
Patch Set 5 : Break long line. #
Messages
Total messages: 19 (5 generated)
mvanouwerkerk@chromium.org changed reviewers: + peter@chromium.org
Peter: please take a look at the whole patch.
mvanouwerkerk@chromium.org changed required reviewers: + peter@chromium.org
mvanouwerkerk@chromium.org changed reviewers: + fgorski@chromium.org
mvanouwerkerk@chromium.org changed required reviewers: + fgorski@chromium.org
Filip: please review the gcm bits.
I am a little worried by the way the test is implemented. https://codereview.chromium.org/673783003/diff/20001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/20001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:56: if (!lastMessageFromServiceWorker && loops < 10) { Isn't there a way to implement asynchronous test case for web features/api? We use the following for extensions: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... and: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/673783003/diff/20001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:68: <h1>Push API Test</h1> What do yo need this for?
Thanks Filip! Not sure this is what you were looking for, but please take another look. https://codereview.chromium.org/673783003/diff/20001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/20001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:56: if (!lastMessageFromServiceWorker && loops < 10) { On 2014/10/23 16:29:51, fgorski wrote: > Isn't there a way to implement asynchronous test case for web features/api? > > We use the following for extensions: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... > > and: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Well, domAutomationController.send must be called in response to content::ExecuteScriptAndExtractString. That is an asynchronous mechanism already, it's why this test works at all, and why the promises are handled cleanly. But here we are waiting for a service worker to be started, and for the service worker to post the event data to the page. There are a few ticks involved in that, and the event was not dispatched in response to content::ExecuteScriptAndExtractString. The current pattern is a little odd, but not particularly slow. If you know of a better way, I'd be happy to use it. If not, let's not spend too much time on it. https://codereview.chromium.org/673783003/diff/20001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:68: <h1>Push API Test</h1> On 2014/10/23 16:29:51, fgorski wrote: > What do yo need this for? It's for manually stepping through the test. Some parts, like the service worker, are difficult to debug.
Filip: since you seemed concerned, I've added an generic js class for requesting data that may not be available yet.
https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/service_worker.js (right): https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/service_worker.js:5: this.onmessage = function(event) { This doesn't seem to be used for anything other than your manual testing? https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/service_worker.js:13: console.log(event.data); nit: are the console.log() calls here necessary for the test to pass? elsewhere too. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/service_worker.js:22: this.clients.getAll().then(function(clients) { |this| is a bit odd here -- normally |this| would refer to the function scope, but clients is defined on the global scope. Just remove |this.|? https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:56: this.requested = {}; I'd like to get rid of the maps here, and change AsyncData to only store a single value. This should be sufficient for these and future tests (the only two values used right now are "push" for the actual test and "message" for your debugging). Through offline discussion I get how this works, but it's not obvious why the request/received separation is needed when reading the code. This class can definitely use a more extensive comment on *why* it works :-). https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:96: getLastMessageFromServiceWorker()</button> While I'm not necessarily against adding some manual tools in a test, in this case they're here without any explanation of expected behavior at all (they'd only be usable to you). Remove them?
lgtm, but please address comments form Peter. Also, I like the solution to the polling loop that you proposed. I think the previous approach might have been prone to flakiness, and the new approach looks more solid. Having spent more time with your code, I was also able to understand how the test flows (through the content::ExecuteScriptAndExtractString) and I think that is what I was originally asking for. Thanks for your patience with my prior comment and taking a second stab at the problem above. I learned something. As for manual part. I think it should stay, but once Peter and others on the team feel comfortable about using it for debugging. Does that sound OK? https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:182: message.data = {{"data", "testdata"}}; Just noticed C++11. Nice. And since it is not banned, OK :) https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:96: getLastMessageFromServiceWorker()</button> On 2014/10/24 14:54:58, Peter Beverloo wrote: > While I'm not necessarily against adding some manual tools in a test, in this > case they're here without any explanation of expected behavior at all (they'd > only be usable to you). Remove them? +1. However, I think if you document it well, they could stay and be useful for manual testing. After all that stuff has to be kept someplace.
Update to one of my comments. https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:182: message.data = {{"data", "testdata"}}; On 2014/10/24 17:09:24, fgorski wrote: > Just noticed C++11. > Nice. And since it is not banned, OK :) although having second look at http://chromium-cpp.appspot.com/ It is not yet allowed. You should fix this line, I think.
Thanks for the reviews! Peter: please take another look. https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:182: message.data = {{"data", "testdata"}}; On 2014/10/24 17:09:24, fgorski wrote: > Just noticed C++11. > Nice. And since it is not banned, OK :) Acknowledged. https://codereview.chromium.org/673783003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:182: message.data = {{"data", "testdata"}}; On 2014/10/24 21:53:48, fgorski wrote: > On 2014/10/24 17:09:24, fgorski wrote: > > Just noticed C++11. > > Nice. And since it is not banned, OK :) > > although having second look at http://chromium-cpp.appspot.com/ > It is not yet allowed. You should fix this line, I think. Done. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/service_worker.js (right): https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/service_worker.js:5: this.onmessage = function(event) { On 2014/10/24 14:54:58, Peter Beverloo wrote: > This doesn't seem to be used for anything other than your manual testing? Deleted. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/service_worker.js:13: console.log(event.data); On 2014/10/24 14:54:58, Peter Beverloo wrote: > nit: are the console.log() calls here necessary for the test to pass? elsewhere > too. Deleted. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/service_worker.js:22: this.clients.getAll().then(function(clients) { On 2014/10/24 14:54:58, Peter Beverloo wrote: > |this| is a bit odd here -- normally |this| would refer to the function scope, > but clients is defined on the global scope. Just remove |this.|? Done. I think it's perfectly normal for the execution context to be the global scope, but, yeah, this was redundant here. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:56: this.requested = {}; On 2014/10/24 14:54:58, Peter Beverloo wrote: > I'd like to get rid of the maps here, and change AsyncData to only store a > single value. This should be sufficient for these and future tests (the only two > values used right now are "push" for the actual test and "message" for your > debugging). > > Through offline discussion I get how this works, but it's not obvious why the > request/received separation is needed when reading the code. This class can > definitely use a more extensive comment on *why* it works :-). Done. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:96: getLastMessageFromServiceWorker()</button> On 2014/10/24 14:54:58, Peter Beverloo wrote: > While I'm not necessarily against adding some manual tools in a test, in this > case they're here without any explanation of expected behavior at all (they'd > only be usable to you). Remove them? Done. https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:96: getLastMessageFromServiceWorker()</button> On 2014/10/24 17:09:24, fgorski wrote: > On 2014/10/24 14:54:58, Peter Beverloo wrote: > > While I'm not necessarily against adding some manual tools in a test, in this > > case they're here without any explanation of expected behavior at all (they'd > > only be usable to you). Remove them? > > +1. However, I think if you document it well, they could stay and be useful for > manual testing. After all that stuff has to be kept someplace. Deleted.
lgtm https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:34: if (this.data) { Maybe strict check on === null? Also, isn't the push event's data attribute defined as nullable, in which case this would be a perfectly valid value in case nothing was provided? https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:61: sendResultToTest(pushRegistration.pushEndpoint + ' - ' + pushRegistration.pushRegistrationId); nit: break the line. sorry for missing this! https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:66: function isControlled() { This is not being used.
Thanks! https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:34: if (this.data) { On 2014/10/27 15:20:49, Peter Beverloo wrote: > Maybe strict check on === null? Also, isn't the push event's data attribute > defined as nullable, in which case this would be a perfectly valid value in case > nothing was provided? Something for a later patch, perhaps? For its current purposes, the test does not use null payloads. https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:61: sendResultToTest(pushRegistration.pushEndpoint + ' - ' + pushRegistration.pushRegistrationId); On 2014/10/27 15:20:49, Peter Beverloo wrote: > nit: break the line. sorry for missing this! Done. https://codereview.chromium.org/673783003/diff/60001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:66: function isControlled() { On 2014/10/27 15:20:49, Peter Beverloo wrote: > This is not being used. It is, see push_messaging_browsertest.cc
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/673783003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/092dc8bd2de3286a5b13893493e173be2578110d Cr-Commit-Position: refs/heads/master@{#301376} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
