Closed Bug 1446833 Opened 2 years ago Closed 2 years ago

Fix some common xpcshell harness annoyances

Categories

(Testing :: XPCShell Harness, enhancement)

enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(4 files)

There are a bunch of common annoyances I always run into when running xpcshell tests. I generally fix them locally and then throw the changes away, but they should really be fixed in the harness.
Comment on attachment 8960049 [details]
Bug 1446833: Part 2 - Stop using Task.jsm in xpcshell/head.js.

https://reviewboard.mozilla.org/r/228816/#review234596

How useful are the remaining _Task.Debugging uses? I'm looking forward to us completely eliminating Task.jsm!

Would be nice to do something similar for mochitests, but I'm afraid devtools tests still depend on using generators as tasks.
Attachment #8960049 - Flags: review?(florian) → review+
Comment on attachment 8960048 [details]
Bug 1446833: Part 1 - Stop throwing raw numeric error codes in xpcshell/head.js.

https://reviewboard.mozilla.org/r/228814/#review234600

Looks OK. Are the other 'throw Cr.' in our code not a problem? https://searchfox.org/mozilla-central/search?q=throw%20Cr.&path=

::: testing/xpcshell/head.js:41
(Diff revision 1)
>  // Register the testing-common resource protocol early, to have access to its
>  // modules.
> -var _Services = ChromeUtils.import("resource://gre/modules/Services.jsm", {}).Services;
> +var _Services = ChromeUtils.import("resource://gre/modules/Services.jsm", null).Services;
>  _register_modules_protocol_handler();
>  
>  var _PromiseTestUtils = ChromeUtils.import("resource://testing-common/PromiseTestUtils.jsm", {}).PromiseTestUtils;

Why is PromiseTestUtils treated differently than the other imports?
Attachment #8960048 - Flags: review?(florian) → review+
Comment on attachment 8960050 [details]
Bug 1446833: Part 3 - Match more common head.js filename patterns when filtering assertion stack frames.

https://reviewboard.mozilla.org/r/228818/#review234616

::: testing/xpcshell/head.js:787
(Diff revision 1)
>                       stack: _format_stack(ex.stack)
>                     });
>  }
>  
>  function do_report_result(passed, text, stack, todo) {
> -  while (stack.filename.includes("head.js") && stack.caller) {
> +  while (/(\/head(_.+)?|head)\.js$/.test(stack.filename) && stack.caller) {

This regexp seems more complicated than it needs to be. Is there a case where you need to match head.js$ and where matching /head.js$ isn't enough?
Comment on attachment 8960048 [details]
Bug 1446833: Part 1 - Stop throwing raw numeric error codes in xpcshell/head.js.

https://reviewboard.mozilla.org/r/228814/#review234600

Some of them are, but the xpcshell/head.js ones (and a few from httpd.js that I fixed in another bug) are the ones I run into most often, so I figured I'd restrict this bug to those.

> Why is PromiseTestUtils treated differently than the other imports?

Oops, I missed that one.
Comment on attachment 8960049 [details]
Bug 1446833: Part 2 - Stop using Task.jsm in xpcshell/head.js.

https://reviewboard.mozilla.org/r/228816/#review234596

Probably not very useful. I figured I'd leave it for the sake of other code in tree that was still using Task.jsm, but there's not very much of that left at this point.
Comment on attachment 8960050 [details]
Bug 1446833: Part 3 - Match more common head.js filename patterns when filtering assertion stack frames.

https://reviewboard.mozilla.org/r/228818/#review234616

> This regexp seems more complicated than it needs to be. Is there a case where you need to match head.js$ and where matching /head.js$ isn't enough?

There are still a few head files with names like foo_head.js, even though the more common pattern is head_foo.js these days.
Comment on attachment 8960050 [details]
Bug 1446833: Part 3 - Match more common head.js filename patterns when filtering assertion stack frames.

https://reviewboard.mozilla.org/r/228818/#review234726

::: testing/xpcshell/head.js:787
(Diff revision 1)
>                       stack: _format_stack(ex.stack)
>                     });
>  }
>  
>  function do_report_result(passed, text, stack, todo) {
> -  while (stack.filename.includes("head.js") && stack.caller) {
> +  while (/(\/head(_.+)?|head)\.js$/.test(stack.filename) && stack.caller) {

Ok, can you please add a comment explaining that we want to match head.js, head_foo.js and foo_head.js?
Attachment #8960050 - Flags: review?(florian) → review+
Fun fact. Using Task.spawn to spawn tasks had us silently ignoring the cases where the function we passed was not actually a function. E.g., the cases where people were doing [foo, bar].forEach(add_test). Which, of course, passes the index as a second argument to add_task, causing the index to be used as the test function, and the function to be treated as a task. Causing tests which were added that way to never actually run.
Comment on attachment 8960051 [details]
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

https://reviewboard.mozilla.org/r/228820/#review234750

::: testing/xpcshell/head.js:1113
(Diff revision 1)
>             .registerProvider(provider);
>  
> +  try {
> +    _Services.dirsvc.undefine("TmpD");
> +  } catch (e) {
> +    // This throws if the key is not already registers, but that

s/registers/registered/

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:308
(Diff revision 1)
> +        // FileTestUtils lazily creates a directory to hold the temporary files
> +        // it creates. If that directory exists, ignore it.
> +        let {value} = Object.getOwnPropertyDescriptor(FileTestUtils,
> +                                                      "_globalTemporaryDirectory");
> +        if (value) {
> +          ignoreEntries.add(value.leafName);

Hm, maybe I am misreading this, but is this creating a `Set` that only can ever have a single entry?

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1079
(Diff revision 1)
>      Services.dirsvc.registerProvider(dirProvider);
> +
> +    try {
> +      Services.dirsvc.undefine(key);
> +    } catch (e) {
> +      // This throws if the key is not already registers, but that

s/registers/registered/
Attachment #8960051 - Flags: review?(rhelmer)
Comment on attachment 8960051 [details]
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

https://reviewboard.mozilla.org/r/228820/#review234750

> Hm, maybe I am misreading this, but is this creating a `Set` that only can ever have a single entry?

At the moment, yes, but I was expecting that we'd run into more issues like this down the road, so having a Set rather than a single string would make it easier to update. I already had to hack around a problem with the built_in_addons.json overrides file when it was initially being added to the global tmp directory and conflicting with parallel runs by adding it to the generated XPIs list. Having a whitelist mechanism should really simplify those kinds of things.
Comment on attachment 8960051 [details]
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

https://reviewboard.mozilla.org/r/228820/#review234980
Attachment #8960051 - Flags: review+
Comment on attachment 8960051 [details]
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

https://reviewboard.mozilla.org/r/228820/#review234982
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd915117e71156d6518ee948f61105263dd5bb4a
Bug 1446833: Part 1 - Stop throwing raw numeric error codes in xpcshell/head.js. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a78fa784c69656b837b9034d008046576fa60f
Bug 1446833: Part 2 - Stop using Task.jsm in xpcshell/head.js. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f99f10985f541a9e192ffe3c59b06ef91dce1f1
Bug 1446833: Part 3 - Match more common head.js filename patterns when filtering assertion stack frames. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8f5976ecbb4b9ed119540bd95bed5824848074
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect. r=rhelmer
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a14c71ddab6fd13b16afc769efe1fc1c24f5a3
Bug 1446833: Follow-up: Fix intermittent Windows file locking xpcshell bustage. r=bustage DONTBUILD
Blocks: 1453881
You need to log in before you can comment on or make changes to this bug.