924481 - JSRefTest: load and run jit-test tests

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: jstest_load_jittests-v0.diff

This needs a large amount of tedious path work and just a touch of generalization.

The first patch teaches manifest.py how to identify and ignore the |jit-test| header, and fixes a number of bugs that cropped up with the slightly different directory layout.

The second patch is 90% tedious changes to the names of paths, so that we can have all the different path names we need to support loading and running tests from an external jit-tests directory. I expect we will want to eventually move the jit-tests/tests/ directory to tests/jittests/ or something, invalidating a bunch of this work; however, I'd like for the new workflow to be well supported before we cause too much disruption. The rest of the changes make the test flags we exec the superset of the jit-test and reftest flags.

In total, this allows us to load and run (successfully) the entirety of the jit and ref tests suites with one command. Unfortunately, all of the jit-tests are reported as failing because their output is significantly different from ref-tests. For now I've left the loading of jit-tests commented out until we can implement bug 924469.

Comment 1

[Terrence Cole [:terrence]]

Err, I meant to say we execute all of the non-headered jit-tests. I'm leaving support for the individual header properties for bug 746836.

Comment 2

[Terrence Cole [:terrence]]

Attachment: jstest_run_nonheader_jittests-v0.diff

And part two.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 814458 [details] [diff] [review]
jstest_load_jittests-v0.diff

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

Yep, r=me. Thanks for the renames too.

>-TEST_HEADER_PATTERN_INLINE = re.compile(r'//\s*\|(.*?)\|\s*(.*?)\s*(--\s*(.*))?$')
>-TEST_HEADER_PATTERN_MULTI  = re.compile(r'/\*\s*\|(.*?)\|\s*(.*?)\s*(--\s*(.*))?\*/')
>+TEST_HEADER_PATTERN_INLINE = re.compile(r'//\s*\|(.+?)\|\s*(.*?)\s*(--\s*(.*))?$')
>+TEST_HEADER_PATTERN_MULTI  = re.compile(r'/\*\s*\|(.+?)\|\s*(.*?)\s*(--\s*(.*))?\*/')

Right. I mean, obviously! Hard to believe we ever had it the other way. r'//\s*\|(.*?)\|\s*(.*?)\s*(--\s*(.*))?$'?! Clearly crazy! heh heh...

heh...

:-\

I wonder if jwz has an ornate NYH2P tattoo across his chest. I mean, I would, if I had said it. But that's just me.

::: js/src/tests/lib/manifest.py
@@ +105,5 @@
>      def test(self, cond):
>          return False
>  
> +def _parse_one_jittest_header(testcase):
> +    # TODO: do not skip jittests with headers.

Is the followup bug on file?

