Last Comment Bug 638274 - re-enable tests disabled due to debug shell reftest assertion: !CompartmentHasLiveScripts(comp)
: re-enable tests disabled due to debug shell reftest assertion: !CompartmentHa...
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on: 634648
Blocks: 632343
  Show dependency treegraph
 
Reported: 2011-03-02 15:50 PST by Steve Fink [:sfink] [:s:]
Modified: 2011-05-23 17:55 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mark tests that need to run in debug mode (12.92 KB, patch)
2011-03-02 15:52 PST, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
Mark tests that need to run in debug mode (11.70 KB, patch)
2011-03-03 00:16 PST, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
Mark tests that need to run in debug mode (13.05 KB, patch)
2011-04-29 10:43 PDT, Steve Fink [:sfink] [:s:]
dbaron: review+
Details | Diff | Splinter Review
Mark tests that need to run in debug mode (15.92 KB, patch)
2011-04-29 17:08 PDT, Steve Fink [:sfink] [:s:]
sphink: review+
Details | Diff | Splinter Review
Mark tests that need to run in debug mode (16.04 KB, patch)
2011-04-29 17:34 PDT, Steve Fink [:sfink] [:s:]
sphink: review+
dmandelin: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2011-03-02 15:50:24 PST
Forked from bug 634648. We need to agree on a portable manifest syntax for tests that require debug mode to already be on, then I can implement it and re-enable 7 tests.
Comment 1 Steve Fink [:sfink] [:s:] 2011-03-02 15:52:35 PST
Created attachment 516438 [details] [diff] [review]
Mark tests that need to run in debug mode

Imported from bug 634648. Uses 'debug' on the manifest line to mark the tests that must be run in debug mode. I will update this patch to some other syntax before requesting review again.
Comment 2 Steve Fink [:sfink] [:s:] 2011-03-02 16:07:26 PST
(In reply to bug 634648 comment 17 from bclary)
> Comment on attachment 513040 [details] [diff] [review]
> Mark tests that need to run in debug mode
> 
> dbaron is reftest owner.
> 
> I'm concerned about overloading the keywords in this fashion to pass an
> argument to the test and in this case a single class of test (js shell) when
> the reftest framework is used in so many other testing areas. The choice of
> "debug" as a keyword seems problematic as well as it does not clearly imply the
> keyword is not for running tests in debug builds but for adding a specific
> option to the js shell to invoke the shell only function setDebug(). I would
> have preferred a more general approach of passing arguments rather than
> inventing a keyword for each possible argument that may be desired.

Given that this manifest is used by both the in-browser tests and the js shell tests, an option to add a command-line isn't right by itself (since browser command-line flags and js shell command line flags are totally different.) We could sort of make it work by having shell-only flags or something, and perhaps we should. But in this particular case, the semantics of what is necessary are portable even if we don't have the browser-side implementation in the test harness yet: these tests need debug mode to be on before they begin to run.
Comment 3 Steve Fink [:sfink] [:s:] 2011-03-03 00:16:28 PST
Created attachment 516530 [details] [diff] [review]
Mark tests that need to run in debug mode

Updated patch with one possible name: precondition(debugMode). It's a little weird because it tries to enforce the precondition, not just test it, but I couldn't think of a better name. Suggestions welcome.

For the js shell tests, precondition(debugMode) means "pass -d to the shell when running this test." For the browser tests, it means "skip this test because we haven't implemented turning on debug mode before a test."

I could imagine a precondition(alwaysMethodJIT) that passed -a to the shell, and set the appropriate preference in the browser.
Comment 4 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-04-22 13:09:46 PDT
So it's not clear to me that if this is a generic concept, 'skip' is always the right replacement.  Maybe we want something like require-or(precondition, otherwise), e.g., require-or(debugMode, skip)?
Comment 5 Steve Fink [:sfink] [:s:] 2011-04-22 13:26:39 PDT
Good point.

