1014482 - Replace XPCShell-test assertion methods with Assert.jsm equivalents

Status: VERIFIED FIXED

(Testing :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

Since Assert.jsm has landed, it'd be nice to reduce code duplication due to various flavors of assertion methods defined by Mochi and XPCShell.

When function names between a test harness assertion and an Assert.jsm equivalent are incompatible, both will be kept to ensure backward compatibility.
If a mass-change to Assert.jsm assertions is preferred after all, a follow-up bug would be more appropriate.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: WIP: fully functional, needs chopping up

This WIP patch contains all the work that needs to be done here.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5fc1cc50139b

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: make Assert.jsm methods globally available and deprecate XPCShell-test's custom assert methods

Please see another attachment I posted with whitespace changes ignored.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1, ignore-whitespace.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: update XPCShell self-tests to be compatible with global Assert.jsm methods

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: initialize bookmarkData variable to prevent failing tests and other unexpected bugs

Steven, you introduced this code in bug 570266. Did you intentionally _not_ initialize it?

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: make existing tests compatible with global Assert.jsm methods

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8430026 [details] [diff] [review]
Patch 1: make Assert.jsm methods globally available and deprecate XPCShell-test's custom assert methods

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

\o/

Words cannot express how happy this patch makes me.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8430028 [details] [diff] [review]
Patch 2: update XPCShell self-tests to be compatible with global Assert.jsm methods

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

::: testing/xpcshell/example/unit/test_do_check_matches.js
@@ -32,5 @@
> -  // well, even if 'length' is non-enumerable in the pattern. So arrays
> -  // are useful as patterns.
> -  do_check_matches({0:0, 1:1, length:2}, [0,1]); // pass
> -  do_check_matches({0:1}, [1,2]);                // pass
> -  do_check_matches([0], {0:0, length:1});        // pass

I guess nobody was relying on this in practice? It would be nice to document that in the commit message.

Normally I'd withhold r+, for apparently losing test coverage, but since the tests all pass, you get a pass.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8430030 [details] [diff] [review]
Patch 3: initialize bookmarkData variable to prevent failing tests and other unexpected bugs

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

Trivial patch. Stealing review.

The variable in question isn't reference outside of the function, so there should be no scoping concern.

Comment 10

[Steven MacLeod [:smacleod]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8430030 [details] [diff] [review]
> Patch 3: initialize bookmarkData variable to prevent failing tests and other
> unexpected bugs
> 
> Review of attachment 8430030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Trivial patch. Stealing review.
> 
> The variable in question isn't reference outside of the function, so there
> should be no scoping concern.

Yeah, looks to me like it was just an error.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8430031 [details] [diff] [review]
Patch 4: make existing tests compatible with global Assert.jsm methods

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

::: toolkit/devtools/server/tests/unit/test_forwardingprefix.js
@@ +91,2 @@
>                        else
> +                        do_check_matches({ from: actor, error: 'noSuchActor', message: "No such actor for ID: " + actor }, aResponse);

You uncovered a buggy do_check_matches() implementation! More justification that Assert.jsm globally applied is a good idea.

::: toolkit/devtools/server/tests/unit/test_framebindings-06.js
@@ +20,4 @@
>    do_test_pending();
>  }
>  
>  function test_banana_environment()

banana environment?! sometimes it's best to not ask questions.

::: tools/profiler/tests/test_enterjit_osr.js
@@ +28,5 @@
>  	return p.getProfileData().threads[0].samples;
>      })();
>      do_check_neq(profile.length, 0);
>      let stack = profile[profile.length - 1].frames.map(f => f.location);
> +    stack = stack.slice(stack.lastIndexOf("js::RunScript") + 1);

I'm not sure what's going on here. I'd feel better if you got a devtools peer to sign off on this.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8430031 [details] [diff] [review]
Patch 4: make existing tests compatible with global Assert.jsm methods

(In reply to Gregory Szorc [:gps] from comment #11)
> ::: tools/profiler/tests/test_enterjit_osr.js
> @@ +28,5 @@
> >  	return p.getProfileData().threads[0].samples;
> >      })();
> >      do_check_neq(profile.length, 0);
> >      let stack = profile[profile.length - 1].frames.map(f => f.location);
> > +    stack = stack.slice(stack.lastIndexOf("js::RunScript") + 1);
> 
> I'm not sure what's going on here. I'd feel better if you got a devtools
> peer to sign off on this.

Jed, since you wrote the test in bug 914561, I'd like to ask you if this change is OK? It added another "js::RunScript" frame to the stack, because `do_check_neq()` now calls another function instead of having its own implementation. At least I *think* that's why the additional frame is there...

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8430031 [details] [diff] [review]
Patch 4: make existing tests compatible with global Assert.jsm methods

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

::: tools/profiler/tests/test_enterjit_osr.js
@@ +28,5 @@
>  	return p.getProfileData().threads[0].samples;
>      })();
>      do_check_neq(profile.length, 0);
>      let stack = profile[profile.length - 1].frames.map(f => f.location);
> +    stack = stack.slice(stack.lastIndexOf("js::RunScript") + 1);

If the test passes (and doesn't print "No JIT?" at line 38 on a platform where there's supposed to be a JIT) then this looks good.

