525213 - gTestfile should not be set in individual js/tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

It would be less verbose and less error-prone to have each of the test runners populate gTestfile. A little grepping suggests that 2800 of 3100 tests have it, which is not so hot.

Stripping them out is the easy part:

find . -name '*.js' | xargs perl -pi -e 's/^(var )?gTestfile.*\n//;'

Making the test runners do the job instead is something I don't know much about. I don't even really know how many js/src/tests test runners we're actively maintaining. If anyone has a few pointers for me, I can hack that up real quick.

The setter can then be removed. There's only one test left (ecma_3/Date/15.9.5.6.js) that mentions gTestfile, though the rest still use it, via shell.js and browser.js.

Comment 1

[Bob Clary [:bc] (inactive)]

(In reply to comment #0)
> It would be less verbose and less error-prone to have each of the test runners
> populate gTestfile. A little grepping suggests that 2800 of 3100 tests have it,
> which is not so hot.
> 

Lets get our facts straight as the implied message is about my incompetence.

$ find . -mindepth 2 -name '*.js' | grep -v template.js | grep -v jsref.js | grep -v shell.js | grep -v browser.js | wc -l
    2870

$ find . -mindepth 2 -name '*.js' | grep -v template.js | grep -v jsref.js | grep -v shell.js | grep -v browser.js | xargs grep -L gTestfile
./ecma_3/FunExpr/regress-518103.js
./ecma_3/LexicalConventions/7.8.3-01.js
./js1_8_1/extensions/strict-warning.js
./js1_8_1/regress/regress-515885.js

> Stripping them out is the easy part:
> 
> find . -name '*.js' | xargs perl -pi -e 's/^(var )?gTestfile.*\n//;'
> 
> Making the test runners do the job instead is something I don't know much
> about. I don't even really know how many js/src/tests test runners we're
> actively maintaining. If anyone has a few pointers for me, I can hack that up
> real quick.
> 
> The setter can then be removed. There's only one test left
> (ecma_3/Date/15.9.5.6.js) that mentions gTestfile, though the rest still use
> it, via shell.js and browser.js.

Do whatever you want. I don't care.

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to comment #1)
> Lets get our facts straight as the implied message is about my incompetence.

I certainly didn't mean to imply any such thing and I welcome the correction.

Comment 3

[Bob Clary [:bc] (inactive)]

fixed the tests mentioned in comment 1.
http://hg.mozilla.org/tracemonkey/rev/84d84b53babd

Overall I think this is a good idea to have the frameworks take care of this if possible. We have currently the jsDriver.pl driver, the Sisyphus driver (to be removed shortly), the browser jsrefests and dmandelin's new shell framework jstest.py.

I don't want to lose the message about which test (including the full path) is being executed. I'm not sure of the best approach though. I kind of don't like to have it done in separate places, but that is not a big deal. One approach would be to have jsDriver.pl, jsreftest.html, jstest.py output the path instead of having the gTestfile setter do so. If you run the test directly through the shell you wouldn't get the message, but that isn't probably a blocker.

What do you think?

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)
> I don't want to lose the message about which test (including the full path) is
> being executed. I'm not sure of the best approach though. I kind of don't like
> to have it done in separate places, but that is not a big deal. One approach
> would be to have jsDriver.pl, jsreftest.html, jstest.py output the path instead
> of having the gTestfile setter do so. If you run the test directly through the
> shell you wouldn't get the message, but that isn't probably a blocker.
> 
> What do you think?

I agree with all of this.

When I run a single test directly, usually the command line contains the test filename, so the message isn't as important.

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4363529bb7c9