Summary: | [Chromium] Exception running reftest with --no-pixel-tests | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||
Component: | Tools / Tests | Assignee: | Xianzhu Wang <wangxianzhu> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, dpranke, ojan, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 80623 | ||||||
Attachments: |
|
Description
Xianzhu Wang
2012-03-08 14:44:26 PST
Created attachment 130913 [details]
patch
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.
(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). (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. (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 on attachment 130913 [details]
patch
Ah, makes sense. Thanks!
(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? (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 on attachment 130913 [details] patch Clearing flags on attachment: 130913 Committed r110256: <http://trac.webkit.org/changeset/110256> All reviewed patches have been landed. Closing bug. |