new-run-webkit-tests: support chromium code paths in mock_drt
Created attachment 80711 [details] Patch
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.
Created attachment 80713 [details] fix missing filesystem import
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 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.
Created attachment 80989 [details] revise w/ tony's review feedback
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.
(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).
Committed r77431: <http://trac.webkit.org/changeset/77431>