Closed Bug 899742 Opened 11 years ago Closed 9 years ago

Create some basic mochitests for the Push Notifications API

Categories

(Core :: DOM: Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: jsmith, Assigned: lina)

References

Details

Attachments

(1 file, 4 obsolete files)

In the original implementation in bug 822712, there was mochitests being developed, but we never finished getting them implemented (https://bugzilla.mozilla.org/attachment.cgi?id=725660&action=diff). We should finish off the work that was started there to get some basic smoke test coverage in the tree for the Push Notifications API.
Assignee: nobody → kcambridge
Blocks: 1129256
I can take this one...we're exposing a chrome-only Push API for Desktop in bug 1129256; it'll help to have test coverage for the parent `PushService.jsm` as part of that.
Status: NEW → ASSIGNED
Part one: a lot of code shuffling to extract socket creation, network info, and timers into separate objects and functions that can be mocked out (or overridden) by the tests. I tried to minimize the disruption, but there's still a lot of churn, compounded by my lackluster knowledge of Gecko idioms...suggestions for improvement are much appreciated. :-)

I haven't pushed to the try server yet...Nikhil, if you have some spare cycles, could you help me with getting try set up, please?
Attachment #8578362 - Flags: review?(nsm.nikhil)
Attached patch Add Push test mocks (obsolete) — Splinter Review
Part two: add mocks for preferences, sockets, network info, observer notifications, and storage. The fake clock is adapted from https://github.com/sinonjs/lolex, so that we can test request timeouts and reconnect alarms.
Attachment #8578365 - Flags: review?(nsm.nikhil)
Attached patch Add xpcshell-based tests (obsolete) — Splinter Review
Part three: the actual tests. I set up code coverage using https://github.com/gotwarlost/istanbul locally; it's reporting ~78% coverage.
Attachment #8578367 - Flags: review?(nsm.nikhil)
Attached patch Add xpcshell-based tests (obsolete) — Splinter Review
Attachment #8578367 - Attachment is obsolete: true
Attachment #8578367 - Flags: review?(nsm.nikhil)
Attachment #8578687 - Flags: review?(nsm.nikhil)
Comment on attachment 8578365 [details] [diff] [review]
Add Push test mocks

Review of attachment 8578365 [details] [diff] [review]:
-----------------------------------------------------------------

Please also rename the files to prefix Mock when they hold mock implementations.

::: dom/push/modules-testing/Services.jsm
@@ +19,5 @@
> +/**
> + * Creates an object that implements a subset of the nsIObserver interface.
> + * notifyObservers() dispatches notifications synchronously.
> + */
> +function MockObserverService() {

Replace with the standard observer service.

::: dom/push/modules-testing/Socket.jsm
@@ +75,5 @@
> +    return this._serverURI;
> +  },
> +
> +  get protocol() {
> +    return this._protocol;

if these getters/setters are only required to satisfy the interface constraints, i'd have them all throw NS_ERROR_NOT_IMPLEMENTED.

@@ +89,5 @@
> +    assert(uri.equals(this._serverURI), 'Wrong server URI');
> +    assert(this._serverURI.spec == origin, 'Wrong origin');
> +    this._listener = listener;
> +    this._context = context;
> +    this._listener.onStart(this._context);

This should be called async.

@@ +173,5 @@
> +  serverClose(statusCode, reason = '') {
> +    if (!isFinite(statusCode)) {
> +      statusCode = WEBSOCKET_CLOSE_GOING_AWAY;
> +    }
> +    this._listener.onServerClose(this._context, statusCode, reason);

make async. this and all other calls to the listener.

::: dom/push/modules-testing/Timer.jsm
@@ +15,5 @@
> + * and MockScheduler to simulate request and reconnect timeouts.
> + *
> + * MockClock is based on Lolex, copyright (c) 2010-2015 Christian Johansen
> + * <christian@cjohansen.no>. Lolex is distributed under the three-clause BSD
> + * license. <https://github.com/sinonjs/lolex>

I'm not sure what our policies are about having license clauses in code. Please ask bsmedberg or someone

::: dom/push/modules-testing/test/xpcshell/test_timer.js
@@ +7,5 @@
> +function run_test() {
> +  run_next_test();
> +}
> +
> +add_task(function* test_timer_unique_clock() {

don't need this test if you get rid of the mock.
Attachment #8578365 - Flags: review?(nsm.nikhil)
Hey Kit,

Thanks for the patch.  This is a lot of work and effort.  I am trying to understand the value/impact here.  In some ways not having any testing is horrible, right.  But I would much rather test against a real push server (either in releng infra, or the actual stage).  In this way, we test what we ship.

Can you help me understand the value in all of these moch* systems?  What do you think about just testing the system directly given the work:
    https://github.com/mozilla/gecko-dev/compare/master...dougt:dougt_push_api_hacking (See tests)
* Removed mocks for timers, alarms, preferences, and the observer service. Reduced request timeouts and reconnect delays to 1s and 150ms, respectively. I noticed shorter timeouts caused a slew of warnings (`WARNING: 'quotaClient->IsShuttingDown()', file /gecko-dev/dom/indexedDB/ActorsParent.cpp, line 11217` followed by `WARNING: 'NS_FAILED(rv)', file /gecko-dev/dom/indexedDB/ActorsParent.cpp, line 11150`) when the tests finished.
* Replaced `WaitGroup` with using deferreds directly.
* Removed `MockPushDB`, since we're already calling `do_get_profile` before running the tests, and `AlarmService` is backed by IndexedDB.
* Changed all `*SocketListener` calls to be async.
Attachment #8578362 - Attachment is obsolete: true
Attachment #8578365 - Attachment is obsolete: true
Attachment #8578687 - Attachment is obsolete: true
Attachment #8578362 - Flags: review?(nsm.nikhil)
Attachment #8578687 - Flags: review?(nsm.nikhil)
Attachment #8579463 - Flags: review?(nsm.nikhil)
Hi Doug!

(In reply to Doug Turner (:dougt) from comment #7)
> Can you help me understand the value in all of these moch* systems?  What do
> you think about just testing the system directly given the work:
>    
> https://github.com/mozilla/gecko-dev/compare/master...dougt:
> dougt_push_api_hacking (See tests)

Testing against a real server is definitely desirable. I think it's helpful to have some local tests to catch cases like request timeouts, client ACK behavior, and invalid responses. This was particularly useful when working on bugs 1134292 and 1129256.

We could certainly modify our current server to emit malformed responses, and report on client commands...but then we'd need to maintain a separate server fork, since we'd never want these hooks in the official binary. Depending on how much we'd like to test, it could diverge enough to become less of a "real" server. There's also the question of what happens if the test server is unreachable...either because you're working offline or on a slow connection, or the server is on fire.

Anyway, just some thoughts. You and Nikhil obviously have a lot more context around this, so I'll defer to whatever you think is best. We can talk more about this Friday, too.
Comment on attachment 8579463 [details] [diff] [review]
Add xpcshell-based tests

Review of attachment 8579463 [details] [diff] [review]:
-----------------------------------------------------------------

Conditional r+ with the following comments once the questions below are answered. Sorry for the delay!

::: dom/push/PushService.jsm
@@ +433,5 @@
>              let duration = Date.now() - this._pendingRequests[channelID].ctime;
>  
>              // If any of the registration requests time out, all the ones after it
>              // also made to fail, since we are going to be disconnecting the socket.
> +            if (requestTimedOut || duration >= this._requestTimeout) {

why >= instead of >?

@@ +622,5 @@
>      debug("init()");
>      if (!prefs.get("enabled"))
>          return null;
>  
> +    this._prefs = prefs;

Please move this up to closer to the prefs initialization (above the enabled check)

@@ +738,5 @@
> +
> +  _makeWebSocket: function(uri) {
> +    if (!this._prefs.get("connection.enabled")) {
> +      debug("_makeWebSocket: connection.enabled is not set to true. Aborting.");
> +      return;

return null; here and throughout this function.

@@ +1402,5 @@
>  
> +    // Cancel pending requests for this channel ID.
> +    let pendingRequest = this._pendingRequests[data.channelID];
> +    if (pendingRequest) {
> +      pendingRequest.deferred.reject({status: 0, error: "CancelledError"});

Does this propagate all the way to content? In that case, you want AbortError.

@@ +1900,5 @@
> +    if (statusCode != Cr.NS_OK &&
> +        !(statusCode == Cr.NS_BASE_STREAM_CLOSED && this._willBeWokenUpByUDP)) {
> +      debug("Socket error " + statusCode);
> +      this._reconnectAfterBackoff();
> +    }

Why is this block moved around? _reconnectAfterBackoff() is async and it is ok to call it before shutdownWS(). Unless there is a reason, this will just obfuscate future blames.

@@ +1939,5 @@
> +    this._setAlarm(this._prefs.get("pingInterval"));
> +
> +    if (this._onMessage) {
> +      this._onMessage(message);
> +    }

Please add a comment why this is being called when pings are explicitly ignored just below it.

::: dom/push/test/xpcshell/head.js
@@ +407,5 @@
> +  },
> +
> +  /**
> +   * Responds with the given message, calling onMessageAvailable() and
> +   * onAcknowledge() synchronously. Throws if the message is not a string.

comment does not reflect that onMessageAvailable is actually calls async.

@@ +416,5 @@
> +  serverSendMsg(msg) {
> +    assert(typeof msg == 'string', 'Unexpected message type');
> +    nextTick(
> +      () => this._listener.onMessageAvailable(this._context, msg),
> +      () => this._listener.onAcknowledge(this._context, 0)

onAcknowledge will be called on the next run of the event loop after onMessageAvailable since reduce in nextTick() gates each call on the Promise returned by the prior call. Is that ok? If so, nextTick() should really be renamed to something else, waterfall?

@@ +441,5 @@
> +};
> +
> +/**
> + * Creates a mock UDP socket that implements a subset of the nsIUDPSocket
> + * interface. All nsIUDPSocketListener methods will be called synchronously.

not being called sync, please update comment.

@@ +483,5 @@
> +   */
> +  serverPing() {
> +    // TODO: Pass an nsIUDPMessage instead of null. PushService does not
> +    // check the message contents, so null is sufficient.
> +    nextTick(() => this._listener.onPacketReceived(this, null));

if it is ok to pass null, do we need the TODO?
Attachment #8579463 - Flags: review?(nsm.nikhil) → review+
did this land?
Most of it landed in bug 1150683; the UDP tests will land in bug 1157696. The reconnect and adaptive ping tests are obsoleted by the switch to H/2 in bug 1150812. I think we can close this out.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: