Closed Bug 1014482 Opened 7 years ago Closed 7 years ago

Replace XPCShell-test assertion methods with Assert.jsm equivalents

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla32

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Keywords: dev-doc-complete, Whiteboard: p=5 s=it-32c-31a-30b.3 [qa-])

Attachments

(5 files, 1 obsolete file)

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.
Flags: firefox-backlog+
This WIP patch contains all the work that needs to be done here.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5fc1cc50139b
Please see another attachment I posted with whitespace changes ignored.
Attachment #8429333 - Attachment is obsolete: true
Attachment #8430026 - Flags: review?(gps)
Steven, you introduced this code in bug 570266. Did you intentionally _not_ initialize it?
Attachment #8430030 - Flags: review?(smacleod)
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.
Attachment #8430026 - Flags: review?(gps) → review+
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.
Attachment #8430028 - Flags: review?(gps) → review+
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.
Attachment #8430030 - Flags: review?(smacleod) → review+
(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 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.
Attachment #8430031 - Flags: review?(gps) → review+
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...
Attachment #8430031 - Flags: feedback?(jld)
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.
Attachment #8430031 - Flags: feedback?(jld) → feedback+
(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.
(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.
To avoid a backout-dance, I pushed another cset to Try: https://tbpl.mozilla.org/?tree=Try&rev=4ee809ebbddb
Marco, can you add this to the current iteration?
Flags: needinfo?(mmucci)
Added to Iteration 32.3.

Can you recommend if this bug should be marked as [qa+] or [qa-] for QA verification.
Flags: needinfo?(mmucci)
Whiteboard: p=5 → p=5 s=it-32c-31a-30b.3 [qa?]
Whiteboard: p=5 s=it-32c-31a-30b.3 [qa?] → p=5 s=it-32c-31a-30b.3 [qa-]
Summary: Replace assertion methods with Assert.jsm equivalents → Replace XPCShell assertion methods with Assert.jsm equivalents
Summary: Replace XPCShell assertion methods with Assert.jsm equivalents → Replace XPCShell-test assertion methods with Assert.jsm equivalents
Blocks: 1018226
(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.
Status: RESOLVED → VERIFIED
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 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.
Blocks: 1036639
You need to log in before you can comment on or make changes to this bug.