Modifications to layout test framework so that it can work with browser_tests.

Modifications to layout test framework so that it can work with browsertests. This is for end to end testing for print preview.

BUG=388517
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178486

[Dan Beam, 2014-07-09 22:19:15]

i'll defer to dpranke@ here (-dbeam@)

[ivandavid, 2014-07-10 22:39:10]

I fixed some of the mistakes I made when merging that caused some code to be
accidentally deleted. I also refactored browsert_test.py so that PORT_CLASSES in
factory.py wouldn't need so many browser_test classes to look for. This
currently works on linux, however I am unsure about the other two platforms. My
first priority is to actually test out this code on those platforms and iron out
any kinks, however I think there will be minimal changes needed (if any).

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/base.py:737: '.htm', '.php', '.svg',
'.mht', '.pdf'])
Added so that the framework can look for .pdf tests.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1538:
['--enable-accelerated-2d-canvas']),
All of this VirtualTestSuite stuff is the undoing of a mistake I made when
getting the code. The changes put it in sync with the current origin/master
base.py.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:57: class
BrowserTestPort(object):
I decided to to make a class that creates the ports for each platform because
making a different class for each port was tedious and not really needed, since
the only changes that were needed were changes to path name and not anything
else, I felt like this would be a good solution. Also, it means that
PORT_CLASSES in factory.py won't need so many browser test classes.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:52: if options.platform
and 'test' in options.platform and not 'browser_test' in options.platform:
Without this addition to the conditional, the framework would set the wrong
host.

[Dirk Pranke, 2014-07-10 23:37:13]

This looks like it's on the right track.

1) Please update the change description with a *lot* more information about what
you're actually doing.

2) the bug number you linked to -- 388217 -- isn't right, is it? that looks
quite unrelated.

3) I would break my feedback into two sets ... the first involves your use of
__new__, and the second is everything else. I expected the "everything else" is
straightforward. If you *really* want to use __new__ feel free to upload two
patches to this, one with everything else (but leaving __new__ in), and one with
everything else *plus* removing __new__, and attempt to convince me that the
non-__new__ approach really is horrible :).

Does that make sense?

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:39: return
self._driver_class()(self, worker_number,
pixel_tests=self.get_option('pixel_tests'), no_timeout=no_timeout)
I don't think you need to override create_driver(), do you?

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:54: return
self.path_from_chromium_base('chrome', 'test', 'data', 'printing',
'layout_tests', 'results')
this is probably not the best place to write the results to (the default we use
in base is wrong as well), but I'm not sure if I have a better suggestion
offhand.

The questions to ask yourself are:

1) if you're running this interactively (from the shell), where are you likely
to be doing that, and where would you assume the results would be?

2) where would we want the results on the bots? 

I'd be inclined to either try to keep this outside of the checkout completely
(i.e., into /tmp/ somewhere), or at least under out/ somewhere.

And, actually, the bots pass in an explicit --results-directory anyway, so this
is mostly aimed at 1).

But, I probably wouldn't change anything here w/o a discussion with a few people
to agree on a destination. If you've already had that discussion and agreed on
what you implemented, that's fine too.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:57: class
BrowserTestPort(object):
Unfortunately, while our coding style allows for multiple inheritance, it bans
this sort of metaprogramming.

Doing things like this tends to make it a little too difficult to understand
what is going on at runtime, and also to understand, statically, just how many
different classes there are in the system (i.e., how many different subclasses
of Port there are). That's why I suggested you either create a proxy class like
MockDRT does, or create the 3 classes you need.

In this case, I'd probably encourage the latter; it's probably only another
10-20 lines of code.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:54:
self._server_process._proc.stdin = open(path, 'w', 0)
I am worried that this approach won't actually work on windows; have you tested
it there?

