1033362 - Implement basic mochitests for MozLoopAPI

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

We need basic mochitests for MozLoopAPI. I think I'm not too fussed about testing everything there - as some of it is "straight through" call forwarding, but I think it is important to have tests for the more complex parts.

Comment 1

[Mark Banner (:standard8)]

Attachment: Implement basic mochitests for MozLoopAPI.

This implements basic mochitests for a couple of the simpler functions of MozLoopAPI, and in doing so proves out the test framework for head.js.

I choose the route of opening the panel as this a) tests that mozLoop is injected, b) seemed simpler than trying to inject mozLoopAPI into some random content window (I was having various issues with objects not having the wrapped versions available or maybe just not being wrapped).

Dan: can you check I've got the framing right in browser.ini? I thought that'd be a good a place as any to explain what we're using the mochitests for, although granted at some stage we might want to do an overview readme somewhere.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Added one comment suggesting a minor wording tweak; otherwise the framing looks great.  You might consider just putting it in a README, though, since it would never occur to me look in the browser ini file for context.

Comment 3

[Mark Banner (:standard8)]

Attachment: Implement basic mochitests for MozLoopAPI v2

Separated out the text into a readme file.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8450443 [details] [diff] [review]
Implement basic mochitests for MozLoopAPI v2

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

::: browser/components/loop/moz.build
@@ +11,5 @@
>  XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell/xpcshell.ini']
>  
> +BROWSER_CHROME_MANIFESTS += [
> +    'test/mochitest/browser.ini',
> +]

LGTM but needs review from a build peer.

::: browser/components/loop/test/mochitest/browser_mozLoop_charPref.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function generatorTest() {

Never heard of this before but it seems to be a thing. Can you please use the following to make it fit more with how we usually do it?

add_task(function* () {
  yield ...
});

@@ +9,5 @@
> +  Assert.ok(mozLoop, "mozLoop should exist");
> +
> +  // Test setLoopCharPref
> +
> +  mozLoop.setLoopCharPref("test", "foo");

Should use registerCleanupFunction() here to make sure we call clearUserPref().

@@ +17,5 @@
> +
> +  // Test getLoopCharPref
> +
> +  Assert.equal(mozLoop.getLoopCharPref("test"), "foo",
> +               "should get loop pref value correctly");

Just a general nit, leaving empty lines between every LOC looks weird :)

::: browser/components/loop/test/mochitest/browser_mozLoop_doNotDisturb.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function generatorTest() {

Same here about empty lines and add_task().

@@ +9,5 @@
> +  Assert.ok(mozLoop, "mozLoop should exist");
> +
> +  // Test doNotDisturb (getter)
> +
> +  Services.prefs.setBoolPref("loop.do_not_disturb", true);

Should use registerCleanupFunction() here to make sure we call clearUserPref().

::: browser/components/loop/test/mochitest/head.js
@@ +12,5 @@
> + * It also registers
> + *
> + * This assumes that the tests are running in a generatorTest.
> + */
> +function loadLoopPanel() {

This should return a promise rather than using the nextStep() magic. We could have some helper functions and let this be a task, that would look nicer.

@@ +53,5 @@
> + * This assumes that the panel has been opened, it closes it and
> + * then gets the mozLoop parameter.
> + */
> +function getMozLoopApiFromPanel() {
> +  EventUtils.sendKey("ESCAPE");

We always seem to call loadLoopPanel() and retrieve the API afterwards. Maybe this should just be one function? Splitting this into two is confusing especially with hitting the Esc key here at the beginning. Can we close the panel by calling panel.hide()/close() or something? EventUtils is quite flaky due to window focus so I'd rather avoid it if we don't need it.

Comment 5

[Mark Banner (:standard8)]

Attachment: Implement basic mochitests for MozLoopAPI v3

Updated for Tim's comments and latest code. As Tim is out, I'm wondering if Justin can review this. Also asking for build-config review as suggested.

Comment 6

[Mark Banner (:standard8)]

Forgot to say, try server build here: https://tbpl.mozilla.org/?tree=Try&rev=696dd3639b61

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 8453791 [details] [diff] [review]
Implement basic mochitests for MozLoopAPI v3

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

Please remove the README.txt. No one ever reads or updates files like these -- except for the occasional new contributor, who is promptly confused by content that is years out-of-date.

If you think the contents are important, roll it into the tests or the manifest where it stands a chance of being updated.

I just kind of skimmed the test since Tim already looked at it, reflag me if you wanted something specific reviewed.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8453791 [details] [diff] [review]
Implement basic mochitests for MozLoopAPI v3

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

::: browser/components/loop/test/mochitest/README.txt
@@ +1,1 @@
> +The main intent of these tests is to test the injection of mozLoop into navigator, and to test the more complex parts of the MozLoopAPI. There are some tests of the simpler functions which forward calls to MozLoopService and are also tested in xpcshell - these are primarily to prove the the basic injection and testing of MozLoopAPI.

Would be nicer to wrap the lines at 80 characters.

Comment 9

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbe70a2249c

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/3cbe70a2249c

Comment 11

[u279076]

Untracking for QA. Please needinfo me to request specific testing.