Closed Bug 1070065 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
34 Sprint 3- 9/1
Iteration:
35.1
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: pkerr, Assigned: pkerr)

References

Details

(Whiteboard: p=2[loop-uplift][loop-inccall1])

Attachments

(1 file, 3 obsolete files)

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).
Flags: qe-verify-
Status: NEW → ASSIGNED
Depends on: 1065155
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.
Attachment #8492476 - Attachment is obsolete: true
whitespace fixes
Attachment #8492495 - Attachment is obsolete: true
Attachment #8492497 - Flags: review?(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.
Attachment #8492497 - Flags: review?(standard8) → review+
> 
> ::: 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.
(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.
OS: Mac OS X → All
Hardware: x86 → All
changes based on review feedback
Attachment #8492497 - Attachment is obsolete: true
Carrying forward standard8 r+
Keywords: checkin-needed
Whiteboard: p=2[loop-uplift][loop-inccall1]
https://hg.mozilla.org/mozilla-central/rev/786a047a934f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8493192 - Flags: approval-mozilla-aurora?
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.
Attachment #8493192 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.