Specifically, windows tends to assume that files are opened exclusively for
writing (unlike unix), and so you may get access errors when you have a file
open in one process for writing and the other process for reading. There might
be some additional flags you can pass to open() that will make this work, I'm
not sure.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:76: called.
I would modify Driver.start() to take an (optional?) deadline argument, so that
you don't need to override this method.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:147: block =
ContentBlockStdin()
If this is the only difference from Driver._read_block, you should just refactor
Driver to add a _read_content_block() method that you can override, so that you
don't have to override this method.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/factory.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/factory.py:111: 
nit: delete the stray blank line?

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:266:
optparse.make_option("--driver_name"),
--driver-name exists already on line 149. Presumably this line isn't needed?

[ivandavid, 2014-07-16 21:29:01]

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:39: return
self._driver_class()(self, worker_number,
pixel_tests=self.get_option('pixel_tests'), no_timeout=no_timeout)
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> I don't think you need to override create_driver(), do you?

Done.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:54: return
self.path_from_chromium_base('chrome', 'test', 'data', 'printing',
'layout_tests', 'results')
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> this is probably not the best place to write the results to (the default we
use
> in base is wrong as well), but I'm not sure if I have a better suggestion
> offhand.
> 
> The questions to ask yourself are:
> 
> 1) if you're running this interactively (from the shell), where are you likely
> to be doing that, and where would you assume the results would be?
> 
> 2) where would we want the results on the bots? 
> 
> I'd be inclined to either try to keep this outside of the checkout completely
> (i.e., into /tmp/ somewhere), or at least under out/ somewhere.
> 
> And, actually, the bots pass in an explicit --results-directory anyway, so
this
> is mostly aimed at 1).
> 
> But, I probably wouldn't change anything here w/o a discussion with a few
people
> to agree on a destination. If you've already had that discussion and agreed on
> what you implemented, that's fine too.
Actually, I think we can just keep everything in a temporary directory for the
results. I also just removed the function, since a directory path can be passed
in.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:54: return
self.path_from_chromium_base('chrome', 'test', 'data', 'printing',
'layout_tests', 'results')
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> this is probably not the best place to write the results to (the default we
use
> in base is wrong as well), but I'm not sure if I have a better suggestion
> offhand.
> 
> The questions to ask yourself are:
> 
> 1) if you're running this interactively (from the shell), where are you likely
> to be doing that, and where would you assume the results would be?
> 
> 2) where would we want the results on the bots? 
> 
> I'd be inclined to either try to keep this outside of the checkout completely
> (i.e., into /tmp/ somewhere), or at least under out/ somewhere.
> 
> And, actually, the bots pass in an explicit --results-directory anyway, so
this
> is mostly aimed at 1).
> 
> But, I probably wouldn't change anything here w/o a discussion with a few
people
> to agree on a destination. If you've already had that discussion and agreed on
> what you implemented, that's fine too.

Done.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:57: class
BrowserTestPort(object):
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> Unfortunately, while our coding style allows for multiple inheritance, it bans
> this sort of metaprogramming.
> 
> Doing things like this tends to make it a little too difficult to understand
> what is going on at runtime, and also to understand, statically, just how many
> different classes there are in the system (i.e., how many different subclasses
> of Port there are). That's why I suggested you either create a proxy class
like
> MockDRT does, or create the 3 classes you need.
> 
> In this case, I'd probably encourage the latter; it's probably only another
> 10-20 lines of code.
OK. I changed it, I don't think I exercised much foresight, because I realized
doing this sort of defeated the point of having multiple classes, since
extending it would be a pain in the butt. I decided to go with the 3 classes
implementation.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:57: class
BrowserTestPort(object):
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> Unfortunately, while our coding style allows for multiple inheritance, it bans
> this sort of metaprogramming.
> 
> Doing things like this tends to make it a little too difficult to understand
> what is going on at runtime, and also to understand, statically, just how many
> different classes there are in the system (i.e., how many different subclasses
> of Port there are). That's why I suggested you either create a proxy class
like
> MockDRT does, or create the 3 classes you need.
> 
> In this case, I'd probably encourage the latter; it's probably only another
> 10-20 lines of code.