Let's see... what does it mean to hit the "false" condition? In this case, it means "the setup to make that precondition true is currently unimplemented". If that's always what it is, then I could see valid actions being "skip" or "todo". Do we support todo tests? I've paged this out. (To me, "todo" means "run the test anyway, it's ok if it fails, if it fails report that it passed unexpectedly.)

What should happen if the setup to make that precondition true is implemented but fails? Should that be an outright failure? Maybe that's not worth worrying about for now.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-04-22 14:17:24 PDT
we use "fails" rather than "todo" -- which means the test is expected to fail, and will report an unexpected pass if it doesn't.  Another option might be "random".
Comment 7 Steve Fink [:sfink] [:s:] 2011-04-22 14:28:28 PDT
Right. Saw that when I made a new patch with requires-or. Waiting for try to reopen before attaching it.

I should probably simplify it, though. Right now it allows

  require-or(debugMode & alwaysMethodJIT, fails)

with any number of &'s since I don't know if people like 'a & b' or 'a && b'.

And I'm not sure what it should do if you give it an unknown precondition -- is that an error, or should it use the 'or' action? If it's an error, then you always have to keep the JS and Python manifest readers in sync. I picked the 'or' action, but that loses some error checking.
Comment 8 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-04-22 14:32:15 PDT
Well, the toplevel parser doesn't even allow spaces inside (), since it tokenizes on space first.  (I've been meaning to fix that at some point.)

How about either "&&" or "," (list of conditions and then the last one is the "or").  I don't want &.
Comment 9 Steve Fink [:sfink] [:s:] 2011-04-29 10:43:56 PDT
Created attachment 529135 [details] [diff] [review]
Mark tests that need to run in debug mode

