All users were logged out of Bugzilla on October 13th, 2018

Expose assertion function in chrome script

RESOLVED FIXED in mozilla31

Status

RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Depends on: 1 bug)

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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!
(Assignee)

Comment 1

5 years ago
Created attachment 8367327 [details] [diff] [review]
Tweak error reporting

A small patch that isn't directly related, but improves loadChromeScript
error messages.
Attachment #8367327 - Flags: review?(ted)
(Assignee)

Comment 2

5 years ago
Created attachment 8367331 [details] [diff] [review]
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.
Attachment #8367331 - Flags: feedback?(ted)
(Assignee)

Updated

5 years ago
Blocks: 963239
Depends on: 914633
(Assignee)

Comment 4

5 years ago
(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 ?
(Assignee)

Comment 5

5 years ago
Created attachment 8367894 [details] [diff] [review]
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.
Attachment #8367331 - Attachment is obsolete: true
Attachment #8367331 - Flags: feedback?(ted)
Attachment #8367894 - Flags: review?(ted)
Attachment #8367894 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #8367894 - Flags: feedback?
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Comment 10

5 years ago
Created attachment 8374145 [details] [diff] [review]
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
Attachment #8367327 - Attachment is obsolete: true
Attachment #8367894 - Attachment is obsolete: true
Attachment #8374145 - Flags: review?(ted)
(Assignee)

Comment 11

5 years ago
Created attachment 8384885 [details] [diff] [review]
patch

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)
(Assignee)

Comment 12

5 years ago
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+
(Assignee)

Comment 14

5 years ago
Created attachment 8396299 [details] [diff] [review]
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
Attachment #8374136 - Attachment is obsolete: true
Attachment #8384885 - Attachment is obsolete: true
Attachment #8396299 - Flags: review+
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Belated <3 for this bug ;-)
You need to log in before you can comment on or make changes to this bug.