Bug 80642 – [Chromium] Exception running reftest with --no-pixel-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xianzhu Wang]

...
  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.

[Xianzhu Wang]

Created attachment 130913 [details]
patch

[Dirk Pranke]

Comment on attachment 130913 [details]
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.

[Xianzhu Wang]

(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).

[Dirk Pranke]

(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.

[Xianzhu Wang]

(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.

[Dirk Pranke]

Comment on attachment 130913 [details]
patch

Ah, makes sense. Thanks!

[Xianzhu Wang]

(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?

[Dirk Pranke]

(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.

[WebKit Review Bot]

Comment on attachment 130913 [details]
patch

Clearing flags on attachment: 130913

Committed r110256: <http://trac.webkit.org/changeset/110256>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.