(In reply to comment #8)
> Well, the toplevel parser doesn't even allow spaces inside (), since it
> tokenizes on space first.  (I've been meaning to fix that at some point.)

Oh, right. Ok, I simplified my regexes to stop allowing impossible whitespace.

> How about either "&&" or "," (list of conditions and then the last one is the
> "or").  I don't want &.

I've updated the patch to require &&. I'd thought about "," originally, but I found it visually confusing. I'm somewhat tempted to use

  require(debugMode,patience)(skip)
  require(debugMode,patience)or(skip)
  require(debugMode,patience)-or-(skip)
  require(debugMode,patience)else(skip)

but it seemed excessive for such rarely used functionality.

I also fixed a line numbering bug in reftest.js that reported errors in the wrong place. I can split it out if you'd like.

This passed the try server.
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-04-29 14:25:04 PDT
Comment on attachment 529135 [details] [diff] [review]
Mark tests that need to run in debug mode

>+            } else if ((m = item.match(/^require-or\((.*?)\)$/))) {
>+                var [precondition_str, fallback_action] = m[1].split(/,/);

Could you check that there are exactly two pieces?

>+                var preconditions = precondition_str.split(/\&\&/)
>+                cond = false;
>+                for each (var precondition in preconditions) {
>+                    if (precondition === "debugMode") {
>+                        // Currently unimplemented. Requires asynchronous
>+                        // JSD call + getting an event while no JS is running
>+                        stat = fallback_action
>+                    } else {
>+                        // Unknown precondition. Assume it is unimplemented.
>+                        stat = fallback_action
>+                    }

I think both of these stat = fallback_action should be followed by "break;".  Also, please use the optional semicolon on both.

Please document this in layout/tools/reftest/README.txt

r=dbaron with that, though perhaps whoever owns the js test harness stuff should review that part as well
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-04-29 14:26:51 PDT
Also, you need to set cond to true either (a) where you set stat, or (b) unconditionally.

Could you also add some tests in layout/reftests/reftest-sanity/ ?
Comment 12 Steve Fink [:sfink] [:s:] 2011-04-29 17:08:41 PDT
Created attachment 529244 [details] [diff] [review]
Mark tests that need to run in debug mode

Updated patch since I'll be requesting JS review. Carrying over the r=dbaron.

> I think both of these stat = fallback_action should be followed by "break;". 
> Also, please use the optional semicolon on both.

But this is python! Oh. No, no it's not. (It's confusing working with a mixture of JS & Python.)

Done.

(In reply to comment #11)
> Also, you need to set cond to true either (a) where you set stat, or (b)
> unconditionally.

Doh! You are right.

> Could you also add some tests in layout/reftests/reftest-sanity/ ?

Good idea, since once I did they failed. Tests are so annoying that way. Added a "true" condition as well, to facilitate the tests.
Comment 13 Steve Fink [:sfink] [:s:] 2011-04-29 17:34:57 PDT
Created attachment 529249 [details] [diff] [review]
Mark tests that need to run in debug mode

Add testing syntax to js parser
Comment 14 Steve Fink [:sfink] [:s:] 2011-04-29 17:36:43 PDT
Comment on attachment 529249 [details] [diff] [review]
Mark tests that need to run in debug mode

dmandelin, this is the evolution of bug 634648 that you reviewed earlier. The syntax is somewhat generalized to handle things that we'll probably never use, but made more sense for the browser reftests.
Comment 15 David Mandelin [:dmandelin] 2011-05-10 15:43:12 PDT
(In reply to comment #14)
> Comment on attachment 529249 [details] [diff] [review] [review]
> Mark tests that need to run in debug mode
> 
> dmandelin, this is the evolution of bug 634648 that you reviewed earlier.
> The syntax is somewhat generalized to handle things that we'll probably
> never use, but made more sense for the browser reftests.

I'm almost done reviewing this. But first, have you run the manifest format changes past dbaron? I think these only get used on JS, so it shouldn't matter *too* much, but it would be good to fit in to the wider reftest style if possible.
Comment 16 Steve Fink [:sfink] [:s:] 2011-05-10 16:15:22 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Comment on attachment 529249 [details] [diff] [review] [review] [review]
> > Mark tests that need to run in debug mode
> > 
> > dmandelin, this is the evolution of bug 634648 that you reviewed earlier.
> > The syntax is somewhat generalized to handle things that we'll probably
> > never use, but made more sense for the browser reftests.
> 
> I'm almost done reviewing this. But first, have you run the manifest format
> changes past dbaron?

Yes, he mostly came up with the current syntax, and the patch is already r=dbaron. He pointed out that I should get the JS portion separately reviewed (see comment 10).
Comment 17 David Mandelin [:dmandelin] 2011-05-10 16:18:18 PDT
Comment on attachment 529249 [details] [diff] [review]
Mark tests that need to run in debug mode

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

::: js/src/tests/manifest.py
@@ +162,5 @@
>                      pos += 1
> +                elif parts[pos].startswith('require-or'):
> +                    cond = parts[pos][len('require-or('):-1]
> +                    (preconditions, fallback_action) = re.split(",", cond)
> +                    for precondition in re.split("\&\&", preconditions):

\s are not needed.

::: layout/tools/reftest/reftest.js
@@ +617,5 @@
> +                if (args.length != 2) {
> +                    throw "Error 7 in manifest file " + aURL.spec + " line " + lineNo + ": wrong number of args to require-or";
> +                }
> +                var [precondition_str, fallback_action] = args;
> +                var preconditions = precondition_str.split(/\&\&/);

'&' isn't a special character in JS regexps, so you don't need the \s.
Comment 18 Steve Fink [:sfink] [:s:] 2011-05-12 10:42:29 PDT
http://hg.mozilla.org/tracemonkey/rev/a5e2394cd75d
Comment 19 Steve Fink [:sfink] [:s:] 2011-05-12 14:46:11 PDT
backout: http://hg.mozilla.org/tracemonkey/rev/49173592f6df
Comment 20 Steve Fink [:sfink] [:s:] 2011-05-12 14:46:47 PDT
re-land: http://hg.mozilla.org/tracemonkey/rev/f07515fc3ebd

Note You need to log in before you can comment on or make changes to this bug.