Bug 1421992 (do_xpcshell_cleanup)

Convert some of the do_* helpers so that they will be unified with other test harnesses

RESOLVED FIXED in mozilla59

Status

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mak, Assigned: florian)

Tracking

Version 3
mozilla59
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 2 obsolete attachments)

5.32 MB, patch
Gijs
: review+
Details | Diff | Splinter Review
20.70 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
12.15 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
14.34 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.59 MB, patch
Gijs
: review+
Details | Diff | Splinter Review
59.11 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
798.94 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
56.83 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
Reporter

Description

2 years ago
We have various do_* helpers that are unique to xpcshell.
We started using new global helpers from Assert.jsm but a lot of tests still use the old methods, and that creates confusion in newcomers and contributors about what to use.

To quickly move towards unification of the helpers used in tests, I discussed with Florian a proposal to replace through scripts the following helpers:

"do_check_true("    => "Assert.ok("
"do_check_false("   => "Assert.ok(!"
"do_check_eq("      => "Assert.equal("
"do_check_neq("     => "Assert.notEqual("
"do_check_null("    => "Assert.strictEqual(null, "
"do_check_matches(" => "Assert.deepEqual("

Additionally the following helpers could be renamed:
"do_print("            => "info("
"do_register_cleanup(" => "registerCleanupFunction("

Finally, in a follow-up, the following could be changed:
"do_timeout(" => "setTimeout" (though note the inverted arguments)
Reporter

Comment 1

2 years ago
"do_check_false("   => "Assert.ok(!"
This is a particularly complex case due to constructs like "typeof, instanceof, in, ...", combined boolean conditions like "this && that" or even just "!!this".
Simple variables and function calls can be converted straight to "Assert.ok(!", while others could be converted to "Assert.equal(false, ".
Reporter

Updated

2 years ago
Alias: do_xpcshell_cleanup
Assignee

Comment 2

a year ago
Attachment #8937730 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

a year ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee

Comment 4

a year ago
Attachment #8937732 - Flags: review?(gijskruitbosch+bugs)

Comment 8

a year ago
Comment on attachment 8937731 [details] [diff] [review]
part 2 - cleanup patch to make tests pass after replacing do_check_*

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