@@ +293,5 @@
>      """
>      entries = []
>  
> +    if not os.path.exists(filename):
> +        return entries

Style nit: consider instead making the call site look like this:

-    externalManifestEntries = _parse_external_manifest(manifestFile, '')
+    if os.path.exists(manifestFile):
+        externalManifestEntries = _parse_external_manifest(manifestFile, '')
+    else:
+        externalManifestEntries = []

Hard to articulate why I think this reads better; if it doesn't, to you, then don't change anything :)

@@ +337,5 @@
>              # that can't have their own failure type comments, so we simply
>              # use the terms for the most specific path.
>              testcase.terms = entry["terms"]
>              testcase.comment = entry["comment"]
> +            _parse_one_reftest_header(testcase, xul_tester)

Because external manifests only exist for js/src/tests. Hmm. Well, all right.

Comment 4

[Jason Orendorff [:jorendorff]]

Oh, never mind, the followup bug is bug 924469, and the next patch even changes the comment to say so. Perfect.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 814460 [details] [diff] [review]
jstest_run_nonheader_jittests-v0.diff

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

Clearing r? flag for now. I had enough trouble figuring out what the patch is doing that I'm asking for more comments and simpler code. Ping me when you get it re-posted and I'll review quickly. Sorry for bouncing the patch like this, I do think it's an important fix, and I'm keen to see it land. Thanks for taking this on.

::: js/src/tests/js1_5/extensions/regress-50447-1.js
@@ +159,5 @@
>  {
>    /* generate an error with only msg and filename properties */
>    enterFunc ("test4");
>  
> +  var expectedLine = 165;

If you like, you can instead:
   var expectedLine = (new Error().lineNumber) + 2;

Not that we expect to be touching this a lot in the future...

::: js/src/tests/jstests.py
@@ +13,5 @@
>  from lib.results import NullTestOutput
>  from lib.tests import TestCase
>  from lib.results import ResultsSink
>  from lib.progressbar import ProgressBar
> +from lib.paths import JITTEST_LIB_DIR, JITTEST_TESTS_DIR, REFTEST_TESTS_DIR

Is there a new file lib/paths.py that wasn't added to the patch?

@@ +308,5 @@
>      if options.debug:
>          if len(test_list) > 1:
>              print('Multiple tests match command line arguments, debugger can only run one')
>              for tc in test_list:
> +                print('    %s'%tc.relpath_tests)

Style nit: Can we get spaces around the % operator here please?

::: js/src/tests/lib/manifest.py
@@ +381,5 @@
>          if statbuf.st_size == 0:
>              continue
>  
> +        testcase = TestCase(os.path.join(reldir, filename),
> +                            os.path.join(location, filename))

Blah. The existing code here sucks. Can you clean it up?

At a minimum: _find_all_js_files should only take one argument. Currently it takes two and they are always the same value.

load() appears to be called in just one place, and 2 arguments are passed; the third argument can be removed.

Consider using two-argument os.path.relpath instead of
    root = root[len(base) + 1:]
in _find_all_js_files and possibly elsewhere.

::: js/src/tests/lib/tests.py
@@ +80,5 @@
> +        # Path relative to the top mozilla/ directory.
> +        self.relpath_top = os.path.relpath(abspath, TOP_SRC_DIR)
> +
> +        # Path relative to mozilla/js/src/.
> +        self.relpath_js = os.path.relpath(abspath, JS_DIR)

I found this whole patch a bit hard to follow, partly because with so many attributes here it's hard to remember what they're for and how they interrelate. How many independent axes are there here, really? I think it's just

    def __init__(self, suite_dir, path):
        assert suite_dir in ("tests", "jit-test/tests")
        assert os.path.isfile(os.path.join(JS_DIR, suite_dir, path))
        self.suite_dir = suite_dir
        self.path = path

everything else can be derived from that, in a method or getter, right? Is there a drawback to doing it that way?

Could we please change test.relpath_top to test.path_relative_to(TOP_SRC_DIR)? Readability, it's the Python way.

@@ +115,5 @@
> +    def create_extra_loads(self, libdir):
> +        """
> +        Find and create load directives for extra files needed to run a test.
> +        """
> +        root = self.abspath[0:-len(self.relpath_tests)]

This kind of thing is easy to get wrong (off-by-one, or if relpath_tests is empty the behavior would be crazy) so this will be better:

    root = os.path.join(JS_DIR, self.suite_dir)

@@ +118,5 @@
> +        """
> +        root = self.abspath[0:-len(self.relpath_tests)]
> +        def inner(relpath):
> +            if not relpath:
> +                return []

Maybe it's time to give up on the recursion and just write a while loop? Just a thought, your call.

@@ +153,5 @@
> +        remote execution.
> +        """
> +        if not remote_prefix:
> +            return self.abspath
> +        return self.abspath.replace(JITTEST_TESTS_DIR, remote_prefix)

(get_path_maybe_remote) This is another place where it's not clear what's going on here or why.

@@ +159,5 @@
> +    def get_command(self, js_cmd_prefix, libdir, remote_prefix=None):
> +        path = self.get_path_maybe_remote(remote_prefix)
> +        inject = TestCase.create_environment(path, libdir, remote_prefix)
> +        extra_loads = self.create_extra_loads(libdir)
> +        return js_cmd_prefix + self.options + extra_loads + [ '-e', inject, '-f', self.abspath ]

I don't understand what's going on with remote_prefix either... seems like the Test object should know if this suite requires a remote_prefix or not? I really don't know though.

@@ +176,5 @@
>          if js_args:
>              parts += js_args
>          self.js_cmd_prefix = parts
>  
>      def __cmp__(self, other):

I think you have to modify the notion of equivalence.

Also, pre-existing Python usage nit: __cmp__ implementations shouldn't look like this. You can implement __cmp__ in terms of cmp() on the underlying data. Either like this (slightly preferred):

    def __cmp__(self, other):
        return cmp((self.suite, self.path), (other.suite, other.path))

or like this ("clever" use of `or`):

    def __cmp__(self, other):
        return cmp(self.suite, other.suite) or cmp(self.path, other.path)

@@ +184,5 @@
>              return -1
>          return 1
>  
>      def __hash__(self):
> +        return self.relpath_tests.__hash__()

Of course __hash__ has to agree with __cmp__'s notion of equality, so:

    def __hash__(self):
        return hash((self.suite, self.path))

Comment 6

[Terrence Cole [:terrence]]

Closing with prejudice. The two suites have a different purpose: the different frontends reflect that. We can share (and already have) the complex executor bits in the backend and there's no point taking this further.