Closed Bug 965257 Opened 8 years ago Closed 8 years ago

Expose assertion function in chrome script

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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!
Attached patch Tweak error reporting (obsolete) — Splinter Review
A small patch that isn't directly related, but improves loadChromeScript
error messages.
Attachment #8367327 - Flags: review?(ted)
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.
Attachment #8367331 - Flags: feedback?(ted)
Blocks: 963239
Depends on: 914633
(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 ?
Attached patch expose assert.jsm (obsolete) — Splinter Review
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.
Attachment #8367331 - Attachment is obsolete: true
Attachment #8367331 - Flags: feedback?(ted)
Attachment #8367894 - Flags: review?(ted)
Attachment #8367894 - Flags: feedback?
Attachment #8367894 - Flags: feedback?
review ping, may be someone with a smaller review queue can review these two patches?
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.
Attachment #8367327 - Flags: review?(ted)
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.
Attachment #8367894 - Flags: review?(ted) → review+
Assignee: nobody → poirot.alex
Attached patch interdiff (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8367327 - Attachment is obsolete: true
Attachment #8367894 - Attachment is obsolete: true
Attachment #8374145 - Flags: review?(ted)
Attached patch patch (obsolete) — Splinter Review
Review-ping.

Rebased patch, with another try run:
  https://tbpl.mozilla.org/?tree=Try&rev=5670f35f5879
Attachment #8374145 - Attachment is obsolete: true
Attachment #8374145 - Flags: review?(ted)
Attachment #8384885 - Flags: review?(ted)
Note that the failure on previous try is related to the test script of bug 963239.
So this patch itself is green.
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.
Attachment #8384885 - Flags: review?(ted) → review+
Attached patch patchSplinter Review
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
Attachment #8374136 - Attachment is obsolete: true
Attachment #8384885 - Attachment is obsolete: true
Attachment #8396299 - Flags: review+
Keywords: checkin-needed
I filed bug 987849 on supporting expected failure in Mochitest manifests.
https://hg.mozilla.org/mozilla-central/rev/cad7ebd98272
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Belated <3 for this bug ;-)
Depends on: 1436654
You need to log in before you can comment on or make changes to this bug.