748109 - jstests.py cannot find any test cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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:]]

Attachment: jstests.py cannot find any test cases

Comment 2

[Terrence Cole [:terrence]]

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

For posterity's sake, I missed this because I tested with:
./jstests.py $js_bin

Comment 4

[Steve Fink [:sfink] [:s:]]

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

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:]]

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

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

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:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74d0b096f253

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/74d0b096f253

Comment 11

[Steve Fink [:sfink] [:s:]]

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

https://hg.mozilla.org/mozilla-central/rev/ef75d8acb2a5