80642 2012-03-08 14:44:26 -0800 [Chromium] Exception running reftest with --no-pixel-tests 2012-03-08 20:08:05 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 80623 1 wangxianzhu wangxianzhu abarth dpranke ojan webkit.review.bot oldest_to_newest 574238 0 wangxianzhu 2012-03-08 14:44:26 -0800 ... File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 102, in run return self._run_reftest() File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 276, in _run_reftest test_output = self._driver.run_test(self._driver_input()) File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 212, in run_test cmd_line_key = self._cmd_line_as_key(pixel_tests_needed, driver_input.args) File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 240, in _cmd_line_as_key return ' '.join(self.cmd_line(pixel_tests, per_test_args)) File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 237, in cmd_line return self._driver.cmd_line(pixel_tests or self._pixel_tests, per_test_args or []) File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 453, in cmd_line cmd.extend(self._wrapper_options(pixel_tests)) File ".../WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 415, in _wrapper_options cmd.append("--pixel-tests=" + self._port._convert_path(self._image_path)) TypeError: cannot concatenate 'str' and 'NoneType' objects The error is because self._image_path is not initialized with --no-pixel-tests, but reftests need it. 574278 1 130913 wangxianzhu 2012-03-08 15:09:24 -0800 Created attachment 130913 patch 574285 2 130913 dpranke 2012-03-08 15:14:23 -0800 Comment on attachment 130913 patch This looks plausible, but I suspect there's another bug somewhere ... pixel_tests=True shouldn't be being passed to a driver that wasn't initialized with pixel_tests == True. Can you do a little more debugging and figure out how that's happening? I wonder if there's some of bug in the driver lookup code in DriverProxy. 574302 3 wangxianzhu 2012-03-08 15:23:03 -0800 (In reply to comment #2) > (From update of attachment 130913 [details]) > This looks plausible, but I suspect there's another bug somewhere ... pixel_tests=True shouldn't be being passed to a driver that wasn't initialized with pixel_tests == True. Can you do a little more debugging and figure out how that's happening? I wonder if there's some of bug in the driver lookup code in DriverProxy. Isn't this the intent of http://trac.webkit.org/changeset/109242? At line 522 of chromium.py, pixel test is forced if driver_input.is_reftest, even if --no-pixel-tests is passed (self._pixel_tests is False). 574315 4 dpranke 2012-03-08 15:32:31 -0800 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 130913 [details] [details]) > > This looks plausible, but I suspect there's another bug somewhere ... pixel_tests=True shouldn't be being passed to a driver that wasn't initialized with pixel_tests == True. Can you do a little more debugging and figure out how that's happening? I wonder if there's some of bug in the driver lookup code in DriverProxy. > > Isn't this the intent of http://trac.webkit.org/changeset/109242? At line 522 of chromium.py, pixel test is forced if driver_input.is_reftest, even if --no-pixel-tests is passed (self._pixel_tests is False). Sort of. It's possible I broke the changes and landed them out-of-order in a way that confused things. If you look at http://svn.webkit.org/repository/webkit/trunk@109264 , you'll see that DriverProxy has a map that maps the command line a DRT needs to an instance of the Driver class. If we think the test needs pixel tests, then we should only be forwarding that test to a driver that has self._pixel_tests = True. Put differently, driver_input.is_reftest and not self._pixel_tests should never be true. If it is, then there's a bug in the the DriverProxy code. 574359 5 wangxianzhu 2012-03-08 16:15:44 -0800 (In reply to comment #4) I found the problem: the drivers are keyed by the command line, but the command line containing forced pixel_tests can only be get from the existing driver which may not have pixel_tests, before the new driver could be created. 574364 6 130913 dpranke 2012-03-08 16:22:57 -0800 Comment on attachment 130913 patch Ah, makes sense. Thanks! 574388 7 wangxianzhu 2012-03-08 16:46:37 -0800 (In reply to comment #6) > (From update of attachment 130913 [details]) > Ah, makes sense. Thanks! Thanks for review. I feel passing pixel_tests parameter to Driver.start() and Driver.command_line() is a bit confusing. This let me think a driver can receive different pixel_tests parameters during its life. Would it be better to generate the key based on pixel_tests and per_test_args without calling the driver instance, and pass pixel_tests and per_test_args to the constructor of the driver? 574426 8 dpranke 2012-03-08 17:34:55 -0800 (In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 130913 [details] [details]) > > Ah, makes sense. Thanks! > > Thanks for review. > > I feel passing pixel_tests parameter to Driver.start() and Driver.command_line() is a bit confusing. This let me think a driver can receive different pixel_tests parameters during its life. Would it be better to generate the key based on pixel_tests and per_test_args without calling the driver instance, and pass pixel_tests and per_test_args to the constructor of the driver? You are right, it is confusing. We are moving to a model where the driver will be able to receive different pixel_test parameters per test, and we're halfway there. the cmd_line() function should really be a classmethod, independent of the particular instance. 574527 9 130913 webkit.review.bot 2012-03-08 20:08:00 -0800 Comment on attachment 130913 patch Clearing flags on attachment: 130913 Committed r110256: <http://trac.webkit.org/changeset/110256> 574528 10 webkit.review.bot 2012-03-08 20:08:05 -0800 All reviewed patches have been landed. Closing bug. 130913 2012-03-08 15:09:24 -0800 2012-03-08 20:08:00 -0800 patch 80642 text/plain 1838 wangxianzhu SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDExMDIxMSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE0IEBACisyMDEyLTAzLTA4ICBYaWFuemh1IFdhbmcgIDx3YW5neGlhbnpodUBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBFeGNlcHRpb24gcnVubmluZyByZWZ0ZXN0IHdp dGggLS1uby1waXhlbC10ZXN0cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9ODA2NDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0u cHk6CisgICAgICAgIChDaHJvbWl1bURyaXZlci5fX2luaXRfXyk6CisgICAgICAgIChDaHJvbWl1 bURyaXZlci5fd3JhcHBlcl9vcHRpb25zKToKKwogMjAxMi0wMy0wOCAgVG9yIEFybmUgVmVzdGLD uCAgPHRvci5hcm5lLnZlc3Rib0Bub2tpYS5jb20+CiAKICAgICAgICAgW1F0XSBQcm9zcGVjdGl2 ZSBXaW5kb3dzIGJ1aWxkIGZpeApJbmRleDogVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcG9ydC9jaHJvbWl1bS5weQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9TY3JpcHRzL3dlYmtp dHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5CShyZXZpc2lvbiAxMTAyMDkpCisrKyBU b29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5CSh3b3Jr aW5nIGNvcHkpCkBAIC00MDMsMTIgKzQwMywxMiBAQCBjbGFzcyBDaHJvbWl1bURyaXZlcihEcml2 ZXIpOgogICAgICAgICBEcml2ZXIuX19pbml0X18oc2VsZiwgcG9ydCwgd29ya2VyX251bWJlciwg cGl4ZWxfdGVzdHMsIG5vX3RpbWVvdXQpCiAgICAgICAgIHNlbGYuX3Byb2MgPSBOb25lCiAgICAg ICAgIHNlbGYuX2ltYWdlX3BhdGggPSBOb25lCi0gICAgICAgIGlmIHNlbGYuX3BpeGVsX3Rlc3Rz OgotICAgICAgICAgICAgc2VsZi5faW1hZ2VfcGF0aCA9IHNlbGYuX3BvcnQuX2ZpbGVzeXN0ZW0u am9pbihzZWxmLl9wb3J0LnJlc3VsdHNfZGlyZWN0b3J5KCksICdwbmdfcmVzdWx0JXMucG5nJyAl IHNlbGYuX3dvcmtlcl9udW1iZXIpCiAKICAgICBkZWYgX3dyYXBwZXJfb3B0aW9ucyhzZWxmLCBw aXhlbF90ZXN0cyk6CiAgICAgICAgIGNtZCA9IFtdCiAgICAgICAgIGlmIHBpeGVsX3Rlc3RzIG9y IHNlbGYuX3BpeGVsX3Rlc3RzOgorICAgICAgICAgICAgaWYgbm90IHNlbGYuX2ltYWdlX3BhdGg6 CisgICAgICAgICAgICAgICAgc2VsZi5faW1hZ2VfcGF0aCA9IHNlbGYuX3BvcnQuX2ZpbGVzeXN0 ZW0uam9pbihzZWxmLl9wb3J0LnJlc3VsdHNfZGlyZWN0b3J5KCksICdwbmdfcmVzdWx0JXMucG5n JyAlIHNlbGYuX3dvcmtlcl9udW1iZXIpCiAgICAgICAgICAgICAjIFNlZSBub3RlIGFib3ZlIGlu IGRpZmZfaW1hZ2UoKSBmb3Igd2h5IHdlIG5lZWQgX2NvbnZlcnRfcGF0aCgpLgogICAgICAgICAg ICAgY21kLmFwcGVuZCgiLS1waXhlbC10ZXN0cz0iICsgc2VsZi5fcG9ydC5fY29udmVydF9wYXRo KHNlbGYuX2ltYWdlX3BhdGgpKQogICAgICAgICAjIEZJWE1FOiBUaGlzIGlzIG5vdCBOb25lIHNo b3VsZG4ndCBiZSBuZWNlc3NhcnksIHVubGVzcyAtLWpzLWZsYWdzPSInJyIgY2hhbmdlcyBiZWhh dmlvciBzb21laG93Pwo=