Bug 53471

Summary: new-run-webkit-tests: support chromium code paths in mock_drt
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, mihai, tkent, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 49566    
Attachments:
Description Flags
Patch
none
fix missing filesystem import
none
revise w/ tony's review feedback tony: review+

Description Dirk Pranke 2011-01-31 20:20:42 PST
new-run-webkit-tests: support chromium code paths in mock_drt
Comment 1 Dirk Pranke 2011-01-31 20:21:29 PST
Created attachment 80711 [details]
Patch
Comment 2 Dirk Pranke 2011-01-31 20:26:03 PST
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.
Comment 3 Dirk Pranke 2011-01-31 20:33:51 PST
Created attachment 80713 [details]
fix missing filesystem import
Comment 4 Tony Chang 2011-02-02 15:01:14 PST
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.
Comment 5 Dirk Pranke 2011-02-02 15:31:25 PST
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.
Comment 6 Dirk Pranke 2011-02-02 15:56:20 PST
Created attachment 80989 [details]
revise w/ tony's review feedback
Comment 7 Tony Chang 2011-02-02 16:17:46 PST
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.
Comment 8 Dirk Pranke 2011-02-02 16:31:24 PST
(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).
Comment 9 Dirk Pranke 2011-02-02 16:33:38 PST
Committed r77431: <http://trac.webkit.org/changeset/77431>