The mochitests rely on the production push server. This causes intermittent oranges due to flaky network connectivity (see bug 1206969), and perma-oranges when the server has issues (bug 1244692). While these can indicate problems, we should detect them separately. Forcing sheriffs to star these intermittents (or back out patches to no avail), particularly when they're not actionable, makes their job harder. Currently, a lot of supporting logic (listening for permission changes, reconnecting, and evicting subscriptions) lives in `PushService.jsm`, with the network calls contained in `PushService{Http2, WebSocket}.jsm`. We should add another backend for testing (`PushServiceTest.jsm`, maybe?). This backend could export helpers for simulating pushes. We'll also stand up a separate Jenkins job that runs the tests against a real push server. This way, we can catch integration failures.

Remind me to remove the rule that's currently marking them as tier-2 on treeherder (which was supposed to stop the fruitless backing out) when this is done.

(In reply to Kit Cambridge [:kitcambridge] from comment #0) > The mochitests rely on the production push server. This causes intermittent > oranges due to flaky network connectivity (see bug 1206969), and > perma-oranges when the server has issues (bug 1244692). While these can > indicate problems, we should detect them separately. Forcing sheriffs to > star these intermittents (or back out patches to no avail), particularly > when they're not actionable, makes their job harder. > > Currently, a lot of supporting logic (listening for permission changes, > reconnecting, and evicting subscriptions) lives in `PushService.jsm`, with > the network calls contained in `PushService{Http2, WebSocket}.jsm`. We > should add another backend for testing (`PushServiceTest.jsm`, maybe?). This > backend could export helpers for simulating pushes. > > We'll also stand up a separate Jenkins job that runs the tests against a > real push server. This way, we can catch integration failures. In regards to catching integration failures on STAGE: per follow-on conversation with :kitcambridge, rather than additionally setting-up mochitests to run against real servers, it might be easier to stand-up a simple Mocha test in services-qa-jenkins that can be pointed at either dev, stage or prod environment and run with greater frequency.

(Setting dom-noted to make this not continue to show up in my triage queries)

Some additional thoughts: None of the integration tests need Special Powers, so we can rewrite them as plain content tests. Richard pointed out we have existing infrastructure that can run these against Nightly builds. We can use Marionette to run these, since it handles spawning Firefox with different prefs, and integrates with Mocha. For the in-tree mochitests...we already have an HTTP/2 server, so we can mock a simple Web Push server instead of adding a separate test-only backend. I think I'll do that instead, so that we exercise more of the `PushService` machinery.

Review commit: https://reviewboard.mozilla.org/r/33343/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33343/

Review commit: https://reviewboard.mozilla.org/r/33345/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33345/

Review commit: https://reviewboard.mozilla.org/r/33347/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33347/

Comment on attachment 8715114 [details] MozReview Request: Bug 1244816 - Run the SPDY and H2 servers for mochitests. r?ted Ted, if you have cycles, I'd love your help getting this to a reviewable state. :-) For context, the Push mochitests currently use the live server. I'd like to switch these over to using a local one. It's fairly easy to mock an H2-based Web Push server, and I see we already launch SPDY and H2 servers for xpcshell tests. This patch does the minimum to make that work for mochitests. I know the paths need to be updated, etc., but is the general approach sensible?

https://reviewboard.mozilla.org/r/33345/#review30061 ::: testing/specialpowers/content/SpecialPowersObserverAPI.js:51 (Diff revision 1) > +function addCertOverride(host, port, bits) { Bug 846581, comment 3 says we should load the cert from a file instead of doing this. The H2 server uses the cert in `netwerk/test/unit/CA.cert.der`. Should we move it to a common directory (like `build/pgo/certs`) so that the Push tests can use it, too? That cert is also issued to `foo.example.com`, so I'm guessing we'll need to override the `network.dns.localDomains` pref.

Nicholas, any thoughts on comment 9? I see `netwerk/test/unit/test_http2.js` still uses a bad cert listener. Is that the best way to make requests to the moz-http2 server, or can we somehow import the cert?

I have added some xpcshell tests for webpush that use the http2 server that we were using only for testing http2 implementation. Then I was not sure if we should not separate them. We can easily start another http2 server that is only use for webpush testing. I am adding this question to Kit's question for Nick. I thing it would be good to separate them.

Questions answers inline below. In the future, when requesting review on networking code, please flag the module owner or a peer first, and we can delegate to a non-peer. For this one, though, I'll go ahead and delegate to Dragana, since you've already flagged her (and I trust her). (In reply to Kit Cambridge [:kitcambridge] from comment #10) > Nicholas, any thoughts on comment 9? I see `netwerk/test/unit/test_http2.js` > still uses a bad cert listener. Is that the best way to make requests to the > moz-http2 server, or can we somehow import the cert? Yes, this is still the proper way to do this (you'll note that bug 846581 hasn't been significantly touched in 3 years). (In reply to Dragana Damjanovic [:dragana] from comment #11) > I have added some xpcshell tests for webpush that use the http2 server that > we were using only for testing http2 implementation. Then I was not sure if > we should not separate them. We can easily start another http2 server that > is only use for webpush testing. I am adding this question to Kit's question > for Nick. > > I thing it would be good to separate them. Yes, please separate them. The h2 test server is *very* stateful, and it would be easy to get yourself into a bad state if you're not careful.

:kitcambridge - one thing I noticed when perusing your patches - you also start a SPDY server for mochitests, when (to my knowledge) you won't need SPDY for these tests. Please don't run the SPDY server if you don't have to (and don't add its server code into the mochitest package).

https://reviewboard.mozilla.org/r/33343/#review30163 I have a few things that I'd like to see fixed before this gets landed, but I think you're on the right track. I can't believe we've been running tests in production against an external server. That's just a recipe for self-inflicted pain! ::: testing/mochitest/moz.build:43 (Diff revision 1) > + '/testing/xpcshell/node-spdy', I hate copying files around like this, but this isn't really any worse than what we already have and hopefully chmanchester is going to make most of this go away. ::: testing/mochitest/runtests.py:1273 (Diff revision 1) > + def trySetupNode(self, options): I'd rather not have this code duplicated between the xpcshell and mochitest harnesses. Maybe we could shoehorn this into the mozhttpd module? https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozhttpd ::: testing/mochitest/runtests.py:2229 (Diff revision 1) > + self.trySetupNode(options) It would be nice to only start the server when we actually need it, but I guess we don't do that in the xpcshell harness either. ::: testing/mochitest/runtests.py (Diff revision 1) > - if mozinfo.info.get('buildapp') == 'mulet' or options.subsuite == 'push': I can't believe someone let dougt land this. :-P

Thanks, everyone! Clearing r? for now, until we figure out the story on Android.

If we're going to split the push server out of moz-http2...could we generate a new cert for it, and add it to the cert DB?

Unless there's some mochitest-specific db I'm unaware of, you're asking to add a root cert to the NSS store, which is both an onerous process, and a Bad Idea for a root that signs certs that are only designed to be used for testing.

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #17) > Unless there's some mochitest-specific db I'm unaware of... That's the one I was asking about. It looks like `fillCertificateDB` might do that (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/testing/mochitest/runtests.py#1544-1606), and I found some certs in `build/pgo/certs`. But I don't really know how all these pieces fit together.

There's a script that generates the testing CA cert and server certs (for the hosts in server-locations.txt): https://dxr.mozilla.org/mozilla-central/source/build/pgo/genpgocert.py The resulting certs are checked in in the tree.

...and then fillCertificateDB puts them in the profile we use to run Mochitest, and the code that launches ssltunnel sets up its config file to use the right certs for the right hostnames.

Thanks, Ted! I was thinking of using a hostname like `push.example.com` for the cert, and setting `network.dns.localDomain` to `push.example.com`. But, as you pointed out in bug 1245596, that won't work on Android if the Node server is running on the host machine. I'm probably missing something...but, if not, `addCertOverride` should work just as well.

I do wish network.dns.localDomain was more flexible so we could use that instead of our proxy hack. If you need a cert to match the server URL then this gets a little more complicated, but if you can work around it then I guess just do that. You've got enough work to do here, I don't think you need to get into yak shaving if you can help it.

...alternately we can come up with some way to let ssltunnel do the SSL termination like we do for our existing HTTP server, and just have it forward connections to your push server, assuming that that doesn't muck things up.

ssltunnel does have support for proxying websocket connections to a separate server: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/testing/mochitest/ssltunnel/ssltunnel.cpp#830

Here is another approach that builds on top of our existing ability to mock the web socket of the web socket backend and doesn't require spinning up a server. This patch does the following: - Adds internal API to the push service to change the backend server (so we may replace it with a mock and change it back at the end of the test). - Factor out the encryption part of webpush in webpush.js - Create a content/chrome pair of classes for the web socket mock that handles communication between processes and allows the implementation to live in content process. I went ahead and converted test_data.html to use the new mock so that it doesn't make network requests to the push server. Let me know what you think of this approach, if it looks good to you I can go ahead and convert the rest and get rid of the remaining code that calls out to the push server.

Uploaded wrong file

Comment on attachment 8729714 [details] [diff] [review] Create PushService mock for mochitests backed by a mock web socket. Review of attachment 8729714 [details] [diff] [review]: ----------------------------------------------------------------- Approach looks good, and much better than a server. Thanks for doing this! ::: dom/push/PushService.jsm @@ +378,5 @@ > } > return [service, uri]; > }, > > + _changeServerURL: function(serverURI, event, options = {}) { Thanks for cleaning this up! ::: dom/push/test/test_data.html @@ +63,5 @@ > + mockSocket.onAck = function(request) { > + // We expect to get acks but we don't need to do anything special. > + }; > + > + setupMockPushService(mockSocket); Side note: I wonder if we'll want to disable "dom.push.connection.enabled" in testing/profiles/prefs_general.js, so that PushService doesn't try to connect with the default URL before the mock is installed. @@ +234,5 @@ > yield pushSubscription.unsubscribe(); > }); > > add_task(function* unregister() { > + teardownMockPushService(); I think we could do this in the cleanup function, in test_utils.js. Maybe we could pass the mock socket to "setPrefs", too, so that it installs the mock and enables the service in one go.

I've implemented your suggestions and converted the rest of the tests.

Comment on attachment 8730573 [details] [diff] [review] Create PushService mock for mochitests backed by a mock web socket. Review of attachment 8730573 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! I'm leery of test_multiple_register_during_service_activation.html, but maybe that's unfounded. ::: dom/push/PushServiceWebSocket.jsm @@ +1209,5 @@ > > let reply; > try { > reply = JSON.parse(message); > + console.log("message rv: " + message); Leftover debugging? ::: dom/push/test/test_data.html @@ +120,5 @@ > }); > > + var version = 0; > + function sendEncryptedMsg(pushSubscription, message) { > + webPushEncrypt(pushSubscription, message) return webPushEncrypt(...) ::: dom/push/test/test_multiple_register_during_service_activation.html @@ +57,5 @@ > > + function setupMultipleSubscriptions(swr) { > + // We need to do this to restart service so that a queue will be formed. > + teardownMockPushService(); > + setupMockPushService(new MockWebSocket()); This test concerns me because I think it could race: the `setup` message is sent synchronously, but `makeWebSocket` is called async, so mockpushserviceparent might try to call `mockWebSocket.close()` before the socket has been created. Hopefully, it's not an issue in practice, but it seems suspicious. I wonder what this is trying to test. Is it just making sure that calling `subscribe()` in parallel won't create multiple subscriptions for the same scope? Is that something we're concerned about? ::: dom/push/test/test_utils.js @@ +25,5 @@ > + function MockWebSocket() {} > + > + let registerCount = 0; > + > + // Default implementation to make the push server work minimally. Nice.

Comment on attachment 8730573 [details] [diff] [review] Create PushService mock for mochitests backed by a mock web socket. Review of attachment 8730573 [details] [diff] [review]: ----------------------------------------------------------------- Looking at this patch, I feel like it's not at the right level. This is mocking the Web Socket connection, which is better than touching production, but doesn't separate the transport layer from the "PushService" layer. I want tests (in particular test_data) to be unit tested at both levels. This is changing the transport layer tests but not separating the PushService layer tests. Does that make sense? Is there a plan to test the layers separately?

(In reply to Kit Cambridge [:kitcambridge] from comment #30) > Comment on attachment 8730573 [details] [diff] [review] > Create PushService mock for mochitests backed by a mock web socket. > > Review of attachment 8730573 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/push/test/test_multiple_register_during_service_activation.html > @@ +57,5 @@ > > > > + function setupMultipleSubscriptions(swr) { > > + // We need to do this to restart service so that a queue will be formed. > > + teardownMockPushService(); > > + setupMockPushService(new MockWebSocket()); > > This test concerns me because I think it could race: the `setup` message is > sent synchronously, but `makeWebSocket` is called async, so > mockpushserviceparent might try to call `mockWebSocket.close()` before the > socket has been created. Hopefully, it's not an issue in practice, but it > seems suspicious. > > I wonder what this is trying to test. Is it just making sure that calling > `subscribe()` in parallel won't create multiple subscriptions for the same > scope? Is that something we're concerned about? Good catch, I've updated the mock setup to make sure that the mock socket has been set up before using it. Based on the test file name, it looks like the test is trying to test registration when the push service is in the activating state which you can enter by changing the server URL. (In reply to Nick Alexander :nalexander from comment #31) > Comment on attachment 8730573 [details] [diff] [review] > Create PushService mock for mochitests backed by a mock web socket. > > Review of attachment 8730573 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looking at this patch, I feel like it's not at the right level. This is > mocking the Web Socket connection, which is better than touching production, > but doesn't separate the transport layer from the "PushService" layer. I > want tests (in particular test_data) to be unit tested at both levels. This > is changing the transport layer tests but not separating the PushService > layer tests. > > Does that make sense? Is there a plan to test the layers separately? I'm not aware of a plan to test the layers separately. If we do want to do that we probably need the interface between the PushService layer and the transport layer to be better defined as it's something that can easily change right now. One advantage of mocking out the web socket is that the protocol is well defined which helps with test stability.

A bit of a bumpy landing, but finally got it working on Beta as well. https://hg.mozilla.org/releases/mozilla-beta/rev/83043e8064e2 https://hg.mozilla.org/releases/mozilla-beta/rev/7f3c4ee12ee1