Done.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:54:
self._server_process._proc.stdin = open(path, 'w', 0)
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> I am worried that this approach won't actually work on windows; have you
tested
> it there?
> 
> Specifically, windows tends to assume that files are opened exclusively for
> writing (unlike unix), and so you may get access errors when you have a file
> open in one process for writing and the other process for reading. There might
> be some additional flags you can pass to open() that will make this work, I'm
> not sure.
I changed it to 'wb' so it can work on windows. When I tested out the code,
everything worked with having one end of the file open for writing and the other
open for reading.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:76: called.
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> I would modify Driver.start() to take an (optional?) deadline argument, so
that
> you don't need to override this method.

Done.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:147: block =
ContentBlockStdin()
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> If this is the only difference from Driver._read_block, you should just
refactor
> Driver to add a _read_content_block() method that you can override, so that
you
> don't have to override this method.

Done.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/factory.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/factory.py:111: 
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> nit: delete the stray blank line?

Done.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:266:
optparse.make_option("--driver_name"),
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> --driver-name exists already on line 149. Presumably this line isn't needed?
Oops. I was always using underscore rather than dash, which was causing the
option to not be found. I removed this code.

https://codereview.chromium.org/378113003/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:266:
optparse.make_option("--driver_name"),
On 2014/07/10 23:37:13, Dirk Pranke wrote:
> --driver-name exists already on line 149. Presumably this line isn't needed?

Done.

[Dirk Pranke, 2014-07-16 23:37:20]

getting close ...

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:71: return
self._build_path_with_configuration(configuration, self.driver_name())
Do you actually need this override?

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:83:
shutil.rmtree(self.stdin_directory)
You probably want ignore_errors=True here, or otherwise do something to catch
the exceptions that may be raised from time to time if the call fails (we do see
this, especially on windows).

[ivandavid, 2014-07-16 23:53:52]

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:71: return
self._build_path_with_configuration(configuration, self.driver_name())
On 2014/07/16 23:37:20, Dirk Pranke wrote:
> Do you actually need this override?
Yes, if it isn't overridden, the one in mac.py can't find the driver as it
appends extra directories and file extensions to the driver_name.

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:83:
shutil.rmtree(self.stdin_directory)
On 2014/07/16 23:37:20, Dirk Pranke wrote:
> You probably want ignore_errors=True here, or otherwise do something to catch
> the exceptions that may be raised from time to time if the call fails (we do
see
> this, especially on windows).

Done.

[Dirk Pranke, 2014-07-16 23:59:31]

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:71: return
self._build_path_with_configuration(configuration, self.driver_name())
On 2014/07/16 23:53:52, ivandavid wrote:
> On 2014/07/16 23:37:20, Dirk Pranke wrote:
> > Do you actually need this override?
> Yes, if it isn't overridden, the one in mac.py can't find the driver as it
> appends extra directories and file extensions to the driver_name.

ah, right.

[Dirk Pranke, 2014-07-17 00:03:07]

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:62: return
linux.LinuxPort.determine_full_port_name(host, options, port_name)
Odd, one of my comments got dropped somewhere. Why do you need to override
determine_full_port_name() in these classes ?

a) aren't you invoking the methods that would normally get invoked anyway via
multiple inheritance?
b) given that this is only invoked via port_factory.get(), and you're
short-circuiting get() and calling get_port_class_name() instead, won't these
never be called?

[ivandavid, 2014-07-17 00:18:09]

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:62: return
linux.LinuxPort.determine_full_port_name(host, options, port_name)
On 2014/07/17 00:03:06, Dirk Pranke wrote:
> Odd, one of my comments got dropped somewhere. Why do you need to override
> determine_full_port_name() in these classes ?
> 
> a) aren't you invoking the methods that would normally get invoked anyway via
> multiple inheritance?
> b) given that this is only invoked via port_factory.get(), and you're
> short-circuiting get() and calling get_port_class_name() instead, won't these
> never be called?
Oops. This is a hold over from some other code I wrote when I was trying to
figure out how this stuff worked. I removed it.

