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
Summary: new-run-webkit-tests: add port-specific mechanism for finding list of tests t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-14 19:30 PDT by Dirk Pranke
Modified: 2010-09-22 18:35 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.52 KB, patch)
2010-09-14 19:33 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (93.77 KB, patch)
2010-09-17 19:39 PDT, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-09-14 19:30:58 PDT
new-run-webkit-tests: add port-specific mechanism for finding list of tests to run
Comment 1 Dirk Pranke 2010-09-14 19:33:46 PDT
Created attachment 67634 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-09-15 14:05:37 PDT
I'm confused why we'd want this?  Why wouldn't the enabled-webgl test run just stop blacklisting them at that point?
Comment 3 Dirk Pranke 2010-09-15 14:14:20 PDT
(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).
Comment 4 Dirk Pranke 2010-09-16 21:11:12 PDT
Comment on attachment 67634 [details]
Patch

per conversation w/ tony@chromium.org, 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.
Comment 5 Dirk Pranke 2010-09-17 19:39:09 PDT
Created attachment 67993 [details]
Patch
Comment 6 Dirk Pranke 2010-09-17 19:48:36 PDT
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.
Comment 7 Ojan Vafai 2010-09-20 22:01:59 PDT
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:"?
Comment 8 Dirk Pranke 2010-09-21 12:01:09 PDT
(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!
Comment 9 Dirk Pranke 2010-09-21 12:11:53 PDT
Committed r67974: <http://trac.webkit.org/changeset/67974>
Comment 11 Tony Chang 2010-09-22 15:21:03 PDT
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.
Comment 12 Dirk Pranke 2010-09-22 18:22:57 PDT
(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.
Comment 13 Tony Chang 2010-09-22 18:32:14 PDT
(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
Comment 14 Dirk Pranke 2010-09-22 18:35:16 PDT
(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.