Closed
Bug 525213
Opened 15 years ago
Closed 15 years ago
gTestfile should not be set in individual js/tests
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Unassigned)
Details
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•15 years ago
|
||
(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.
Reporter | ||
Comment 2•15 years ago
|
||
(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•15 years ago
|
||
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?
Reporter | ||
Comment 4•15 years ago
|
||
(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•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•