https://codereview.chromium.org/378113003/diff/260001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:62: return
linux.LinuxPort.determine_full_port_name(host, options, port_name)
On 2014/07/17 00:03:06, Dirk Pranke wrote:
> Odd, one of my comments got dropped somewhere. Why do you need to override
> determine_full_port_name() in these classes ?
> 
> a) aren't you invoking the methods that would normally get invoked anyway via
> multiple inheritance?
> b) given that this is only invoked via port_factory.get(), and you're
> short-circuiting get() and calling get_port_class_name() instead, won't these
> never be called?

Done.

[Dirk Pranke, 2014-07-17 00:31:16]

Just a couple of nits now, and one real quibble about how you are using
deadlines and timeouts that should be simple to clean up (for consistency) ...

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:13: #     * Neither the
Google name nor the names of its
this is an odd diff ...

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:29: from
webkitpy.layout_tests.port import base
you don't need to import base (see below)

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:46: class
BrowserTestPortOverrides(base.Port):
this should just be object, not base.Port, since it's a mixin.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:82: if
self.stdin_directory != None:
this should be just 'if self.stdin_directory':

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/driver.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:153:
self.start(driver_input.should_run_pixel_test, driver_input.args,
int(driver_input.timeout))
This timeout value might be quite long; you might want to use a shorter value by
default (like 3 seconds).

Also, see comments elsewhere, but deadline is always (I think) a wall clock
time, not a time interval / timeout, so you should adjust this to be start_time
+ X.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:268: def start(self,
pixel_tests, per_test_args, deadline=0):
See comments above re: deadline and wall-clock times vs. timeouts.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:423: return (None, False)
If this is only called by browser_test_driver (which is what it looks like), it
should live in that module instead.

[ivandavid, 2014-07-17 03:04:48]

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:13: #     * Neither the
Google name nor the names of its
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> this is an odd diff ...
O_o ... I don't know how that happened.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:13: #     * Neither the
Google name nor the names of its
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> this is an odd diff ...

Done.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:29: from
webkitpy.layout_tests.port import base
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> you don't need to import base (see below)

Done.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:46: class
BrowserTestPortOverrides(base.Port):
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> this should just be object, not base.Port, since it's a mixin.

Done.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:46: class
BrowserTestPortOverrides(base.Port):
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> this should just be object, not base.Port, since it's a mixin.
I had to swap the order of inheritance, ie.

linux.LinuxPort, BrowserTestPortOverrides -->
BrowserTestOverrides,linux.LinuxPort

because otherwise the overrides would not take effect.

This is because of how super() works correct?

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py:82: if
self.stdin_directory != None:
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> this should be just 'if self.stdin_directory':

Done.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/driver.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:153:
self.start(driver_input.should_run_pixel_test, driver_input.args,
int(driver_input.timeout))
On 2014/07/17 00:31:16, Dirk Pranke wrote:
> This timeout value might be quite long; you might want to use a shorter value
by
> default (like 3 seconds).
> 
> Also, see comments elsewhere, but deadline is always (I think) a wall clock
> time, not a time interval / timeout, so you should adjust this to be
start_time
> + X.
Ok. I made it work based on the wall clock in the manner you suggested. I was
adding the wall clock inside the BrowserTestDriver.start, but that was hard to
follow. I still let the deadline be based on driver_input.timeout, since the
testing being slow (for whatever reason) could change how long it takes for the
path and a few other things to be set up. I use 2000 as the divisor, just to
make it a bit faster, since it shouldn't take that long.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:153:
self.start(driver_input.should_run_pixel_test, driver_input.args,
int(driver_input.timeout))
On 2014/07/17 00:31:16, Dirk Pranke wrote:
> This timeout value might be quite long; you might want to use a shorter value
by
> default (like 3 seconds).
> 
> Also, see comments elsewhere, but deadline is always (I think) a wall clock
> time, not a time interval / timeout, so you should adjust this to be
start_time
> + X.

