965257 - Expose assertion function in chrome script

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Alexandre Poirot [:ochameau]]

Followup based on bug 914633 comment 7.
In 914633, we introduced SpecialPowers.loadChromeScript(), allowing to easily run some chrome code from a mochitest plain.
It has been originaly implement to prevent polluting SpecialPowers with growing numbers of domain-specific code.
But it is also used more and more to also check that the chrome code also behaves correctly. Having some assertiong method would be handy. Having the whole SimpleTest api would be even more usefull!

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Tweak error reporting

A small patch that isn't directly related, but improves loadChromeScript
error messages.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Expose SimpleTest to mochitest-plain chrome script

Here is a way to expose assertion function (and more) to chrome script.
It feels a bit hacky as the script sandbox isn't a mochitest document,
so there is no window/document/SpecialPowers/parentRunner.
It may be better to restrict to a very small subset what we expose from SimpleTest.
Or expose something brand new, with only ok/is...

You can find a test example using this approach, bug 963239 attachment 8367322 [details] [diff] [review].
If you like this path, I can start writing tests for it.

Comment 3

[(not currently active) Ted Mielczarek]

How about loading Assert.jsm in there? http://mxr.mozilla.org/mozilla-central/source/testing/modules/Assert.jsm

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> How about loading Assert.jsm in there?
> http://mxr.mozilla.org/mozilla-central/source/testing/modules/Assert.jsm

I went with SimpleTest to have same API than the mochitest.
But I feel quite bad with the hackiness of using it that way.
So Assert.jsm would work for me.

That would be really great if I can use Assert reporter function to pipe the assertion result back to the mochitest and get a similar assertion output.
I can send a message manager message from SpecialPowersObserverAPI.js to specialpowersAPI.js.
But I don't think I can easily retrieve a reference to the test runner from specialpowersAPI.js:SpecialPowers.loadChromeScript implementation ?

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: expose assert.jsm

Here is a similar patch, but now using Assert.jsm.
See attachment 8367893 [details] [diff] [review] for usage example.

If that works for you, I can try to write a test for that, and push to try.

Comment 6

[Alexandre Poirot [:ochameau]]

review ping, may be someone with a smaller review queue can review these two patches?

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8367327 [details] [diff] [review]
Tweak error reporting

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

This seems to have been rolled into the patch on bug 968857.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8367894 [details] [diff] [review]
expose assert.jsm

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

Sorry for the delay, this looks good.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +579,5 @@
> +        diagnostic +=
> +          " - got " + repr(err.actual) +
> +          ", expected " + repr(err.expected) +
> +          " (operator " + err.operator + ")";
> +      }

I wish you didn't have to do this. It would be nice if we could just pass the assertion up to the runner and let it deal with it. We can handle that in a followup though.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch

This patch brings tests, and fixes Assert.jsm loading on mochitest remote runs.
I tried really hard to make resource://testing-common/ to work in all cases,
but I haven't been able to make it work on Android.
So I removed the related code from the previous patch about tests.manifest,
and instead, I ship Assert.jsm in specialpowers modules.
It's weird as Assert.jsm is in /testing/modules, but it makes it way
easier to ensure it will work in remote scenario.

See attachment 8374136 [details] [diff] [review] for interdiff.

Try with another patch using assert:
https://tbpl.mozilla.org/?tree=Try&rev=95f4b11dfbd3

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: patch

Review-ping.

Rebased patch, with another try run:
  https://tbpl.mozilla.org/?tree=Try&rev=5670f35f5879

Comment 12

[Alexandre Poirot [:ochameau]]

Note that the failure on previous try is related to the test script of bug 963239.
So this patch itself is green.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8384885 [details] [diff] [review]
patch

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

Sorry for the incredibly long review delay, I just had a hard time finding the necessary time to keep this patch in my head.

::: testing/mochitest/tests/test_SpecialPowersLoadChromeScript.html
@@ +62,5 @@
> +  top.TestRunner = {
> +    log: checkTestRunnerCalls.bind(null, "log"),
> +    addFailedTest: checkTestRunnerCalls.bind(null, "addFailedTest"),
> +    error: checkTestRunnerCalls.bind(null, "error")
> +  };

I'm not really wild about this overriding TestRunner stuff. Can you just leave this out? You should be able to test valid assertions in the chrome script to sanity check that it works. In the near future we'll be able to mark mochitests as expected failure, then we could add a test for failing assertions.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: patch

Thanks for the review. No worry for the review delay, I got pulled with many other things as well.
I removed all failure assertions and kept only valid ones.
Is there a bug to track "expected failure" for mochitests?

https://tbpl.mozilla.org/?tree=Try&rev=4e12fa2c9fe1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/cad7ebd98272

Comment 16

[(not currently active) Ted Mielczarek]

I filed bug 987849 on supporting expected failure in Mochitest manifests.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/cad7ebd98272

Comment 18

[Mike de Boer [:mikedeboer]]

Belated <3 for this bug ;-)