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.

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.

(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.

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.

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)?

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.

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".

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.

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 &.

(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 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

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

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.

Add testing syntax to js parser

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.

(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.

(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 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.

backout: http://hg.mozilla.org/tracemonkey/rev/49173592f6df

re-land: http://hg.mozilla.org/tracemonkey/rev/f07515fc3ebd