Done.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:268: def start(self,
pixel_tests, per_test_args, deadline=0):
On 2014/07/17 00:31:16, Dirk Pranke wrote:
> See comments above re: deadline and wall-clock times vs. timeouts.
I removed the default value, is that ok?

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:268: def start(self,
pixel_tests, per_test_args, deadline=0):
On 2014/07/17 00:31:16, Dirk Pranke wrote:
> See comments above re: deadline and wall-clock times vs. timeouts.

Done.

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:423: return (None, False)
On 2014/07/17 00:31:15, Dirk Pranke wrote:
> If this is only called by browser_test_driver (which is what it looks like),
it
> should live in that module instead.

Done.

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:50: self._webkit_finder
= webkit_finder.WebKitFinder(host.filesystem)
'git cl upload' didn't like when I just inherited from object and kept the rest
of the code. It said that there was an error because BrowserTestPortOverrides
didn't have a member function path_from_chromium_base(), yet it was calling it.
To get around this, I just instantiate another WebKitFinder and use it to get
the
correct directory. I know its roundabout, but the presubmit checks wouldn't let
me use the better solution.

[Dirk Pranke, 2014-07-17 21:20:13]

at this point it basically looks good.

1) See my comments about your pylint problem; I think it shouldn't be happening,
so let's figure out what's going wrong for you.

2) Can you add some sort of basic unittests to get code coverage of
browser_test.py ?

Also, make sure you run test-webkitpy and don't have any other regressions
(there are three tests in driver_unittests.py that will fail due to your change
to driver.start()).

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/300001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:46: class
BrowserTestPortOverrides(base.Port):
On 2014/07/17 03:04:48, ivandavid wrote:
> On 2014/07/17 00:31:15, Dirk Pranke wrote:
> > this should just be object, not base.Port, since it's a mixin.
> I had to swap the order of inheritance, ie.
> 
> linux.LinuxPort, BrowserTestPortOverrides -->
> BrowserTestOverrides,linux.LinuxPort
> 
> because otherwise the overrides would not take effect.
> 
> This is because of how super() works correct?

Correct, the overriding class (the mixin) needs to go first.

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:50: self._webkit_finder
= webkit_finder.WebKitFinder(host.filesystem)
On 2014/07/17 03:04:48, ivandavid wrote:
> 'git cl upload' didn't like when I just inherited from object and kept the
rest
> of the code. It said that there was an error because BrowserTestPortOverrides
> didn't have a member function path_from_chromium_base(), yet it was calling
it.
> To get around this, I just instantiate another WebKitFinder and use it to get
> the
> correct directory. I know its roundabout, but the presubmit checks wouldn't
let
> me use the better solution.

That sounds like a bug in pylint/presubmit. If the code actually works, better
to disable the warning rather than work around it.

That said, I applied your patch locally and tested this. First I deleted line 29
and line 50, and that ran cleanly; next I deleted the __init__ method
altogether, and it still ran cleanly, so perhaps something else is going on?

[ivandavid, 2014-07-18 04:24:59]