::: js/xpconnect/tests/unit/component-blob.js
@@ +8,5 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
> +const Assert = {ok: function(cond, text) {

Do we not have method notation here? (ie { ok(cond, text) { ... } } )

Also, I would prefer:

const Assert = {
  ok: function(cond, text) {
  },
};

for legibility (given the weird double braces at the end in the current version)
Attachment #8937731 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 9

a year ago
Comment on attachment 8937732 [details] [diff] [review]
part 3 - remove obsolete do_check_* implementations

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

::: testing/xpcshell/selftest.py
@@ +942,5 @@
>          self.writeManifest(["test_add_test_simple.js"])
>  
>          self.assertTestResult(True, verbose=True)
>          self.assertInLog("true == true")
> +        self.assertNotInLog("[Assert.ok :")

I'm assuming you have checked that this stuff works correctly on infra also in terms of what gets printed on treeherder and so on, if any of the checks fail?
Attachment #8937732 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

a year ago
Attachment #8937733 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 10

a year ago
(In reply to Marco Bonardo [::mak] (Away 16 Dec - 2 Jan) from comment #0)

> "do_check_null("    => "Assert.strictEqual(null, "

strictEqual doesn't work here, because there are some do_check_null(0) or do_check_null(false) in the code that are expected to pass. I used Assert.equal to preserve the existing behavior.

> Additionally the following helpers could be renamed:
> "do_print("            => "info("
> "do_register_cleanup(" => "registerCleanupFunction("

I also renamed:
  do_execute_soon => executeSoon

> Finally, in a follow-up, the following could be changed:
> "do_timeout(" => "setTimeout" (though note the inverted arguments)

Not trying this one here :-). Will be for a follow-up indeed.

> "do_check_false("   => "Assert.ok(!"
> This is a particularly complex case due to constructs like "typeof,
> instanceof, in, ...", combined boolean conditions like "this && that" or
> even just "!!this".
> Simple variables and function calls can be converted straight to
> "Assert.ok(!", while others could be converted to "Assert.equal(false, ".

I did do_check_false(stuff) -> Assert.ok(!stuff) when stuff was a MemberExpression, a CallExpression or an Identifier.
I special cased the simple do_check_false(!stuff -> Assert.ok(stuff
The remaining cases got a do_check_false(stuff -> Assert.equal(false, stuff treatement. This didn't work for a few cases where the stuff that was meant to be falsy was actually null, because "false == null" evaluates to false. I cleaned up by hand these few cases.

The scripts I used are available at https://bitbucket.org/fqueze/xpcshell-rewrites/commits/d3b509b6d80d24c8e7154e699c120126220c4acb

They try to preserve indent (and seem to do a reasonably good job of it from what I've observed).

Try pushes:
All tests on Linux64: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8690319fed4098e84ac38adf923a6e6fb3c4558
xpcshell on all desktop platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e4d6d3a957f1cf20f19b3edba4713fe4ca31376 (looks like there are still a few failures :-().

Comment 11

a year ago
Comment on attachment 8937735 [details] [diff] [review]
part 6 - cleanup patch to make tests pass after removing obsolete xpcshell functions

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

::: devtools/server/tests/unit/test_addon_reload.js
@@ +5,5 @@
>  "use strict";
>  
>  const {AddonManager} = require("resource://gre/modules/AddonManager.jsm");
>  
> +add_task(startupAddonsManager);

Can you put the startup add-on manager changes in a separate cset so it's easier to narrow down blame later if this causes intermittents in some weird way? Like, I think this is strictly an improvement, but I can also see it causing minor issues later, and I'd like to be prepared for that type of thing.

::: toolkit/components/ctypes/tests/chrome/xpcshellTestHarnessAdaptor.js
@@ +25,5 @@
>  
> +var Assert = {
> +  notEqual(left, right, stack) {
> +    if (left == right) {
> +      var text = "do_check_neq failed";

Nit: fix this message please.

@@ +37,4 @@
>  
> +  equal(left, right, stack) {
> +    if (left != right) {
> +      var text = "do_check_eq failed";

Same.
Attachment #8937735 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

a year ago
Comment on attachment 8937734 [details] [diff] [review]
part 5 - script-generated patch to replace do_execute_soon, do_print and do_register_cleanup

rs=me, I reviewed and have some confidence in the script.

Also, I know they say naming is one of the hardest problems in CS, but I think indentation is underrated. Just saying.
Attachment #8937734 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 13

a year ago
Comment on attachment 8937730 [details] [diff] [review]
part 1 - script-generated patch to replace do_check_*

rs=me here too
Attachment #8937730 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 14

a year ago
As noted on IRC, please send an intent to unship.

I look forward to the additional patches for the remaining try orange.
Assignee

Comment 15

a year ago
Attaching separately for easier review, but I'll fold this into part 6 for landing.

Tests are now green on try:
xpcshell on desktop platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e075ee27e1779772e93ac282d8351f0a6de883e
xpcshell on android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4756269d85ec3ea866d629af4391e77bdbe4b4c9
all tests on linux64: https://treeherder.mozilla.org/#/jobs?repo=try&revision=168149124d610adbaacfa66165eb548c5f946b1b

Intent to remove post at https://groups.google.com/forum/#!topic/mozilla.dev.platform/IrzGkgr0xJk
Attachment #8937796 - Flags: review?(gijskruitbosch+bugs)

Comment 16

a year ago
Comment on attachment 8937796 [details] [diff] [review]
part 6bis - follow-up test fix

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

rs=me but please also get review from a devtools peer.

::: devtools/server/tests/unit/head_dbg.js
@@ -594,5 @@
>  StubTransport.prototype.close = function () {};
>  
> -function executeSoon(func) {
> -  Services.tm.dispatchToMainThread({
> -    run: DevToolsUtils.makeInfallible(func)

Uhhh... removing the makeInfallible bit seems like something that wants review from a devtools peer. Also, the different behaviour of dispatchToMainThread compared to what we do now might cause issues with tests...
Attachment #8937796 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Updated

a year ago
Attachment #8937735 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8937796 - Attachment is obsolete: true
Comment on attachment 8937814 [details] [diff] [review]
part 6 v2 - cleanup patch to make tests pass after removing obsolete xpcshell functions

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

(In reply to :Gijs from comment #16)
> Comment on attachment 8937796 [details] [diff] [review]
> part 6bis - follow-up test fix
> 
> Review of attachment 8937796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me but please also get review from a devtools peer.
> 
> ::: devtools/server/tests/unit/head_dbg.js
> @@ -594,5 @@
> >  StubTransport.prototype.close = function () {};
> >  
> > -function executeSoon(func) {
> > -  Services.tm.dispatchToMainThread({
> > -    run: DevToolsUtils.makeInfallible(func)
> 
> Uhhh... removing the makeInfallible bit seems like something that wants
> review from a devtools peer. Also, the different behaviour of
> dispatchToMainThread compared to what we do now might cause issues with
> tests...

Looks good to me.
do_execute_soon is also using dispatchToMainThread. You only renamed it right, you didn't changed its implementation from here?
https://searchfox.org/mozilla-central/source/testing/xpcshell/head.js#697-700

`makeInfallible` is kind of obsolete. We used to put that everywhere in order to get nice error reporting. But now that most exceptions, if not all, get full stack traces... this became useless.
Attachment #8937814 - Flags: review?(poirot.alex) → review+
BTW, thanks a lot for such cleanup! I still remember being very confused about that years ago...
Assignee

Comment 20

a year ago
(In reply to Alexandre Poirot [:ochameau] from comment #18)

> do_execute_soon is also using dispatchToMainThread. You only renamed it
> right, you didn't changed its implementation from here?
> https://searchfox.org/mozilla-central/source/testing/xpcshell/head.js#697-700

Right, it's just a rename to match the mochitest name. Thanks for the quick review!
Comment hidden (advocacy)
Assignee

Comment 22

a year ago
I'm providing the patches generated by my scripts for comm-central, but I have no intent to debug individual c-c tests that may start failing.

This is the biggest patch, and could land now (before bug 1421992 lands on mozilla-central).
Attachment #8937978 - Flags: review?(philipp)
Assignee

Comment 23

a year ago
Smaller script-generated patch. This one can't land before the renames land in m-c.
Attachment #8937979 - Flags: review?(philipp)
Comment on attachment 8937978 [details] [diff] [review]
script-generated patch to replace do_check_* functions

Brief glance suggests this is fine. Would be great to get a try run though, we'll fix the tests if there is an issue.
Attachment #8937978 - Flags: review?(philipp) → review+
Attachment #8937979 - Flags: review?(philipp) → review+

Updated

a year ago
Duplicate of this bug: 1426376

Comment 27

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/455230ec2c82
script-generated patch to replace do_check_* functions with their Assert.* equivalents in C-C. rs=philipp
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Comment 28

a year ago
No "leave-open" here, to the push closed the bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

a year ago
Status: REOPENED → ASSIGNED

Comment 29

a year ago
Pushed by florian@queze.net:
https://hg.mozilla.org/mozilla-central/rev/eeb99638dc7a
script-generated patch to replace do_check_* functions with their Assert.* equivalents, rs=Gijs.
https://hg.mozilla.org/mozilla-central/rev/59b1ac8778fc
hand written cleanup patch to make xpcshell tests pass after replacing do_check_* with Assert.*, r=Gijs.
https://hg.mozilla.org/mozilla-central/rev/294d79abd65f
Remove obsolete do_check_* implementations, r=Gijs.
https://hg.mozilla.org/mozilla-central/rev/5580c205d9c4
Rename do_execute_soon, do_print and do_register_cleanup to executeSoon, info and registerCleanupFunction to match mochitest names, r=Gijs.
https://hg.mozilla.org/mozilla-central/rev/f418166c70b8
script-generated patch to replace do_execute_soon, do_print and do_register_cleanup with executeSoon, info and registerCleanupFunction, rs=Gijs.
https://hg.mozilla.org/mozilla-central/rev/5b1fdaa14d35
Hand written cleanup patch to make tests pass after removing obsolete xpcshell functions. r=Gijs,ochameau a=Aryx

Comment 30

a year ago
I'll land the second C-C patch now.
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 31

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/977d47e829bd
script-generated patch to replace do_execute_soon, do_print and do_register_cleanup with executeSoon, info and registerCleanupFunction in C-C. rs=philipp
Assignee

Comment 32

a year ago
Before landing I ran the scripts again with mobile/android/tests/browser/robocop added to the exclusion list, as I realized that https://searchfox.org/mozilla-central/source/mobile/android/tests/browser/robocop/robocop_head.js is a fork of the xpcshell head.js file, but doesn't use Assert.jsm.
Apologies, but in general the MDN team is focusing their efforts on web platform docs going forward; we don't really have the resources to document mozilla internals and related topics any more.

I am always happy to help in a pinch, but this probably isn't going to get done any tine soon if you leave it to us. Just letting you know.
Keywords: dev-doc-needed

Comment 35

a year ago
I made a poor man's attempt at fixing docs. I can't find MDN macros to mark removal. :-\
(In reply to :Gijs from comment #35)
> I made a poor man's attempt at fixing docs. I can't find MDN macros to mark
> removal. :-\

This is fine. We don't really have a macro for removal — obsolete is probably the closest, but we are phasing out usage of the term as it confuses the heck out of people (e.g. what's the difference between deprecated and removed). I've just turned your note into a warning box so it stands out more.
You need to log in before you can comment on or make changes to this bug.