1070065 - Create xpcshell unit tests for reject/busy functionality in MozLoopService.jsm

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Paul Kerr [:pkerr]]

What I'd really also like is to get an xpcshell test to test the busy notifications. I think you should be able to do this by using a combination of what's in test_looppush_initialize and some of the other tests (e.g. test_loopservice_dnd).

Comment 1

[Paul Kerr [:pkerr]]

Attachment: MozLoopService.jsm xpcshell unit test for busy reject feature

WIP

Comment 2

[Paul Kerr [:pkerr]]

I made this patch dependent on bug 1065155 now since the logic to check both hawk session on an incoming notification needed to also be covered in the unit test. Instead of creating another unit test patch, I have moved the merge point to after 1065155 and 1032700.

Comment 3

[Paul Kerr [:pkerr]]

Attachment: MozLoopService.jsm xpcshell unit test for busy reject feature

Comment 4

[Paul Kerr [:pkerr]]

Attachment: MozLoopService.jsm xpcshell unit test for busy reject feature

whitespace fixes

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8492497 [details] [diff] [review]
MozLoopService.jsm xpcshell unit test for busy reject feature

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

This looks good, thanks. r=Standard8 with the comments addressed.

::: browser/components/loop/test/xpcshell/head.js
@@ -94,5 @@
>   * enables us to check parameters and return messages similar to the push
>   * server.
>   */
> -let MockWebSocketChannel = function(initRegStatus) {
> -  this.initRegStatus = initRegStatus;

I think you could drop the `if (this.initRegStatus) {...}` block in sendMsg as well, as it doesn't seem to be used.

::: browser/components/loop/test/xpcshell/test_loopservice_busy.js
@@ +6,5 @@
> +                                  "resource:///modules/Chat.jsm");
> +
> +let openChatOrig = Chat.open;
> +
> +const loopServiceModule = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});//FIXME(PRK) - is this stil needed?

No it isn't.

@@ +37,5 @@
> +    }, () => {
> +      do_throw("should have opened a chat window");
> +    });
> +
> +    waitForCondition(function() actionReceived).then(() => {

waitForCondition doesn't hold the code, so the waitForCondition opened and waitForCondition actionReceived are executed at the same time. Although its unlikely, due to the way the code currently works, this could mean a race condition if actionReceived is triggered/checked before opened - as the test would then start the next test.

I think it would be safer to put this second waitForCondition inside the "then" for opened.

@@ +55,5 @@
> +{
> +  setupFakeLoopServer();
> +
> +  let callsRespCount = 0;
> +  let callsResponses = [

I think it would be worth a comment here, that even indexes simulate the FxA call to /calls, and that odd ones simulate the Guest call to /calls.

Comment 6

[Paul Kerr [:pkerr]]

> 
> ::: browser/components/loop/test/xpcshell/head.js
> @@ -94,5 @@
> >   * enables us to check parameters and return messages similar to the push
> >   * server.
> >   */
> > -let MockWebSocketChannel = function(initRegStatus) {
> > -  this.initRegStatus = initRegStatus;
> 
> I think you could drop the `if (this.initRegStatus) {...}` block in sendMsg
> as well, as it doesn't seem to be used.
> 
This code is used in the test_looppush_initialize.js and is held in common in head.js.

Comment 7

[Mark Banner (:standard8)]

(In reply to Paul Kerr [:pkerr] from comment #6)
> > I think you could drop the `if (this.initRegStatus) {...}` block in sendMsg
> > as well, as it doesn't seem to be used.
> > 
> This code is used in the test_looppush_initialize.js and is held in common
> in head.js.

Yep, you're right, somehow I missed that.

Comment 8

[Paul Kerr [:pkerr]]

Attachment: MozLoopService.jsm xpcshell unit test for busy reject feature

changes based on review feedback

Comment 9

[Paul Kerr [:pkerr]]

Carrying forward standard8 r+

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/fx-team/rev/786a047a934f

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/786a047a934f

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8493192 [details] [diff] [review]
MozLoopService.jsm xpcshell unit test for busy reject feature

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7ae7b0e8de1

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8493192 [details] [diff] [review]
MozLoopService.jsm xpcshell unit test for busy reject feature

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.