I wrote some tests for each of the ports. I also wrote one for the driver to
test if it can part the path string correctly and handle each case.

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test.py (right):

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:50: self._webkit_finder
= webkit_finder.WebKitFinder(host.filesystem)
On 2014/07/17 21:20:13, Dirk Pranke wrote:
> On 2014/07/17 03:04:48, ivandavid wrote:
> > 'git cl upload' didn't like when I just inherited from object and kept the
> rest
> > of the code. It said that there was an error because
BrowserTestPortOverrides
> > didn't have a member function path_from_chromium_base(), yet it was calling
> it.
> > To get around this, I just instantiate another WebKitFinder and use it to
get
> > the
> > correct directory. I know its roundabout, but the presubmit checks wouldn't
> let
> > me use the better solution.
> 
> That sounds like a bug in pylint/presubmit. If the code actually works, better
> to disable the warning rather than work around it.
> 
> That said, I applied your patch locally and tested this. First I deleted line
29
> and line 50, and that ran cleanly; next I deleted the __init__ method
> altogether, and it still ran cleanly, so perhaps something else is going on?
I just disabled the pylint error message. I noticed a few lines of code in the
same directory with the same problem that handled it this way.

https://codereview.chromium.org/378113003/diff/340001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test.py:50: self._webkit_finder
= webkit_finder.WebKitFinder(host.filesystem)
On 2014/07/17 21:20:13, Dirk Pranke wrote:
> On 2014/07/17 03:04:48, ivandavid wrote:
> > 'git cl upload' didn't like when I just inherited from object and kept the
> rest
> > of the code. It said that there was an error because
BrowserTestPortOverrides
> > didn't have a member function path_from_chromium_base(), yet it was calling
> it.
> > To get around this, I just instantiate another WebKitFinder and use it to
get
> > the
> > correct directory. I know its roundabout, but the presubmit checks wouldn't
> let
> > me use the better solution.
> 
> That sounds like a bug in pylint/presubmit. If the code actually works, better
> to disable the warning rather than work around it.
> 
> That said, I applied your patch locally and tested this. First I deleted line
29
> and line 50, and that ran cleanly; next I deleted the __init__ method
> altogether, and it still ran cleanly, so perhaps something else is going on?

Done.

[Dirk Pranke, 2014-07-18 16:43:14]

lgtm. let's give this a shot!

https://codereview.chromium.org/378113003/diff/360001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/browser_test_unittest.py (right):

https://codereview.chromium.org/378113003/diff/360001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/browser_test_unittest.py:61:
self.assertFalse(self.make_port().uses_apache())
test_uses_apache() has been removed at tip-of-tree, so you'll want to rebase on
tip-of-tree before landing this.

[ivandavid, 2014-07-18 19:20:06]

The CQ bit was checked by ivandavid@chromium.org

[commit-bot: I haz the power, 2014-07-18 19:21:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ivandavid@chromium.org/378113003/380001

[commit-bot: I haz the power, 2014-07-18 20:31:02]

Change committed as 178486

[abarth-chromium, 2014-07-18 22:30:33]

This CL is on the blamelist for a bunch of different LayoutTest failures on the
bots currently...  Do you mind if I try reverting it?

[blink-reviews, 2014-07-18 22:36:52]

Go for it.


On Fri, Jul 18, 2014 at 3:30 PM, <abarth@chromium.org> wrote:

> This CL is on the blamelist for a bunch of different LayoutTest failures
> on the
> bots currently...  Do you mind if I try reverting it?
>
> https://codereview.chromium.org/378113003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2014-07-18 22:37:35]

A revert of this CL has been created in
https://codereview.chromium.org/398543005/ by abarth@chromium.org.

The reason for reverting is: Might have broken a number of LayoutTests..

[ivandavid, 2014-07-19 00:43:38]

The CQ bit was checked by ivandavid@chromium.org

[commit-bot: I haz the power, 2014-07-19 00:44:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ivandavid@chromium.org/378113003/430001

[Dirk Pranke, 2014-07-19 00:47:44]

On 2014/07/19 00:44:13, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> 
https://chromium-status.appspot.com/cq/ivandavid@chromium.org/378113003/430001

I don't think you can just re-open and re-commit this (I think the patch might
fail). I could be wrong.

General practice would be to upload a new patch explaining that you're
re-landing something, and either wait for me to review it, or at least TBR me if
you had a reasonable belief that I'd be fine with this (which you do).