Last Comment Bug 735549 - Generate the jsreftest manifests at build time
: Generate the jsreftest manifests at build time
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on: 779688
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-13 18:34 PDT by Terrence Cole [:terrence]
Modified: 2012-08-01 16:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A script to update all required tests. (27.81 KB, patch)
2012-03-14 15:40 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
wip: still failing on try (397.23 KB, patch)
2012-04-06 10:19 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v1: Added url-prefix and removed dups (397.24 KB, patch)
2012-04-12 11:15 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v2: Had a '/' where I should have had an os.sep. (399.25 KB, patch)
2012-04-12 19:17 PDT, Terrence Cole [:terrence]
dmandelin: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-03-13 18:34:39 PDT
The goal of this bug is to get rid of the need to update a manifest file when adding a test or test directory to the jsreftest suite.

The best way to do this is to store the test metadata inline in the js test files, as is done by the jit tests.  With a fairly small amount of code, we should be able to dump out the reftest manifests what would be required to get the same result on command, say by passing -M or -R or something to jstests.py.

We can then add this command to the build step that generates tarballs for the reftest runner that is used by tbpl and never have to worry about manifest files ever again.
Comment 1 Bob Clary [:bc:] 2012-03-14 11:22:07 PDT
terrence: This sounds like a good plan. I just noticed the jit-test marking today when looking at a patch dmandelin landed. Let me know where I can help.

jmaher: Just confirming this won't affect mobile testing, right?
Comment 2 Joel Maher ( :jmaher ) 2012-03-14 11:28:47 PDT
I don't understand the entire flow here, but let me outline what we do for mobile, 

* We download the tests.zip file and unpack it
* we setup a webserver with the root at the tests.zip:/reftest directory
* we launch fennec on the remote device with a reference to http://<ip>/tests/layout/reftests/reftest.list (different url for jsreftests and crashtests)
* this expects the reftest harness to parse the list and run the tests
Comment 3 Terrence Cole [:terrence] 2012-03-14 15:19:41 PDT
Good!  That's basically how I thought the workflow went.  My plan is to end up giving you a tests.zip with a functionally identical reftest.list.  Ideally, you should not have to even know that that zip is being generated in a different way.
Comment 4 Terrence Cole [:terrence] 2012-03-14 15:40:06 PDT
Created attachment 605998 [details] [diff] [review]
A script to update all required tests.

Of the ~4000 tests we run, 377 require special treatment inside the manifest.  This script writes the information in those 377 lines directly into the affected tests inside of a special comment on the first line.
Comment 5 Terrence Cole [:terrence] 2012-04-06 10:19:47 PDT
Created attachment 612924 [details] [diff] [review]
wip: still failing on try
Comment 6 Terrence Cole [:terrence] 2012-04-12 11:15:28 PDT
Created attachment 614461 [details] [diff] [review]
v1: Added url-prefix and removed dups

Still failing, but only on windows now.  This is getting close.
Comment 7 Terrence Cole [:terrence] 2012-04-12 11:23:53 PDT
In windows the test page we load is jsreftest.html?test=/foo/bar/baz.js and everywhere else it is jsreftest.html?test=foo/bar/baz.js (as intended).  I expect this is down to a platform difference in os.path -- need to investigate what still, but it should be easy to track down.
Comment 8 Terrence Cole [:terrence] 2012-04-12 19:17:06 PDT
Created attachment 614654 [details] [diff] [review]
v2: Had a '/' where I should have had an os.sep.

https://tbpl.mozilla.org/?tree=Try&rev=cb6d4d86df3e

542 files changed, 633 insertions(+), 3922 deletions(-)
Comment 9 David Mandelin [:dmandelin] 2012-04-16 16:22:44 PDT
Comment on attachment 614654 [details] [diff] [review]
v2: Had a '/' where I should have had an os.sep.

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

Please add a note to the README about the metadata format, I think just a link to the reftest docs (don't want redundant docs).
Comment 10 Terrence Cole [:terrence] 2012-04-16 17:06:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/61189954ca17
Comment 11 Bob Clary [:bc:] 2012-04-16 17:50:58 PDT
Doesn't this break the ability to do a  make -C firefox-debug jstestbrowser to run the jsreftests locally?
Comment 12 Terrence Cole [:terrence] 2012-04-16 18:07:39 PDT
(In reply to Bob Clary [:bc:] from comment #11)
> Doesn't this break the ability to do a  make -C firefox-debug jstestbrowser
> to run the jsreftests locally?

I didn't even know you could do that.  I'll take a look at it.
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-17 07:46:09 PDT
https://hg.mozilla.org/mozilla-central/rev/61189954ca17
Comment 14 Bob Clary [:bc:] 2012-04-17 09:14:21 PDT
Terrence, would you like me to file a new bug or would you like to take care of the make -C firefox-debug jstestbrowser issue here?
Comment 15 Terrence Cole [:terrence] 2012-04-17 10:57:12 PDT
Well, this bug got marked fixed, so we better open a new one.  Bug 746236

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