jstests.py cannot find any test cases

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Created attachment 617634 [details] [diff] [review]
jstests.py cannot find any test cases
Attachment #617634 - Flags: review?(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.
Attachment #617634 - Flags: review?(terrence) → review+
For posterity's sake, I missed this because I tested with:
./jstests.py $js_bin
(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.)
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?
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. :)
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.
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d0b096f253
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/74d0b096f253
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
And after all that, I ended up landing the wrong patch. Fixed with:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef75d8acb2a5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ef75d8acb2a5
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.