Bug 45801 – new-run-webkit-tests: add port-specific mechanism for finding list of tests to run and the ability to run w/o needing tests in the filesystem

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: add port-specific mechanism for finding list of tests to run

[Dirk Pranke]

Created attachment 67634 [details]
Patch

[Eric Seidel (no email)]

I'm confused why we'd want this?  Why wouldn't the enabled-webgl test run just stop blacklisting them at that point?

[Dirk Pranke]

(In reply to comment #2)
> I'm confused why we'd want this?  Why wouldn't the enabled-webgl test run just stop blacklisting them at that point?

I'm actually creating a new version of the port that will only run a subset of the tests by default (e.g., fast/compositing). By adding this, we can set the subset in the code and not have to worry about passing the right thing in on the command line or crafting an expectations file with a ton of SKIPs (which would break whenever a new directory was added).

This seems like a generically useful thing to have for any port or configuration that wants to run a minority of the layout tests; I'm open to other suggestions for how to do this (although note that I will need to create other changes for the accelerated versions of the ports anyway, to adjust command line flags and baseline search paths).

[Dirk Pranke]

Comment on attachment 67634 [details]
Patch

per conversation w/ [e-mail], I will re-do the patch to actually make the port/test.py implementation in-memory only. This will require changing more generic code and adding a few more hooks in the port interface to stop assuming that tests can be accessed via the filesystem directly.

We will look at what the resulting port patch looks like and figure out if it's worth it.

[Dirk Pranke]

Created attachment 67993 [details]
Patch

[Dirk Pranke]

Okay, the patch got *much* larger, but it works and I think it makes the generic code somewhat cleaner, at the cost (?) of preventing generic code from being able to assume that test files and expectations actually exist on the local filesystem.

The main advantage of this CL is that it deletes the whole tree under webkitpy/layout_tests/data and makes the test code in port/test.py a whole lot easier to understand.

There are a few test cases added for generic functionality and some minor bug fixes, but those could probably be applied separately. There is also a change that's half implemented to switch the code from
the results-live-in-the-filesystem approach used by test_shell/image_diff to the data-lives-in-memory-and-pipes approach used by DRT/ImageDiff. This could also be done independently of these other changes.

Please take a look at the ChangeLog entry for the list of everything that I had to change and then do some spot checking to see if we think this ends up being a useful change to commit before anyone spends a large amount of time reviewing the whole thing (or asking me to break it up into smaller chunks, which is doable but somewhat painful; I don't want to spend any more time on this CL if we don't actually like the intent). 

I think the patch is worthwhile, but I don't think it's a huge win and won't be bent out of shape if we decide that we would prefer to be able to assume files do exist in the filesystem.

[Ojan Vafai]

Comment on attachment 67993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67993&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:379
> +            return self.relative_test_filename(test)
> +        elif uri.startswith("http://127.0.0.1:8880/"):
> +            # websocket tests
> +            test = test.replace('http://127.0.0.1:8880/',
> +                                '')
> +            return test
> +        elif uri.startswith("http://"):
> +            # regular HTTP test
> +            test = test.replace('http://127.0.0.1:8000/',
> +                                'http/tests/')
> +            return test
> +        elif uri.startswith("https://"):

Per webkit style, these should not be elif since they early return in each block. Once they're made if statements, i'd also prefer to see a newline after each return.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:151
>          try:
> -            if self._executive.run_command(cmd, return_exit_code=True) == 0:
> -                return False
> -        except OSError, e:
> -            if e.errno == errno.ENOENT or e.errno == errno.EACCES:
> -                _compare_available = False
> -            else:
> -                raise e
> +            try:
> +                if self._executive.run_command(cmd, return_exit_code=True) == 0:
> +                    return False
> +            except OSError, e:
> +                if e.errno == errno.ENOENT or e.errno == errno.EACCES:
> +                    _compare_available = False
> +                else:
> +                    raise e
> +        finally:

Can't you write this as a single try/except/finally instead of a try/except nested inside a try/finally?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:108
> +        if not self._have_read_expected_checksum:

Do you really need the boolean? Can't you just check "if not self._image_checksum:"?

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 67993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67993&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:379
> > +            return self.relative_test_filename(test)
> > +        elif uri.startswith("http://127.0.0.1:8880/"):
> > +            # websocket tests
> > +            test = test.replace('http://127.0.0.1:8880/',
> > +                                '')
> > +            return test
> > +        elif uri.startswith("http://"):
> > +            # regular HTTP test
> > +            test = test.replace('http://127.0.0.1:8000/',
> > +                                'http/tests/')
> > +            return test
> > +        elif uri.startswith("https://"):
> 
> Per webkit style, these should not be elif since they early return in each block. Once they're made if statements, i'd also prefer to see a newline after each return.

Done.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:151
> >          try:
> > -            if self._executive.run_command(cmd, return_exit_code=True) == 0:
> > -                return False
> > -        except OSError, e:
> > -            if e.errno == errno.ENOENT or e.errno == errno.EACCES:
> > -                _compare_available = False
> > -            else:
> > -                raise e
> > +            try:
> > +                if self._executive.run_command(cmd, return_exit_code=True) == 0:
> > +                    return False
> > +            except OSError, e:
> > +                if e.errno == errno.ENOENT or e.errno == errno.EACCES:
> > +                    _compare_available = False
> > +                else:
> > +                    raise e
> > +        finally:
> 
> Can't you write this as a single try/except/finally instead of a try/except nested inside a try/finally?
>

You didn't used to be able to do this, but it you can starting in 2.5. I don't think we support 2.4 anywhere anymore, so I'll change it.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:108
> > +        if not self._have_read_expected_checksum:
> 
> Do you really need the boolean? Can't you just check "if not self._image_checksum:"?

expected_checksum() returns None if there is no checksum, so you can't use that. I've changed the routine to set _image_checksum = -1 to indicate that we haven't done a read instead.

Thanks for the review!

[Dirk Pranke]

Committed r67974: <http://trac.webkit.org/changeset/67974>

[WebKit Review Bot]

http://trac.webkit.org/changeset/67974 might have broken Qt Windows 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/67973
http://trac.webkit.org/changeset/67974
http://trac.webkit.org/changeset/67975
http://trac.webkit.org/changeset/67976
http://trac.webkit.org/changeset/67977
http://trac.webkit.org/changeset/67978
http://trac.webkit.org/changeset/67979

[Tony Chang]

Comment on attachment 67993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67993&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:524
> +                    with codecs.open(file1, "r", "utf8") as file_handle1:
> +                        output1 = file_handle1.read()
> +                    with codecs.open(file2, "r", "utf8") as file_handle2:
> +                        output2 = file_handle2.read()

This change broke the rebaseline tool.  There is no file1 or file2 here and you don't do anything with output1 or output2.

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 67993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67993&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:524
> > +                    with codecs.open(file1, "r", "utf8") as file_handle1:
> > +                        output1 = file_handle1.read()
> > +                    with codecs.open(file2, "r", "utf8") as file_handle2:
> > +                        output2 = file_handle2.read()
> 
> This change broke the rebaseline tool.  There is no file1 or file2 here and you don't do anything with output1 or output2.

True. This was fixed in r68085.

[Tony Chang]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 67993 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=67993&action=review
> > 
> > > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:524
> > > +                    with codecs.open(file1, "r", "utf8") as file_handle1:
> > > +                        output1 = file_handle1.read()
> > > +                    with codecs.open(file2, "r", "utf8") as file_handle2:
> > > +                        output2 = file_handle2.read()
> > 
> > This change broke the rebaseline tool.  There is no file1 or file2 here and you don't do anything with output1 or output2.
> 
> True. This was fixed in r68085.

Did you mean to comment out self._scm.add(path)?
http://trac.webkit.org/changeset/68085

[Dirk Pranke]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 67993 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=67993&action=review
> > > 
> > > > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:524
> > > > +                    with codecs.open(file1, "r", "utf8") as file_handle1:
> > > > +                        output1 = file_handle1.read()
> > > > +                    with codecs.open(file2, "r", "utf8") as file_handle2:
> > > > +                        output2 = file_handle2.read()
> > > 
> > > This change broke the rebaseline tool.  There is no file1 or file2 here and you don't do anything with output1 or output2.
> > 
> > True. This was fixed in r68085.
> 
> Did you mean to comment out self._scm.add(path)?
> http://trac.webkit.org/changeset/68085

Grr. Technically, no, I didn't mean to commit that change. On the other hand, that call doesn't actually work in a git checkout, which causes a crash (which is why it's commented out). Given that I expect scm.add() is a no-op under SVN, it's probably harmless.

I've been meaning to file a separate bug to track and fix that issue, but until then, I'll submit a change undoing this.