Bug 80642 - [Chromium] Exception running reftest with --no-pixel-tests
Summary: [Chromium] Exception running reftest with --no-pixel-tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on:
Blocks: 80623
  Show dependency treegraph
 
Reported: 2012-03-08 14:44 PST by Xianzhu Wang
Modified: 2012-03-08 20:08 PST (History)
4 users (show)

See Also:


Attachments
patch (1.79 KB, patch)
2012-03-08 15:09 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-03-08 14:44:26 PST
...
  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.
Comment 1 Xianzhu Wang 2012-03-08 15:09:24 PST
Created attachment 130913 [details]
patch
Comment 2 Dirk Pranke 2012-03-08 15:14:23 PST
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.
Comment 3 Xianzhu Wang 2012-03-08 15:23:03 PST
(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).
Comment 4 Dirk Pranke 2012-03-08 15:32:31 PST
(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.
Comment 5 Xianzhu Wang 2012-03-08 16:15:44 PST
(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.
Comment 6 Dirk Pranke 2012-03-08 16:22:57 PST
Comment on attachment 130913 [details]
patch

Ah, makes sense. Thanks!
Comment 7 Xianzhu Wang 2012-03-08 16:46:37 PST
(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?
Comment 8 Dirk Pranke 2012-03-08 17:34:55 PST
(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.
Comment 9 WebKit Review Bot 2012-03-08 20:08:00 PST
Comment on attachment 130913 [details]
patch

Clearing flags on attachment: 130913

Committed r110256: <http://trac.webkit.org/changeset/110256>
Comment 10 WebKit Review Bot 2012-03-08 20:08:05 PST
All reviewed patches have been landed.  Closing bug.