Bug 53471 – new-run-webkit-tests: support chromium code paths in mock_drt

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: support chromium code paths in mock_drt

[Dirk Pranke]

Created attachment 80711 [details]
Patch

[Dirk Pranke]

This patch makes the "TestShell" code paths for the mock- * implementation of DRT work - we write the image file to the file system, and output the correct text for the chromium.py code path.

One non-obvious change in this patch: in the run_one_test method of MockDRT, there previously were calls to UTF-8 encode the output returned from reading the text_file. It turns out that this would not work when we were mocking out a real platform, because filesystem.read_text_file() doesn't normally enforce the text being decoded to UTF-8, and it can't, since there is at least one file containing non-UTF-8 input
(svg/text/caret-in-svg-text-expected.txt). 

This is probably a bug in the checked-in baseline, but I'm still not sure if the output from DRT is always supposed to be UTF-8 or not. Arguably this should be a separate patch from the Chromium code path changes.

[Dirk Pranke]

Created attachment 80713 [details]
fix missing filesystem import

[Tony Chang]

Comment on attachment 80713 [details]
fix missing filesystem import

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

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:130
> -    option_parser = optparse.OptionParser(option_list=option_list)
> -    return option_parser.parse_args(argv)
> +    platform_index = argv.index('--platform')
> +    platform = argv[platform_index + 1]

What's wrong with using optparse here?  You could still convert it into a base.DummyOptions.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:177
> +        vals = line.strip().split("'")
> +        if len(vals) == 1:
> +            return (vals[0], None, None)
> +        return (vals[0], None, vals[1])

I would prefer if this returned a class to improve readability.  It's not obvious what the three values being returned are.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:214
> +class MockChromiumDRT(MockDRT):
> +    def parse_input(self, line):
> +        vals = line.split()

This could share the same class as above.

[Dirk Pranke]

Comment on attachment 80713 [details]
fix missing filesystem import

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

>> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:130
>> +    platform = argv[platform_index + 1]
> 
> What's wrong with using optparse here?  You could still convert it into a base.DummyOptions.

We can't use optparse because the WebKitDriver syntax uses --pixel-tests as a boolean, and ChromiumDriver uses --pixel-test=<path> as a field. In addition optparse will complain if it gets command line flags it doesn't recognize, and I don't want to repeat all of the possible options here (I don't even care about them).

>> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:177
>> +        return (vals[0], None, vals[1])
> 
> I would prefer if this returned a class to improve readability.  It's not obvious what the three values being returned are.

Okay. I don't have the aversion to tuples that you do, but I agree it's a bit undercommented otherwise.

[Dirk Pranke]

Created attachment 80989 [details]
revise w/ tony's review feedback

[Tony Chang]

Comment on attachment 80989 [details]
revise w/ tony's review feedback

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

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:238
> +            self.uri, self.timeout, self.checksum = vals
> +        else:
> +            self.uri = vals[0]
> +            self.timeout = vals[1]

Why do we bother saving the timeout if we're not going to use it?  You could document it with a comment if you wanted.

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 80989 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80989&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:238
> > +            self.uri, self.timeout, self.checksum = vals
> > +        else:
> > +            self.uri = vals[0]
> > +            self.timeout = vals[1]
> 
> Why do we bother saving the timeout if we're not going to use it?  You could document it with a comment if you wanted.

No huge reason, but I figured it made things a little clearer to keep around. Eventually this'll switch over to the DriverInput class (as per the FIXME).

[Dirk Pranke]

Committed r77431: <http://trac.webkit.org/changeset/77431>