1446833 - Fix some common xpcshell harness annoyances

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, enhancement)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1446833: Part 1 - Stop throwing raw numeric error codes in xpcshell/head.js.

The xpcshell harness expects to catch the Cr result codes that it throws, but
it often fails when an assertion is used in asynchronous code. That results in
a raw numeric error code being reported, with no location information
available to figure out why or where the test is failing.

Throwing an actual Exception object maintains location information for use
when the error is reported.

Review commit: https://reviewboard.mozilla.org/r/228814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228814/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1446833: Part 2 - Stop using Task.jsm in xpcshell/head.js.

Review commit: https://reviewboard.mozilla.org/r/228816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228816/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1446833: Part 3 - Match more common head.js filename patterns when filtering assertion stack frames.

The xpcshell harness tries to filter out head.js stack frames when reporting
errors. When it fails, it tends to report strange error locations, with the
name of a unit test file, but the line number of a line in a head file. This
changes the filter to match more common head.js files, such as head_addons.js.

Review commit: https://reviewboard.mozilla.org/r/228818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228818/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

The directory service caches certain directory entries, or has them set by
other callers using `.set()`. When we try to override those directories with
custom providers, the cached value still takes precedence.

It might make more sense to just store the directory entry values directly in
the directory service's hash, but this patch just takes the less obtrusive
path of clearing cached values for keys that we override.

This patch also fixes the few instances where add-on manager tests leave files
in the global temporary directory which are now caught by the shutdown
assertions.

Review commit: https://reviewboard.mozilla.org/r/228820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228820/

Comment 5

[Florian Quèze [:florian]]

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.

Comment 6

[Florian Quèze [:florian]]

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?

Comment 7

[Florian Quèze [:florian]]

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 8

[Kris Maglione [:kmag]]

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 9

[Kris Maglione [:kmag]]

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 10

[Kris Maglione [:kmag]]

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 11

[Florian Quèze [:florian]]

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?

Comment 12

[Kris Maglione [:kmag]]

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 13

[Robert Helmer [: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

::: 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/

Comment 14

[Kris Maglione [:kmag]]

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 15

[Robert Helmer [:rhelmer]]

Comment on attachment 8960051 [details]
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

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

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8960051 [details]
Bug 1446833: Part 4 - Make sure directory service overrides actually take effect.

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

Comment 17

[Kris Maglione [:kmag]]

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

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a14c71ddab6fd13b16afc769efe1fc1c24f5a3
Bug 1446833: Follow-up: Fix intermittent Windows file locking xpcshell bustage. r=bustage DONTBUILD

Comment 19

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/bd915117e711
https://hg.mozilla.org/mozilla-central/rev/e2a78fa784c6
https://hg.mozilla.org/mozilla-central/rev/1f99f10985f5
https://hg.mozilla.org/mozilla-central/rev/fd8f5976ecbb
https://hg.mozilla.org/mozilla-central/rev/d9a14c71ddab