Last Comment Bug 748109 - jstests.py cannot find any test cases
: jstests.py cannot find any test cases
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 13:59 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-04-26 10:43 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
jstests.py cannot find any test cases (1.39 KB, patch)
2012-04-23 13:59 PDT, Steve Fink [:sfink] [:s:]
terrence: review+
Details | Diff | Review

Description Steve Fink [:sfink] [:s:] 2012-04-23 13:59:08 PDT
When I do:
  cd js/src/tests
  python jstests.py $js_bin js1_8_5/extensions

it is unable to find any tests to run because it's looking relative to the jstests.py script, but at least in this case that is a relative path (plain "jstests.py") so os.path.dirname(__file__) is the empty string and that doesn't seem to work very well.
Comment 1 Steve Fink [:sfink] [:s:] 2012-04-23 13:59:19 PDT
Created attachment 617634 [details] [diff] [review]
jstests.py cannot find any test cases
Comment 2 Terrence Cole [:terrence] 2012-04-23 14:17:14 PDT
Comment on attachment 617634 [details] [diff] [review]
jstests.py cannot find any test cases

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

r+ with a single change made.

::: js/src/tests/jstests.py
@@ +146,5 @@
>              xul_debug = xul_debug.lower() is 'true'
>              xul_info = manifest.XULInfo(xul_abi, xul_os, xul_debug)
>          xul_tester = manifest.XULInfoTester(xul_info, JS)
>  
> +    test_dir = os.path.dirname(os.path.abspath(__file__))

Lets go all the way and use os.path.realpath here: it's like abspath, but also follows symlinks.  This will also allow people to symlink jstests.py into another directory and still have things work.
Comment 3 Terrence Cole [:terrence] 2012-04-23 14:18:57 PDT
For posterity's sake, I missed this because I tested with:
./jstests.py $js_bin
Comment 4 Steve Fink [:sfink] [:s:] 2012-04-23 14:49:25 PDT
(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 617634 [details] [diff] [review]
> jstests.py cannot find any test cases
> 
> Review of attachment 617634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a single change made.
> 
> ::: js/src/tests/jstests.py
> @@ +146,5 @@
> >              xul_debug = xul_debug.lower() is 'true'
> >              xul_info = manifest.XULInfo(xul_abi, xul_os, xul_debug)
> >          xul_tester = manifest.XULInfoTester(xul_info, JS)
> >  
> > +    test_dir = os.path.dirname(os.path.abspath(__file__))
> 
> Lets go all the way and use os.path.realpath here: it's like abspath, but
> also follows symlinks.  This will also allow people to symlink jstests.py
> into another directory and still have things work.

But is that how you would expect it to work? For example:

 % mkdir tests2
 % cd tests2
 % ln -s ../tests/jstests.py jstests.py
 % cp -rp ../tests/shell.js ../tests/js1_4 .
 % python jstests.py

I would expect that to run just the tests sitting in the new tests2/ directory, not the tests in the original tests/ directory. With abspath, you'll run the tests2/ tests. With realpath, you'll run the tests/ tests. (Worse, if you tab-complete js1_4/Eval/eval-001.js when running from the new directory, it'll run eval-001.js out of the old directory.)
Comment 5 Terrence Cole [:terrence] 2012-04-23 15:51:18 PDT
I thing I find odd there is the combination of symlinking and copying :-).  I wouldn't expect a link to change the behavior of the underlying executable.  The specific scenario I see being useful is:

% ln -s ./tests/jstests.py t
% ./t

Perhaps the real problem here is that the test matching behavior does not match the shell's behavior?
Comment 6 Steve Fink [:sfink] [:s:] 2012-04-23 16:03:50 PDT
I just copied the tests because it was easier to give an example that way. You can replace the copy setp with "# write some tests".

But I think I see where you're coming from. You want a link that runs the one and only set of tests. jstests.py == "run all of the JS tests"

I was thinking of jstests.py as a reusable harness that you could use for independent piles of tests. jstests.py == "run the JS test harness on these tests"

Since we don't see to have or want to have little piles of tests scattered around different places, your interpretation seems more useful. So please forget I said anything. :)
Comment 7 Terrence Cole [:terrence] 2012-04-23 16:09:49 PDT
Ah, and now I see where you are coming from too.  What the harness did before I removed manifests was look for a manifest file next to jstests.py.  You could run the harness on any old pile of tests by specifying a manifest.  I think this still had the same confusion factor with the default search, however.  We should probably add a CLI switch to specify a base directory for test searching, similar to the previous switch that let you specify a manifest.
Comment 8 Terrence Cole [:terrence] 2012-04-23 16:19:57 PDT
Well, this is easy to find, so let me just paste in what we did before.

-    if OPTIONS.manifest is None:
-        filename = os.path.join(os.path.dirname(__file__), 'jstests.list')
-        if os.path.isfile(filename):
-            OPTIONS.manifest = filename
-        else:
-            print >> sys.stderr, 'no manifest file given and defaults not found'
-            sys.exit(2)

As you can see, it doesn't fall over where my change did, because it's adding the manifest to the path in all cases.  It does, however, appear to favor the "any old pile of tests" workflow over the "symlink a shorter name" workflow.

Given that the previous behavior matches abspath, we should probably go with your original patch to lower the surprise factor.  I've already got my shorter name through a wrapper script, so I've got no pony in this race.  I'll open a bug for the CLI switch and a bug to change this to a realpath, if we decide that's what we want to do.
Comment 9 Steve Fink [:sfink] [:s:] 2012-04-24 17:04:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d0b096f253
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:24:11 PDT
https://hg.mozilla.org/mozilla-central/rev/74d0b096f253
Comment 11 Steve Fink [:sfink] [:s:] 2012-04-25 16:58:26 PDT
And after all that, I ended up landing the wrong patch. Fixed with:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef75d8acb2a5
Comment 12 Ed Morley [:emorley] 2012-04-26 10:43:26 PDT
https://hg.mozilla.org/mozilla-central/rev/ef75d8acb2a5

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