Bug 87128 – Add --driver-name option to run_webkit_tests.py to allow for selecting alternative DRT binaries

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] add --driver-name option to run_webkit_tests.py to allow for selecting alternative DRT binaries

Created attachment 143294 [details]
Patch

[Dirk Pranke]

Comment on attachment 143294 [details]
Patch

We should probably have at least one test for this ... I'd add something to run_webkit_tests_integrationtest.py to ensure that the command line arg has the right effect.

What's the long-term vision for running both DRT and ContentShell? Will we need two different "port"s? Do we need to set up additional bots?

[Dirk Pranke]

The other thing I should have mentioned is that this is basically the same as the other ports' use of -2 / --webkit-test-runner ; maybe we should reuse that concept and/or rename that flag and combine it with this?

Since it's not actually using the Webkit2 code base, it might be confusing to reuse the flag as is, and we probably wouldn't want to reuse much of the same code.

(In reply to comment #2)
> (From update of attachment 143294 [details])
> We should probably have at least one test for this ... I'd add something to run_webkit_tests_integrationtest.py to ensure that the command line arg has the right effect.

will do

> What's the long-term vision for running both DRT and ContentShell? Will we need two different "port"s? Do we need to set up additional bots?

It's not yet clear. I think we'll want to run content shell at least on the chromium waterfall. I could imagine running some of the tests on the webkit waterfall as well when this becomes more stable, esp. tests exercising the storage systems.

wrt what -2 does: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py#L64

they basically do the same, override the driver_name.

I'd rather not hard code content_shell into this, because we might end up renaming it, or introduce a wrapper script or similar.

Created attachment 143335 [details]
Patch

[Dirk Pranke]

Comment on attachment 143335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143335&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> +        return self.get_option('driver_name', 'DumpRenderTree')

Sorry I missed this before ... this should actually either be implemented in base.py or we should have a "virtual" base method in base.py that raises NotImplementedError.

(In reply to comment #6)
> (From update of attachment 143335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143335&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> > +        return self.get_option('driver_name', 'DumpRenderTree')
> 
> Sorry I missed this before ... this should actually either be implemented in base.py or we should have a "virtual" base method in base.py that raises NotImplementedError.

Not sure I understand. Other ports might not want this behavior (i.e. the ability to override the driver name). At the same time, they might rely on self.driver_name() to return "DumpRenderTree" (all chromium ports depended on this before this change)

So both options you suggest seem to break things?

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 143335 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143335&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> > > +        return self.get_option('driver_name', 'DumpRenderTree')
> > 
> > Sorry I missed this before ... this should actually either be implemented in base.py or we should have a "virtual" base method in base.py that raises NotImplementedError.
> 
> Not sure I understand. Other ports might not want this behavior (i.e. the ability to override the driver name). At the same time, they might rely on self.driver_name() to return "DumpRenderTree" (all chromium ports depended on this before this change)
> 
> So both options you suggest seem to break things?

Oh, sorry, I was thinking driver_name was a new method. Disregard the NotImplementedError idea, then :). That said, we try pretty hard to discourage port-specific behavior where possible, and I don't see any reason for this to be port-specific. 

Certainly if a port implemented --driver support in driver_name() and then had "DumpRenderTree" hardcoded elsewhere in their code, that would be a problem, but that sounds like bugs that should just be fixed if they are issues. A quick grep of the code shows that this only seems to show up in the files you've changed, and in the chromium_android code, and in unittest code, though, so we should be fine.

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 143335 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=143335&action=review
> > > 
> > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> > > > +        return self.get_option('driver_name', 'DumpRenderTree')
> > > 
> > > Sorry I missed this before ... this should actually either be implemented in base.py or we should have a "virtual" base method in base.py that raises NotImplementedError.
> > 
> > Not sure I understand. Other ports might not want this behavior (i.e. the ability to override the driver name). At the same time, they might rely on self.driver_name() to return "DumpRenderTree" (all chromium ports depended on this before this change)
> > 
> > So both options you suggest seem to break things?
> 
> Oh, sorry, I was thinking driver_name was a new method. Disregard the NotImplementedError idea, then :). That said, we try pretty hard to discourage port-specific behavior where possible, and I don't see any reason for this to be port-specific. 
> 
> Certainly if a port implemented --driver support in driver_name() and then had "DumpRenderTree" hardcoded elsewhere in their code, that would be a problem, but that sounds like bugs that should just be fixed if they are issues. A quick grep of the code shows that this only seems to show up in the files you've changed, and in the chromium_android code, and in unittest code, though, so we should be fine.

So is it ok to commit as is?

[Dirk Pranke]

(In reply to comment #9)
> So is it ok to commit as is?

Sorry I was unclear: you should move the implementation to base.py so that everyone has the option.

Created attachment 143384 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143384 [details]
Patch for landing

Clearing flags on attachment: 143384

Committed r118084: <http://trac.webkit.org/changeset/118084>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

REVERTED in r118088 - failing to run webkit_tests on Mac after this change:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.5/builds/11893
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/16640
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.7/builds/6091
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.5%20%28dbg%29%281%29/builds/7476

Sample log:

python /Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/webkit/tools/layout_tests/run_webkit_tests.py --no-show-results --no-new-test-results --verbose --full-results-html --clobber-old-results --results-directory /Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/layout-test-results --target Debug --platform chromium-mac --run-part 1:2 --builder-name "Webkit Mac10.5 (dbg)(1)" --build-number 7476 --master-name ChromiumWebkit --build-name Webkit_Mac10_5__dbg__1_ --test-results-server test-results.appspot.com ""
The current directory (/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build) is not a WebKit checkout, using /Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts
No handlers could be found for logger "webkitpy.common.system.executive"
17:19:14.701 1547 Using port 'chromium-mac-leopard'
17:19:14.701 1547 Test configuration: <leopard, x86, debug>
17:19:14.701 1547 Placing test results in /Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/layout-test-results
17:19:14.701 1547 Baseline search path: chromium-mac-leopard -> chromium-mac-snowleopard -> chromium-mac -> chromium -> mac -> generic
17:19:14.701 1547 Using Debug build
17:19:14.701 1547 Pixel tests enabled
17:19:14.701 1547 Regular timeout: 12000, slow test timeout: 60000
Traceback (most recent call last):
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 454, in <module>
    sys.exit(main())
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 449, in main
    return run(port, options, args)
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 102, in run
    manager.print_config()
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 1135, in print_config
    ' '.join(self._port.driver_cmd_line()))
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 571, in driver_cmd_line
    driver = self.create_driver(0)
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 770, in create_driver
    return driver.DriverProxy(self, worker_number, self._driver_class(), pixel_tests=self.get_option('pixel_tests'), no_timeout=no_timeout)
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 187, in __init__
    self._running_drivers[self._cmd_line_as_key(pixel_tests, [])] = self._driver
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 241, in _cmd_line_as_key
    return ' '.join(self.cmd_line(pixel_tests, per_test_args))
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py", line 238, in cmd_line
    return self._driver.cmd_line(pixel_tests, per_test_args or [])
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 487, in cmd_line
    cmd.append(self._port._path_to_driver())
  File "/Volumes/data/b/build/slave/Webkit_Mac10_5__dbg__1_/build/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py", line 167, in _path_to_driver
    return self._build_path(configuration, self.driver_name() + '.app',
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

[Joshua Bell]

(In reply to comment #14)
> REVERTED in r118088 - failing to run webkit_tests on Mac after this change:
> 

Linux too:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/25740
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/builds/18399
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29/builds/7106

Created attachment 143869 [details]
Patch

Dirk, can you have another look please?

It seems like the way I implemented driver_name was broken. I now implemented it with the same pattern as in webkit.py

[Dirk Pranke]

Comment on attachment 143869 [details]
Patch

It's really a bug that get_option doesn't return the default value that you pass in if the attr exists but is set to None. I need to go back and fix that.

(In reply to comment #18)
> (From update of attachment 143869 [details])
> It's really a bug that get_option doesn't return the default value that you pass in if the attr exists but is set to None. I need to go back and fix that.

Filed https://bugs.webkit.org/show_bug.cgi?id=87413

[WebKit Review Bot]

Comment on attachment 143869 [details]
Patch

Clearing flags on attachment: 143869

Committed r118408: <http://trac.webkit.org/changeset/118408>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.