But I also don't understand what's actually going on here — the extra "js::RunScript" would have to be pushed (and not popped) before we enter the IIFE for |arbitrary_name|.  So, naively, I'd expect it to be something that changed about how test scripts are run, not how |do_check_*| are implemented.  But maybe I'm missing something obvious.

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to Jed Davis [:jld] from comment #13)
> But I also don't understand what's actually going on here — the extra
> "js::RunScript" would have to be pushed (and not popped) before we enter the
> IIFE for |arbitrary_name|.  So, naively, I'd expect it to be something that
> changed about how test scripts are run, not how |do_check_*| are
> implemented.  But maybe I'm missing something obvious.

So this is what `frames` array looks like:

```
{"location":"(root)"}
{"location":"js::RunScript","line":0}
{"location":"js::RunScript","line":0}
{"location":"arbitrary_name (/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin13.1.0/_tests/xpcshell/tools/profiler/tests/test_enterjit_osr.js:20)","line":22}
{"location":"EnterJIT"}
{"location":"arbitrary_name (/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin13.1.0/_tests/xpcshell/tools/profiler/tests/test_enterjit_osr.js:20)","line":27}
```

So this looks exactly like I guessed in comment 12: like a stack trace, the list shows the last frame first (FIFO). `do_check_equal()` now invokes `Assert.equal()`, both located outside the compartment of this script, thus explaining the two 'js::RunScript' entries where there was just one before.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Gregory Szorc [:gps] from comment #7)
> \o/
> 
> Words cannot express how happy this patch makes me.

Excellent! It keeps surprising me how much some code can make people happy. :)

(In reply to Gregory Szorc [:gps] from comment #8)
> I guess nobody was relying on this in practice? It would be nice to document
> that in the commit message.

Yeah, it was a 'hidden 'feature'' of do_check_matches(). (Note the _double_ quoting there). In practice it was only used by 'toolkit/devtools/server/tests/unit/test_framebindings-06.js' (see attachment attachment 8430031 [details] [diff] [review]) to do perform fuzzy matching against a loosely defined dictionary.
What it did do in practice, however, was render that test completely unreadable.

On top of that, the test coverage of this 'feature' was spotty at most.

Comment 16

[Mike de Boer [:mikedeboer]]

To avoid a backout-dance, I pushed another cset to Try: https://tbpl.mozilla.org/?tree=Try&rev=4ee809ebbddb

Comment 17

[Mike de Boer [:mikedeboer]]

Marco, can you add this to the current iteration?

Comment 18

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3.

Can you recommend if this bug should be marked as [qa+] or [qa-] for QA verification.

Comment 19

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/47db23835754
https://hg.mozilla.org/integration/fx-team/rev/f3899da86410
https://hg.mozilla.org/integration/fx-team/rev/2abe332d428e
https://hg.mozilla.org/integration/fx-team/rev/2f439b950c81

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Mike de Boer [:mikedeboer] from comment #14)
> ```
> {"location":"(root)"}
> {"location":"js::RunScript","line":0}
> {"location":"js::RunScript","line":0}
> {"location":"arbitrary_name
> (/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin13.1.0/_tests/
> xpcshell/tools/profiler/tests/test_enterjit_osr.js:20)","line":22}
> {"location":"EnterJIT"}
> {"location":"arbitrary_name
> (/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin13.1.0/_tests/
> xpcshell/tools/profiler/tests/test_enterjit_osr.js:20)","line":27}
> ```
> 
> So this looks exactly like I guessed in comment 12: like a stack trace, the
> list shows the last frame first (FIFO). `do_check_equal()` now invokes
> `Assert.equal()`, both located outside the compartment of this script, thus
> explaining the two 'js::RunScript' entries where there was just one before.

Just to make sure we're on the same page: The innermost (leaf) function is last; the two js:RunScript frames are (indirectly) callers of arbitrary_name.  Maybe more importantly: That stack was recorded in an asynchronous signal handler while arbitrary_name was running, before the getProfileData() call.  So if the earlier call to do_check_true is somehow responsible for the second js::RunScript, then… why is it still there after that call returned?  That's the part I don't understand.

But as long as the OSR trampoline is still doing what it's supposed to — and it is — the test doesn't need to care about what's going on outside.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47db23835754
https://hg.mozilla.org/mozilla-central/rev/f3899da86410
https://hg.mozilla.org/mozilla-central/rev/2abe332d428e
https://hg.mozilla.org/mozilla-central/rev/2f439b950c81

Comment 22

[Mike de Boer [:mikedeboer]]

Updated the documentation at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests.

I'll send an email out to dev-platform today.

Comment 23

[Mike de Boer [:mikedeboer]]

dev-platform thread: https://groups.google.com/d/msg/mozilla.dev.platform/vdyLJBJTdYY/KudBBp8oaD4J

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8430026 [details] [diff] [review]
Patch 1: make Assert.jsm methods globally available and deprecate XPCShell-test's custom assert methods

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

::: testing/xpcshell/head.js
@@ +770,5 @@
>    _do_check_eq(left, right, stack, true);
>  }
>  
>  function do_check_true(condition, stack) {
> +  Assert.ok(condition);

We lost the "stack" parameter here and a few other places in this patch. Is this intended? I don't know if the extra parameter to xpcshell's assert functions are used particularly often (or profitably), but it is used, and seems like someone will encounter this someday and get frustrated their diagnostic message doesn't end up in the log.