Issue 4202004: Add named testing interface

dtu

http://codereview.chromium.org/4202004/diff/1/4 File chrome/browser/browser_init.cc (right): http://codereview.chromium.org/4202004/diff/1/4#newcode1025 chrome/browser/browser_init.cc:1025: if (!channel_id.empty() && channel_id[0] == '/') add comments Use: ...

10 years, 1 month ago (2010-10-28 19:17:51 UTC) #1

John Grabowski

Also wait for Pawel feedback http://codereview.chromium.org/4202004/diff/22001/23001 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/4202004/diff/22001/23001#newcode161 chrome/browser/automation/automation_provider.cc:161: new IPC::SyncChannel(channel_id, is_server ? ...

10 years, 1 month ago (2010-10-28 20:43:54 UTC) #2

dtu

PTAL http://codereview.chromium.org/4202004/diff/22001/23001 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/4202004/diff/22001/23001#newcode161 chrome/browser/automation/automation_provider.cc:161: new IPC::SyncChannel(channel_id, is_server ? IPC::Channel::MODE_SERVER : On 2010/10/28 ...

10 years, 1 month ago (2010-10-29 01:07:10 UTC) #3

Nirnimesh

+Scott as reviewer, for reviewing the change to ipc_channel_posix.cc (or please suggest someone else who ...

10 years, 1 month ago (2010-10-29 06:40:17 UTC) #4

Paweł Hajdan Jr.

Do you have some design documents for this new testing interface? I would like to ...

10 years, 1 month ago (2010-10-29 10:05:18 UTC) #5

Do you have some design documents for this new testing interface? I would like
to see a discussion before the implementation.

My general comment is that we should rethink the design of our testing
infrastructure. We have pyauto, webdriver, and the new named_testing_interface.
This is great! However, we cannot get into the trap of download code, that its
design wasn't improved as new features were added.

http://codereview.chromium.org/4202004/diff/1011/34002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/4202004/diff/1011/34002#newcode88
chrome/browser/automation/automation_provider.h:88: // Otherwise, if is_server
is false, it will connect to an anonymous fd
This sounds quite ambiguous. What if is_server=true but channel_id is not
prefixed with a slash?

I've seen Nirnimesh's concern about the change in ipc/ and I agree with it. It's
a very fragile interface.

http://codereview.chromium.org/4202004/diff/1011/34003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34003#newcode1033
chrome/browser/browser_init.cc:1033:
automation->SetExpectedTabCount(expected_tabs);
Why this is only executed for non-named testing interfaces? Is that what you
intended? Also, I think it's making CreateAutomationProvider more tricky,
because now one of its arguments is silently ignored.

http://codereview.chromium.org/4202004/diff/1011/34005
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34005#newcode157
chrome/test/automation/automation_proxy.cc:157: is_server ?
IPC::Channel::MODE_SERVER : IPC::Channel::MODE_CLIENT,
Why do we have two different modes at all?

AutomationProxy assumes it is in the same binary that has launched browser under
test (for an example, see WaitForAppLaunch, but not only).

I wonder that maybe we should make a bigger design change than just sticking one
more boolean in.

http://codereview.chromium.org/4202004/diff/1011/34007
File chrome/test/named_testing_interface/named_interface_test.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34007#newcode9
chrome/test/named_testing_interface/named_interface_test.cc:9: class
NamedInterfaceTest : public UITest {
If it's a UI test, why add a new testing target, rather than adding it to
ui_tests target?

http://codereview.chromium.org/4202004/diff/1011/34007#newcode17
chrome/test/named_testing_interface/named_interface_test.cc:17:
scoped_refptr<BrowserProxy> window_proxy(automation()->GetBrowserWindow(0));
nit: This should be browser_proxy to avoid confusion with WindowProxy.

http://codereview.chromium.org/4202004/diff/1011/34007#newcode21
chrome/test/named_testing_interface/named_interface_test.cc:21:
ASSERT_TRUE(window_proxy->AppendTab(GURL("about:blank")));
Use a constant from chrome/common/url_constants

http://codereview.chromium.org/4202004/diff/1011/34008
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34008#newcode145
chrome/test/ui/ui_test.cc:145: if (use_named_interface_)
This looks bad. Should we have a different test base class rather than a bool
for that?

http://codereview.chromium.org/4202004/diff/1011/34008#newcode188
chrome/test/ui/ui_test.cc:188: if (use_named_interface_)
Again, this is going to cause sadness in the future (we need to check
use_named_interface_ in multiple places).

http://codereview.chromium.org/4202004/diff/1011/34008#newcode727
chrome/test/ui/ui_test.cc:727:
command_line.AppendSwitchASCII(switches::kTestingChannelID, channel_id);
This is very similar to the code below. Can we unify it?

sky

On Thu, Oct 28, 2010 at 11:40 PM, <nirnimesh@chromium.org> wrote: > +Scott as reviewer, for ...

10 years, 1 month ago (2010-10-29 14:59:41 UTC) #6

John Grabowski

At a higher level, Pawel has a good thought that a better description of the ...

10 years, 1 month ago (2010-10-31 05:15:22 UTC) #7

dtu

This named testing interface is not an alternative to PyAuto or WebDriver, but rather is ...

10 years, 1 month ago (2010-11-01 19:56:33 UTC) #8

This named testing interface is not an alternative to PyAuto or WebDriver, but
rather is used in the underlying layer beneath both of these testing frameworks
for communication between the browser and the test. This named interface will
allow you to connect both PyAuto and WebDriver to the same browser instance.
I've updated the change description to reflect this.

I've changed the IPC stuff to use the Mode parameter rather than checking the
channel_id for "/". This also makes the AutomationProxy/Provider interfaces
cleaner. But this solution is still not ideal; I'm going to defer to further
suggestions.

I'm also going to look into Pawel's comments that the changes in UITest be
refactored into a separate class. It is true that having all these
use_named_interface checks all over the place is messy, but all functional tests
inherit from UITest, and being able to change any test between unnamed and named
by flipping one boolean is nice. I'm not yet sure how one would separate the
class to make it cleaner.

http://codereview.chromium.org/4202004/diff/1011/34001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34001#newcode162
chrome/browser/automation/automation_provider.cc:162: is_server ?
IPC::Channel::MODE_SERVER : IPC::Channel::MODE_CLIENT,
On 2010/10/29 06:40:18, Nirnimesh wrote:
> nit: you probably should remove the space before ? and :

Done.

http://codereview.chromium.org/4202004/diff/1011/34002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/4202004/diff/1011/34002#newcode88
chrome/browser/automation/automation_provider.h:88: // Otherwise, if is_server
is false, it will connect to an anonymous fd
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> This sounds quite ambiguous. What if is_server=true but channel_id is not
> prefixed with a slash?
> 
> I've seen Nirnimesh's concern about the change in ipc/ and I agree with it.
It's
> a very fragile interface.

Haha, you're right; it's ambiguous because the behavior is undefined. I've
changed the ipc/ stuff so that this is unnecessary.

http://codereview.chromium.org/4202004/diff/1011/34002#newcode89
chrome/browser/automation/automation_provider.h:89: // that is passed from the
parent process, so the AutomationProxy must be
On 2010/10/29 06:40:18, Nirnimesh wrote:
> The context to 'parent' is not clear. re-word in terms of client/server end of
> the testing IPC channel

Done.

http://codereview.chromium.org/4202004/diff/1011/34002#newcode91
chrome/browser/automation/automation_provider.h:91: // TODO(dtu): Right now the
channel_id determines whether the socket
On 2010/10/31 05:15:23, John Grabowski wrote:
> Please fix this TODO now, not later.

Done.

http://codereview.chromium.org/4202004/diff/1011/34003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34003#newcode1025
chrome/browser/browser_init.cc:1025: // If the channel_id starts with
NamedTestingInterface:, create a named
On 2010/10/29 06:40:18, Nirnimesh wrote:
> put quotes around "NamedTestingInterface:"

Done.

http://codereview.chromium.org/4202004/diff/1011/34003#newcode1026
chrome/browser/browser_init.cc:1026: // IPC and listen on it, rather than trying
to connect to an existing IPC.
On 2010/10/29 06:40:18, Nirnimesh wrote:
> IPC server and listen on it, else connect as client to an existing IPC server

Done.

http://codereview.chromium.org/4202004/diff/1011/34003#newcode1029
chrome/browser/browser_init.cc:1029: std::string pipe_path =
channel_id.substr(named_prefix.length());
On 2010/10/29 06:40:18, Nirnimesh wrote:
> assert pipe_path.length() > 0

Done.

http://codereview.chromium.org/4202004/diff/1011/34003#newcode1033
chrome/browser/browser_init.cc:1033:
automation->SetExpectedTabCount(expected_tabs);
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> Why this is only executed for non-named testing interfaces? Is that what you
> intended? Also, I think it's making CreateAutomationProvider more tricky,
> because now one of its arguments is silently ignored.

It is intentional. It seems like SetExpectedTabCount is a misnomer; what it
really does is send a message to notify the AutomationProxy when the browser has
finished loading all of its initial tabs. For the named interface we assume that
the browser has already been loaded before we connect to it with the
AutomationProxy, so this isn't useful anymore.

As for silently ignoring an argument: yes, but I don't know what would be a
better alternative.

http://codereview.chromium.org/4202004/diff/1011/34005
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34005#newcode157
chrome/test/automation/automation_proxy.cc:157: is_server ?
IPC::Channel::MODE_SERVER : IPC::Channel::MODE_CLIENT,
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> Why do we have two different modes at all?
> 
> AutomationProxy assumes it is in the same binary that has launched browser
under
> test (for an example, see WaitForAppLaunch, but not only).
> 
> I wonder that maybe we should make a bigger design change than just sticking
one
> more boolean in.

WaitForAppLaunch() doesn't make that assumption. It relies on a message sent
over the IPC channel from the AutomationProvider to signal AppLaunch. I don't
know if there are other things that make that assumption, but if they do, they
probably shouldn't.

There is one place in AutomationProvider where that assumption is made, and
that's establishing the initial connection. But the named testing interface
doesn't use that part of the code.

http://codereview.chromium.org/4202004/diff/1011/34007
File chrome/test/named_testing_interface/named_interface_test.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34007#newcode6
chrome/test/named_testing_interface/named_interface_test.cc:6: 
On 2010/10/29 06:40:18, Nirnimesh wrote:
> Please add some comments explaining this test

Done.

http://codereview.chromium.org/4202004/diff/1011/34007#newcode9
chrome/test/named_testing_interface/named_interface_test.cc:9: class
NamedInterfaceTest : public UITest {
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> If it's a UI test, why add a new testing target, rather than adding it to
> ui_tests target?

This is not really a UI test; it's a test of an underlying interface that UI
tests can use. I also noticed that the UITest class is not limited to just UI
tests. Most, if not all, functional tests inherit from UITest.

http://codereview.chromium.org/4202004/diff/1011/34007#newcode17
chrome/test/named_testing_interface/named_interface_test.cc:17:
scoped_refptr<BrowserProxy> window_proxy(automation()->GetBrowserWindow(0));
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> nit: This should be browser_proxy to avoid confusion with WindowProxy.

Done.

http://codereview.chromium.org/4202004/diff/1011/34007#newcode21
chrome/test/named_testing_interface/named_interface_test.cc:21:
ASSERT_TRUE(window_proxy->AppendTab(GURL("about:blank")));
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> Use a constant from chrome/common/url_constants

Done.

http://codereview.chromium.org/4202004/diff/1011/34008
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34008#newcode145
chrome/test/ui/ui_test.cc:145: if (use_named_interface_)
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> This looks bad. Should we have a different test base class rather than a bool
> for that?

Maybe. This check does appear multiple times in this class. I am looking into
it.

http://codereview.chromium.org/4202004/diff/1011/34008#newcode188
chrome/test/ui/ui_test.cc:188: if (use_named_interface_)
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> Again, this is going to cause sadness in the future (we need to check
> use_named_interface_ in multiple places).

Same as above.

http://codereview.chromium.org/4202004/diff/1011/34008#newcode726
chrome/test/ui/ui_test.cc:726: channel_id =
channel_id.append(kNamedPrefix).append(channel_id_);
On 2010/10/29 06:40:18, Nirnimesh wrote:
> This should be done in CreateAutomationProxy above and below

No, the kNamedPrefix is only prepended for sending the command line switch to
the browser. Internally, this AutomationProxy is using just the path as the
channel_id.

http://codereview.chromium.org/4202004/diff/1011/34008#newcode727
chrome/test/ui/ui_test.cc:727:
command_line.AppendSwitchASCII(switches::kTestingChannelID, channel_id);
On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> This is very similar to the code below. Can we unify it?

Done.

http://codereview.chromium.org/4202004/diff/1011/34010
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34010#newcode278
ipc/ipc_channel_posix.cc:278: (!channel_id.empty() && channel_id[0] == '/')),
On 2010/10/29 06:40:18, Nirnimesh wrote:
> This is the part I'm not very sure about. Using '/' as the first character as
> the differentiator for named vs anonymous channel seems hacky to me.
> I understand that we cannot pass the kIPCUseFIFO switch because that'll make
all
> IPC channel named, not just the testing interface's.
> 
> I'll defer this for suggestions from someone with better taste.

I agree that it is hacky. I've changed it to use the Mode parameter instead,
which is also hacky but somewhat less so. I will wait for further suggestions on
what would be a better alternative.

Nirnimesh

-sky +evan as reviewer for ipc_channel_posix changes http://codereview.chromium.org/4202004/diff/1011/34007 File chrome/test/named_testing_interface/named_interface_test.cc (right): http://codereview.chromium.org/4202004/diff/1011/34007#newcode9 chrome/test/named_testing_interface/named_interface_test.cc:9: class NamedInterfaceTest ...

10 years, 1 month ago (2010-11-01 23:59:24 UTC) #9

dtu

http://codereview.chromium.org/4202004/diff/1011/34007 File chrome/test/named_testing_interface/named_interface_test.cc (right): http://codereview.chromium.org/4202004/diff/1011/34007#newcode9 chrome/test/named_testing_interface/named_interface_test.cc:9: class NamedInterfaceTest : public UITest { On 2010/11/01 23:59:24, ...

10 years, 1 month ago (2010-11-02 00:46:14 UTC) #10

Nirnimesh

Code I commented LGTM, after the one comment below. Let's wait for someone for the ...

10 years, 1 month ago (2010-11-02 01:12:16 UTC) #11

Paweł Hajdan Jr.

I'd really like to know the rationale behind this change in more detail, and the ...

10 years, 1 month ago (2010-11-02 08:16:28 UTC) #12

I'd really like to know the rationale behind this change in more detail, and the
future plans. It's one of the factors that affects the best way to design it.

Here are my general concerns about this change:

1. We're adding a lot of new interfaces without rethinking the design.
2. There is no explanation why we want both PyAuto and WebDriver to connect to
the same browser instance, and why it is done that way.
3. There is no explanation what are the future plans. Should all testing
frameworks switch to named testing interface? If not, how do we design it
cleanly?
4. Why do we support both client and server mode for AutomationProvider? Is this
only temporary? If so, what is the plan for the future?
5. How do we ensure necessary synchronization between PyAuto and WebDriver?
Currently there are assumptions in the automation code that may break if more
than one "driver" manipulates the browser.

http://codereview.chromium.org/4202004/diff/1011/34003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/1011/34003#newcode1033
chrome/browser/browser_init.cc:1033:
automation->SetExpectedTabCount(expected_tabs);
On 2010/11/01 19:56:34, Dave Tu wrote:
> On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
> > Why this is only executed for non-named testing interfaces? Is that what you
> > intended? Also, I think it's making CreateAutomationProvider more tricky,
> > because now one of its arguments is silently ignored.
> 
> It is intentional. It seems like SetExpectedTabCount is a misnomer; what it
> really does is send a message to notify the AutomationProxy when the browser
has
> finished loading all of its initial tabs. For the named interface we assume
that
> the browser has already been loaded before we connect to it with the
> AutomationProxy, so this isn't useful anymore.

How do you ensure that assumption is valid?

> As for silently ignoring an argument: yes, but I don't know what would be a
> better alternative.

Redesign if needed.

http://codereview.chromium.org/4202004/diff/55001/56003#newcode1025
chrome/browser/browser_init.cc:1025: // If the channel_id starts with
NamedTestingInterface:, create a named
Again, why do we support *two* modes of operation?

If named testing interface is a successor/unifier of our current infrastructure,
why not agree on one mode for everything?

http://codereview.chromium.org/4202004/diff/55001/56005
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/4202004/diff/55001/56005#newcode157
chrome/test/automation/automation_proxy.cc:157: use_named_interface?
IPC::Channel::MODE_SERVER
nit: Space between _interface and ? .

http://codereview.chromium.org/4202004/diff/55001/56008
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/55001/56008#newcode145
chrome/test/ui/ui_test.cc:145: if (use_named_interface_)
Those checks scattered across ui_test are just a no-go. It is hard for me to
suggest alternatives without a detailed description of what you're trying to do
in this patch, and what are the future plans.

krisr

So there are two reasons for this change. One I know a lot about one, ...

10 years, 1 month ago (2010-11-02 08:39:51 UTC) #13

So there are two reasons for this change.  One I know a lot about one, the
other I don't.
  1.) So WebDriver and PyAuto can connect to the same instance - this I
leave to jrg to explain more
  2.) ChromeOS automation - this I can discuss.  On ChromeOS Chrome is
already running and it controls everything on the system.  We need a way to
connect to the current running Chrome (the one that is running the battery
UI, network UI, clock, login screen, lock screen, etc.).  The current model
of the test launching Chrome will not work for ChromeOS.  This is why we
need this feature.

To answer 3 and 4, the reason we support both is because Chrome is running
in a much different way on ChromeOS than it is on other platforms, as
described above.  It is a large enough use case that adding this new
functionality is reasonable.  The future plan will be to support both.  In
my mind I think that most tests that are meant to run on the 3 client
platforms can continue to use the current model.  However as we create
PyAuto hooks for ChromeOS features (this CL is the gate to starting all of
that work) those tests will be using this new model.  We are already running
over 100 of the current PyAuto tests on ChromeOS, those tests create a new
instance of Chrome and go.  Since they are not testing ChromeOS specific
features this is acceptable.  I mention this to be clear that new Chrome
core functional tests do not have to use this new interface model, even when
running on ChromeOS.

I think the goal of 5 is a nice to have, and may become more important when
WebDriver starts making inroads into the Chrome testing infrastructure.  I
would not gate this CL on that conversation.

Let me know if you have more questions,
Kris

On Tue, Nov 2, 2010 at 1:16 AM, <phajdan.jr@chromium.org> wrote:

> I'd really like to know the rationale behind this change in more detail,
> and the
> future plans. It's one of the factors that affects the best way to design
> it.
>
> Here are my general concerns about this change:
>
> 1. We're adding a lot of new interfaces without rethinking the design.
> 2. There is no explanation why we want both PyAuto and WebDriver to connect
> to
> the same browser instance, and why it is done that way.
> 3. There is no explanation what are the future plans. Should all testing
> frameworks switch to named testing interface? If not, how do we design it
> cleanly?
> 4. Why do we support both client and server mode for AutomationProvider? Is
> this
> only temporary? If so, what is the plan for the future?
> 5. How do we ensure necessary synchronization between PyAuto and WebDriver?
> Currently there are assumptions in the automation code that may break if
> more
> than one "driver" manipulates the browser.
>
>
>
> http://codereview.chromium.org/4202004/diff/1011/34003
> File chrome/browser/browser_init.cc (right):
>
>
> http://codereview.chromium.org/4202004/diff/1011/34003#newcode1033
> chrome/browser/browser_init.cc:1033:
> automation->SetExpectedTabCount(expected_tabs);
> On 2010/11/01 19:56:34, Dave Tu wrote:
>
>> On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
>> > Why this is only executed for non-named testing interfaces? Is that
>>
> what you
>
>> > intended? Also, I think it's making CreateAutomationProvider more
>>
> tricky,
>
>> > because now one of its arguments is silently ignored.
>>
>
>  It is intentional. It seems like SetExpectedTabCount is a misnomer;
>>
> what it
>
>> really does is send a message to notify the AutomationProxy when the
>>
> browser has
>
>> finished loading all of its initial tabs. For the named interface we
>>
> assume that
>
>> the browser has already been loaded before we connect to it with the
>> AutomationProxy, so this isn't useful anymore.
>>
>
> How do you ensure that assumption is valid?
>
>
>  As for silently ignoring an argument: yes, but I don't know what would
>>
> be a
>
>> better alternative.
>>
>
> Redesign if needed.
>
> http://codereview.chromium.org/4202004/diff/55001/56003#newcode1025
>
> chrome/browser/browser_init.cc:1025: // If the channel_id starts with
> NamedTestingInterface:, create a named
> Again, why do we support *two* modes of operation?
>
> If named testing interface is a successor/unifier of our current
> infrastructure, why not agree on one mode for everything?
>
> http://codereview.chromium.org/4202004/diff/55001/56005
> File chrome/test/automation/automation_proxy.cc (right):
>
> http://codereview.chromium.org/4202004/diff/55001/56005#newcode157
> chrome/test/automation/automation_proxy.cc:157: use_named_interface?
> IPC::Channel::MODE_SERVER
> nit: Space between _interface and ? .
>
> http://codereview.chromium.org/4202004/diff/55001/56008
> File chrome/test/ui/ui_test.cc (right):
>
> http://codereview.chromium.org/4202004/diff/55001/56008#newcode145
>
> chrome/test/ui/ui_test.cc:145: if (use_named_interface_)
> Those checks scattered across ui_test are just a no-go. It is hard for
> me to suggest alternatives without a detailed description of what you're
> trying to do in this patch, and what are the future plans.
>
>
> http://codereview.chromium.org/4202004/show
>

Paweł Hajdan Jr.

On Tue, Nov 2, 2010 at 09:39, Kris Rambish <krisr@chromium.org> wrote: > So there are ...

10 years, 1 month ago (2010-11-02 12:42:23 UTC) #14

On Tue, Nov 2, 2010 at 09:39, Kris Rambish <krisr@chromium.org> wrote:

> So there are two reasons for this change.  One I know a lot about one, the
> other I don't.
>   1.) So WebDriver and PyAuto can connect to the same instance - this I
> leave to jrg to explain more
>

Okay. The answer to this question seems important.

>   2.) ChromeOS automation - this I can discuss.  On ChromeOS Chrome is
> already running and it controls everything on the system.  We need a way to
> connect to the current running Chrome (the one that is running the battery
> UI, network UI, clock, login screen, lock screen, etc.).  The current model
> of the test launching Chrome will not work for ChromeOS.  This is why we
> need this feature.
>

Okay. However, for simplicity it would be better to switch the entire
automation framework to that model.

Moreover, it's not obvious whether the proposed solution (a named pipe
created by the browser process) is right, and the only possible for the
above problem. We have to pass the pipe name between the browser and test
process anyway, right?

> To answer 3 and 4, the reason we support both is because Chrome is running
> in a much different way on ChromeOS than it is on other platforms, as
> described above.  It is a large enough use case that adding this new
> functionality is reasonable.
>

Alternatively, we should also consider changing the design of ChromeOS, if
this is making testing harder.

> The future plan will be to support both.
>

This is worrying and adding to the confusion. I'd prefer to switch
everything to one model, if not now, then in the future.

Anyway, this does not answer 3 and 4 fully.

3. There is no explanation what are the future plans. Should all testing
> frameworks switch to named testing interface? *If not, how do we design it
> cleanly?*

4. *Why do we support both client and server mode for
AutomationProvider?*Is this
> only temporary? If so, what is the plan for the future?

Is there any other reason to support both client and server mode than we can
and existing tests use "the old" interface?

dtu

On Tue, Nov 2, 2010 at 1:16 AM, <phajdan.jr@chromium.org> wrote: > I'd really like to ...

10 years, 1 month ago (2010-11-03 07:19:42 UTC) #15

On Tue, Nov 2, 2010 at 1:16 AM, <phajdan.jr@chromium.org> wrote:

> I'd really like to know the rationale behind this change in more detail,
> and the
> future plans. It's one of the factors that affects the best way to design
> it.
>
> Here are my general concerns about this change:
>
> 1. We're adding a lot of new interfaces without rethinking the design.
>

Just one :)

2. There is no explanation why we want both PyAuto and WebDriver to connect
> to
> the same browser instance, and why it is done that way.
>

From what I understand, it is because PyAuto and WebDriver have different,
somewhat complementing feature sets. The example that jrg gave me was that
if you have a Selenium test navigating a web page and the browser crashes,
Selenium would be unable to tell you more information but PyAuto would be
able to print a stack trace.

3. There is no explanation what are the future plans. Should all testing
> frameworks switch to named testing interface? If not, how do we design it
> cleanly?
>

No, and a big reason is compatibility with existing tests. I'm not really
sure what kind of assumptions tests make as to the testing interface. E.g.
some tests might assume that the browser closes itself after the test
disconnects, but with the named interface that is not the case. There are
hundreds of tests that we have to be careful not to break. I'm not sure if
there are other major reasons besides compatibility, I'll defer to jrg.

As for how we design it cleanly, I don't have an answer for that. That's
what we figure out next.

4. Why do we support both client and server mode for AutomationProvider? Is
> this
> only temporary? If so, what is the plan for the future?
>

Let me define server as the one who initializes the channel, and thus is
whoever is launched first. In the "old" way the Provider is the client and
the Proxy is the server. This is fine as is, and has worked so far because
it is always the test that runs first and then launches the browser.
However, Chrome OS by its nature launches a highly customized version of
Chrome at startup. Since Chrome is launched first, it becomes the server and
has to initialize the channel. Changing the design of Chrome OS in this
respect is not really an option.

So in the end there are both client and server modes, with different use
cases, and they are both here to stay.

5. How do we ensure necessary synchronization between PyAuto and WebDriver?
> Currently there are assumptions in the automation code that may break if
> more
> than one "driver" manipulates the browser.

For this changelist, having multiple connections is not supported yet. It is
a worry, but as krisr said, shouldn't gate this CL.

If anything was unclear/unanswered or you still have more questions, please
feel free to ask.

>
>
> http://codereview.chromium.org/4202004/diff/1011/34003
> File chrome/browser/browser_init.cc (right):
>
>
> http://codereview.chromium.org/4202004/diff/1011/34003#newcode1033
> chrome/browser/browser_init.cc:1033:
> automation->SetExpectedTabCount(expected_tabs);
> On 2010/11/01 19:56:34, Dave Tu wrote:
>
>> On 2010/10/29 10:05:19, Paweł Hajdan Jr. wrote:
>> > Why this is only executed for non-named testing interfaces? Is that
>>
> what you
>
>> > intended? Also, I think it's making CreateAutomationProvider more
>>
> tricky,
>
>> > because now one of its arguments is silently ignored.
>>
>
>  It is intentional. It seems like SetExpectedTabCount is a misnomer;
>>
> what it
>
>> really does is send a message to notify the AutomationProxy when the
>>
> browser has
>
>> finished loading all of its initial tabs. For the named interface we
>>
> assume that
>
>> the browser has already been loaded before we connect to it with the
>> AutomationProxy, so this isn't useful anymore.
>>
>
> How do you ensure that assumption is valid?
>
>
>  As for silently ignoring an argument: yes, but I don't know what would
>>
> be a
>
>> better alternative.
>>
>
> Redesign if needed.
>
> http://codereview.chromium.org/4202004/diff/55001/56003#newcode1025
>
> chrome/browser/browser_init.cc:1025: // If the channel_id starts with
> NamedTestingInterface:, create a named
> Again, why do we support *two* modes of operation?
>
> If named testing interface is a successor/unifier of our current
> infrastructure, why not agree on one mode for everything?
>
> http://codereview.chromium.org/4202004/diff/55001/56005
> File chrome/test/automation/automation_proxy.cc (right):
>
> http://codereview.chromium.org/4202004/diff/55001/56005#newcode157
> chrome/test/automation/automation_proxy.cc:157: use_named_interface?
> IPC::Channel::MODE_SERVER
> nit: Space between _interface and ? .
>
> http://codereview.chromium.org/4202004/diff/55001/56008
> File chrome/test/ui/ui_test.cc (right):
>
> http://codereview.chromium.org/4202004/diff/55001/56008#newcode145
>
> chrome/test/ui/ui_test.cc:145: if (use_named_interface_)
> Those checks scattered across ui_test are just a no-go. It is hard for
> me to suggest alternatives without a detailed description of what you're
> trying to do in this patch, and what are the future plans.
>
>
> http://codereview.chromium.org/4202004/show
>

Paweł Hajdan Jr.

On Wed, Nov 3, 2010 at 08:19, Dave Tu <dtu@chromium.org> wrote: > On Tue, Nov ...

10 years, 1 month ago (2010-11-03 07:39:50 UTC) #16

On Wed, Nov 3, 2010 at 08:19, Dave Tu <dtu@chromium.org> wrote:

> On Tue, Nov 2, 2010 at 1:16 AM, <phajdan.jr@chromium.org> wrote:
>
>> I'd really like to know the rationale behind this change in more detail,
>> and the
>> future plans. It's one of the factors that affects the best way to design
>> it.
>>
>> Here are my general concerns about this change:
>>
>> 1. We're adding a lot of new interfaces without rethinking the design.
>>
>
> Just one :)
>

Well, we haven't really changed the design for PyAuto and WebDriver, and we
already have problems with that (i.e. people inherit privately from
UITestBase). Now we're adding yet another thing, and it's time to think more
about that.

> 2. There is no explanation why we want both PyAuto and WebDriver to connect
>> to
>> the same browser instance, and why it is done that way.
>>
>
> From what I understand, it is because PyAuto and WebDriver have different,
> somewhat complementing feature sets. The example that jrg gave me was that
> if you have a Selenium test navigating a web page and the browser crashes,
> Selenium would be unable to tell you more information but PyAuto would be
> able to print a stack trace.
>

Using PyAuto just to get stack traces doesn't make sense to me. PyAuto is
more than a launcher, and to get a stack trace we only need a launcher.

>  3. There is no explanation what are the future plans. Should all testing
>> frameworks switch to named testing interface? If not, how do we design it
>> cleanly?
>>
>
> No, and a big reason is compatibility with existing tests.
>

I don't care about compatibility with existing tests as much as I do about
the maintainability of the testing infrastructure. We can convert the tests
if needed (I think we shouldn't need to convert individual tests).

> I'm not really sure what kind of assumptions tests make as to the testing
> interface. E.g. some tests might assume that the browser closes itself after
> the test disconnects, but with the named interface that is not the case.
>

The main assumption is nothing else is manipulating the browser. When the
automation waits for an event, it assumes the even will be triggered by one
of its actions. If something else sends commands to the browser as well,
both automation provider may get confused. It is possible to solve this
technically, but we should be prepared for it.

> As for how we design it cleanly, I don't have an answer for that. That's
> what we figure out next.
>

Can we figure it out before this change lands? I'm afraid that after it
lands there will be huge incentive to continue building things on top of
this, rather than cleaning it up.

> 4. Why do we support both client and server mode for AutomationProvider? Is
>> this
>> only temporary? If so, what is the plan for the future?
>>
>
> So in the end there are both client and server modes, with different use
> cases, and they are both here to stay.
>

This does not answer the question why we need both. Even if we need server
mode for ChromeOS, why not convert everything to that mode?

Nirnimesh

> Well, we haven't really changed the design for PyAuto and WebDriver, and we > ...

10 years, 1 month ago (2010-11-03 20:25:39 UTC) #17

> Well, we haven't really changed the design for PyAuto and WebDriver, and we
> already have problems with that (i.e. people inherit privately from
> UITestBase). Now we're adding yet another thing, and it's time to think more
> about that.

Let's back up a step, and break it into two parts:
1. The objective: have a named testing interface
     - to control first chrome on chromeos. When chromeos boots up you have a
chrome up and running. The first chrome manages the login window, and other
chormeos specific ui
     - PyAuto and webdriver can control the same chrome instance. Why is it
needed you ask. Well, PyAuto be design doesn't touch the web view portion,
webdriver by design concentrates on the webview portion. We want to automate
use-cases where driving the browser is done by PyAuto, but interacting with the
webview is done using webdriver. So these two tools don't control the browser at
the same time, but they take turns, and we have strong incentives to do that.
Each of these tools currently assume that it's the one who launched the browser
process to begin with -- this is what we want to break such that they can
interoperate.

Please let me know fi you have any questions with the objectives.

2. The implementation
The test driver (ui_tests, pyauto, webdriver) currently keeps the "server" side
of the automation IPC channel, and fires the browser which acts at the client
side. Once a connection is established there isn't much difference between the
server and the client; whichever portion starts up first is the server.
Converting the framework such that the browser is always the server side would
break several tests but most notably the startup_tests which is time sensitive.
Therefore we want to preserve as much of that as we can, but introduce a
reversed channel for catering to the new situations. The decision of which side
is the server then lies with the test tool -- I don't see all the usecases being
solved with just one direction, hence the new flags.

> 
> 
> > 2. There is no explanation why we want both PyAuto and WebDriver to connect
> >> to
> >> the same browser instance, and why it is done that way.
> >>
> >
> > From what I understand, it is because PyAuto and WebDriver have different,
> > somewhat complementing feature sets. The example that jrg gave me was that
> > if you have a Selenium test navigating a web page and the browser crashes,
> > Selenium would be unable to tell you more information but PyAuto would be
> > able to print a stack trace.
> >
> 
> Using PyAuto just to get stack traces doesn't make sense to me. PyAuto is
> more than a launcher, and to get a stack trace we only need a launcher.

I hope I've explained this above.

> 
> 
> >  3. There is no explanation what are the future plans. Should all testing
> >> frameworks switch to named testing interface? If not, how do we design it
> >> cleanly?
> >>
> >
> > No, and a big reason is compatibility with existing tests.
> >
> 
> I don't care about compatibility with existing tests as much as I do about
> the maintainability of the testing infrastructure. We can convert the tests
> if needed (I think we shouldn't need to convert individual tests).

We do care about compatibility. The perf graphs should not get affected by this
CL.

> 
> 
> > I'm not really sure what kind of assumptions tests make as to the testing
> > interface. E.g. some tests might assume that the browser closes itself after
> > the test disconnects, but with the named interface that is not the case.
> >
> 
> The main assumption is nothing else is manipulating the browser. When the
> automation waits for an event, it assumes the even will be triggered by one
> of its actions. If something else sends commands to the browser as well,
> both automation provider may get confused. It is possible to solve this
> technically, but we should be prepared for it.
> 
> 
> > As for how we design it cleanly, I don't have an answer for that. That's
> > what we figure out next.

The understanding right now is that only one tool controls chrome at a time. I
can see problems if they start talking at the same time, so I want to make it
explicit. Even though multiple tools might be driving at different times, they
should not conflict.

> >
> 
> Can we figure it out before this change lands? I'm afraid that after it
> lands there will be huge incentive to continue building things on top of
> this, rather than cleaning it up.
> 
> 
> > 4. Why do we support both client and server mode for AutomationProvider? Is
> >> this
> >> only temporary? If so, what is the plan for the future?
> >>
> >
> > So in the end there are both client and server modes, with different use
> > cases, and they are both here to stay.
> >
> 
> This does not answer the question why we need both. Even if we need server
> mode for ChromeOS, why not convert everything to that mode?
>

dtu

http://codereview.chromium.org/4202004/diff/55001/56004 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/4202004/diff/55001/56004#newcode509 chrome/chrome_tests.gypi:509: 'test/named_testing_interface/named_interface_test.cc', On 2010/11/02 01:12:17, Nirnimesh wrote: > follow the ...

10 years, 1 month ago (2010-11-04 00:39:56 UTC) #18

Paweł Hajdan Jr.

+jabdelmalek for ipc comments Nirnimesh, thanks for explaining, especially the perf test parts. Please note ...

10 years, 1 month ago (2010-11-04 07:58:26 UTC) #19

dtu

I'm sorry, I've had to revert the change that moved the named socket code out ...

10 years, 1 month ago (2010-11-05 03:05:01 UTC) #20

I'm sorry, I've had to revert the change that moved the named socket code out of
UITest and into its own class. It was not working very well because PyAuto and
other frameworks that don't use gtest should inherit from UIBaseTest, so
UINamedTest would have had to also. But then it wouldn't be possible to create
tests that use both the named interface and gtest. Being able to switch between
named and unnamed by flipping a boolean is more useful.

I think there is still a reason
 why we are unable to convert all the tests to have the browser as the server,
and that is that we have no way of knowing when the browser has created the
named socket is ready to accept incoming connections. We could use a polling
loop to check the existence of the file, but that would throw off the timing of
the startup_tests.

http://codereview.chromium.org/4202004/diff/98003/77008
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77008#newcode1034
chrome/browser/browser_init.cc:1034:
automation->SetExpectedTabCount(expected_tabs);
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> So now we are back with this ignored parameter problem. Should we make
> expected_tabs a constructor parameter of AutomationProvider?
> 
> Even if the browser is launched in server mode, we should wait for the initial
> pages to load to start the tests in a known browser state.
> 
> Alternatively, maybe we should just move this logic into AutomationProvider?
If
> SetExpectedTabCount is an internal thing, that is a good start.

I changed AutomationProvider around a bit so that this parameter is used either
way. This also removes the assumption that the browser is already finished
initial loading when the AutomationProxy connects to it.

http://codereview.chromium.org/4202004/diff/98003/77011
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/4202004/diff/98003/77011#newcode65
chrome/test/automation/automation_proxy.h:65: static std::string
GenerateChannelID();
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> Why don't we make it part of InitializeChannel? GenerateChannelID and
> InitializeChannel seem to be always called together.

They aren't always called together in UINamedTest; in the case of a named
channel, the channel id comes from a constant instead of from
GenerateChannelID().

http://codereview.chromium.org/4202004/diff/98003/77012
File chrome/test/ui/named_interface_uitest.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77012#newcode8
chrome/test/ui/named_interface_uitest.cc:8: // The named testing interface
enables the use of a named socket for controlling
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: Would this comment make more sense directly above TEST_F?

Done. You're right, the first part applies to the entire set of tests but the
second part only applies to that one test.

http://codereview.chromium.org/4202004/diff/98003/77012#newcode21
chrome/test/ui/named_interface_uitest.cc:21: void RunNamedInterfaceTest(int
num_tabs) {
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: Why not put this code directly inside TEST_F? This method has only one
> caller now.

Done.

http://codereview.chromium.org/4202004/diff/98003/77013
File chrome/test/ui/ui_named_test.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77013#newcode12
chrome/test/ui/ui_named_test.cc:12:
TestTimeouts::command_execution_timeout_ms()));
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: Explicitly #include TestTimeouts. Also indent +4.

Done. Fixed in UITestBase also.

http://codereview.chromium.org/4202004/diff/98003/77013#newcode14
chrome/test/ui/ui_named_test.cc:14: ASSERT_EQ(AUTOMATION_SUCCESS,
connection_->WaitForAppLaunch())
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> This duplicates code from UITestBase::LaunchBrowserAndServer. Please share it.

Moved back into UITestBase. See message.

http://codereview.chromium.org/4202004/diff/98003/77013#newcode20
chrome/test/ui/ui_named_test.cc:20: AutomationProxy*
UINamedTest::CreateAutomationProxy(int execution_timeout) {
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> This duplicates code from UITestBase::CreateAutomationProxy.

Moved back into UITestBase. See message.

http://codereview.chromium.org/4202004/diff/98003/77015
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77015#newcode63
chrome/test/ui/ui_test.cc:63: static const std::string kNamedPrefix =
"NamedTestingInterface:";
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> This constant is now duplicated throughout the code base. That's bad.

It's used in ui_test.cc and browser_init.cc. What would be a better place for
it? chrome_constants.h?

http://codereview.chromium.org/4202004/diff/98003/77015#newcode704
chrome/test/ui/ui_test.cc:704: std::string channel_id;
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: huh?

Whoops.

http://codereview.chromium.org/4202004/diff/98003/77015#newcode784
chrome/test/ui/ui_test.cc:784: wait, process);
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: indentation.

Done.

dtu

I'm sorry, I've had to revert the change that moved the named socket code out ...

10 years, 1 month ago (2010-11-05 03:05:01 UTC) #21

I'm sorry, I've had to revert the change that moved the named socket code out of
UITest and into its own class. It was not working very well because PyAuto and
other frameworks that don't use gtest should inherit from UIBaseTest, so
UINamedTest would have had to also. But then it wouldn't be possible to create
tests that use both the named interface and gtest. Being able to switch between
named and unnamed by flipping a boolean is more useful.

I think there is still a reason
 why we are unable to convert all the tests to have the browser as the server,
and that is that we have no way of knowing when the browser has created the
named socket is ready to accept incoming connections. We could use a polling
loop to check the existence of the file, but that would throw off the timing of
the startup_tests.

http://codereview.chromium.org/4202004/diff/98003/77008
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77008#newcode1034
chrome/browser/browser_init.cc:1034:
automation->SetExpectedTabCount(expected_tabs);
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> So now we are back with this ignored parameter problem. Should we make
> expected_tabs a constructor parameter of AutomationProvider?
> 
> Even if the browser is launched in server mode, we should wait for the initial
> pages to load to start the tests in a known browser state.
> 
> Alternatively, maybe we should just move this logic into AutomationProvider?
If
> SetExpectedTabCount is an internal thing, that is a good start.

I changed AutomationProvider around a bit so that this parameter is used either
way. This also removes the assumption that the browser is already finished
initial loading when the AutomationProxy connects to it.

http://codereview.chromium.org/4202004/diff/98003/77011
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/4202004/diff/98003/77011#newcode65
chrome/test/automation/automation_proxy.h:65: static std::string
GenerateChannelID();
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> Why don't we make it part of InitializeChannel? GenerateChannelID and
> InitializeChannel seem to be always called together.

They aren't always called together in UINamedTest; in the case of a named
channel, the channel id comes from a constant instead of from
GenerateChannelID().

http://codereview.chromium.org/4202004/diff/98003/77012
File chrome/test/ui/named_interface_uitest.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77012#newcode8
chrome/test/ui/named_interface_uitest.cc:8: // The named testing interface
enables the use of a named socket for controlling
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: Would this comment make more sense directly above TEST_F?

Done. You're right, the first part applies to the entire set of tests but the
second part only applies to that one test.

http://codereview.chromium.org/4202004/diff/98003/77012#newcode21
chrome/test/ui/named_interface_uitest.cc:21: void RunNamedInterfaceTest(int
num_tabs) {
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: Why not put this code directly inside TEST_F? This method has only one
> caller now.

Done.

http://codereview.chromium.org/4202004/diff/98003/77013
File chrome/test/ui/ui_named_test.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77013#newcode12
chrome/test/ui/ui_named_test.cc:12:
TestTimeouts::command_execution_timeout_ms()));
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: Explicitly #include TestTimeouts. Also indent +4.

Done. Fixed in UITestBase also.

http://codereview.chromium.org/4202004/diff/98003/77013#newcode14
chrome/test/ui/ui_named_test.cc:14: ASSERT_EQ(AUTOMATION_SUCCESS,
connection_->WaitForAppLaunch())
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> This duplicates code from UITestBase::LaunchBrowserAndServer. Please share it.

Moved back into UITestBase. See message.

http://codereview.chromium.org/4202004/diff/98003/77013#newcode20
chrome/test/ui/ui_named_test.cc:20: AutomationProxy*
UINamedTest::CreateAutomationProxy(int execution_timeout) {
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> This duplicates code from UITestBase::CreateAutomationProxy.

Moved back into UITestBase. See message.

http://codereview.chromium.org/4202004/diff/98003/77015
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77015#newcode63
chrome/test/ui/ui_test.cc:63: static const std::string kNamedPrefix =
"NamedTestingInterface:";
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> This constant is now duplicated throughout the code base. That's bad.

It's used in ui_test.cc and browser_init.cc. What would be a better place for
it? chrome_constants.h?

http://codereview.chromium.org/4202004/diff/98003/77015#newcode704
chrome/test/ui/ui_test.cc:704: std::string channel_id;
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: huh?

Whoops.

http://codereview.chromium.org/4202004/diff/98003/77015#newcode784
chrome/test/ui/ui_test.cc:784: wait, process);
On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> nit: indentation.

Done.

John Grabowski

On Thu, Nov 4, 2010 at 12:58 AM, <phajdan.jr@chromium.org> wrote: > +jabdelmalek for ipc comments ...

10 years, 1 month ago (2010-11-05 04:32:55 UTC) #22

Paweł Hajdan Jr.

On 2010/11/05 04:32:55, John Grabowski wrote: > Hmm. Do you really think it's a better ...

10 years, 1 month ago (2010-11-05 15:18:26 UTC) #23

On 2010/11/05 04:32:55, John Grabowski wrote:
> Hmm.  Do you really think it's a better idea to take a bunch of PyAuto
> infrastructure (e.g. starting the browser, enabling crash reports, clearing
> the profile, setting up a specific test profile of bookmarks, et al) and
> duplicate it in a new launcher system?

This might indeed be unrelated to this CL, but generally it seems we need some
kind of a "thin launcher" used by UI tests, PyAuto, and WebDriver. The launcher
should be shared, not duplicated.

> Do you really not see the value in the ability to control Chrome in both
> Chrome-specific ways (e.g. PyAuto calls) and general in-the-web calls (a la
> WebDriver)?

My comments were not about value, but possible issues. Anyway, this indeed is
beyond the scope of this CL in terms of code. However, it would be nice to have
a more detailed plan for one of the main scenarios this change is going to
enable.

> If you have a problem with how WebDriver "misuses UITestBase", that is
> outside the context of this change.

Not necessarily. Please note I *could* request the author of the WebDriver
change to fix that right away, and maybe I should have done that. I have started
cleaning up the mess afterwards, it's not trivial, and not done yet.

Now we're adding one more thing to the system. Please note it's really close,
just needs attention to a few important details.

> Can you please give a summary of the issues you still feel are outstanding?

This is a good idea. Please just take a look at the comments below. The most
important one is the scattered if (use_named_interface) checks in ui_test.cc.

http://codereview.chromium.org/4202004/diff/98003/77015
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/98003/77015#newcode63
chrome/test/ui/ui_test.cc:63: static const std::string kNamedPrefix =
"NamedTestingInterface:";
On 2010/11/05 03:05:02, Dave Tu wrote:
> On 2010/11/04 07:58:26, Paweł Hajdan Jr. wrote:
> > This constant is now duplicated throughout the code base. That's bad.
> 
> It's used in ui_test.cc and browser_init.cc. What would be a better place for
> it? chrome_constants.h?

We have chrome/test/automation_constants.h. You may need/want to move it to
chrome/common.

http://codereview.chromium.org/4202004/diff/144001/145002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/4202004/diff/144001/145002#newcode99
chrome/browser/automation/automation_provider.h:99: // Sends an
AutomationMsg_InitialLoadsComplete to the AutomationProxy.
nit: This is an implementation detail. In the header file just say something
like "Called when the initial set of tabs has finished loading.".

http://codereview.chromium.org/4202004/diff/144001/145002#newcode100
chrome/browser/automation/automation_provider.h:100: void SetLoadsComplete();
nit: Could you rename to OnInitialLoadsComplete? Please rename the member
variable accordingly.

http://codereview.chromium.org/4202004/diff/144001/145002#newcode416
chrome/browser/automation/automation_provider.h:416: bool connected_;
Please comment both member variables.

http://codereview.chromium.org/4202004/diff/144001/145004
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/144001/145004#newcode1028
chrome/browser/browser_init.cc:1028: if (channel_id.find(named_prefix) == 0) {
Could you move this logic to InitializeChannel?

http://codereview.chromium.org/4202004/diff/144001/145007
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/4202004/diff/144001/145007#newcode70
chrome/test/automation/automation_proxy.h:70: // If use_named_interface is true,
it will act as a server. It creates
nit: "It creates..." - that's an implementation detail. If you want to keep that
comment, please move it to the .cc file.

http://codereview.chromium.org/4202004/diff/144001/145009
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/144001/145009#newcode145
chrome/test/ui/ui_test.cc:145: if (use_named_interface_)
Those scattered checks are a no-go, as said earlier.

http://codereview.chromium.org/4202004/diff/144001/145009#newcode207
chrome/test/ui/ui_test.cc:207: void UITestBase::LaunchClient() {
nit: Could you rename it to "ConnectToRunningBrowser"?

http://codereview.chromium.org/4202004/diff/144001/145009#newcode214
chrome/test/ui/ui_test.cc:214: void UITestBase::WaitForBrowser() {
nit: "WaitForBrowserLaunch"?

http://codereview.chromium.org/4202004/diff/144001/145009#newcode889
chrome/test/ui/ui_test.cc:889: AutomationProxy *proxy = new
AutomationProxy(execution_timeout, true);
nit: star on the wrong side.

http://codereview.chromium.org/4202004/diff/144001/145010
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/4202004/diff/144001/145010#newcode457
chrome/test/ui/ui_test.h:457: scoped_ptr<AutomationProxy> connection_;
nit: Why not just automation_proxy_?

John Grabowski

On Fri, Nov 5, 2010 at 8:18 AM, <phajdan.jr@chromium.org> wrote: > On 2010/11/05 04:32:55, John ...

10 years, 1 month ago (2010-11-05 17:27:11 UTC) #24

dtu

On 2010/11/05 17:27:11, John Grabowski wrote: > On Fri, Nov 5, 2010 at 8:18 AM, ...

10 years, 1 month ago (2010-11-05 18:51:51 UTC) #25

dtu

jrg gave me some basic pointers and I was able to refactor out the scattered ...

10 years, 1 month ago (2010-11-06 02:55:13 UTC) #26

Nirnimesh

http://codereview.chromium.org/4202004/diff/179001/180002 File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/179001/180002#newcode415 chrome/browser/automation/automation_provider.h:415: bool is_connected_; // True iff is connected to an ...

10 years, 1 month ago (2010-11-06 04:05:20 UTC) #27

Paweł Hajdan Jr.

This is going in a good direction. Thank you for the work on that and ...

10 years, 1 month ago (2010-11-06 12:17:59 UTC) #28

dtu

Made changes in accordance with comments. Modified UITestBase so that CreateAutomationProxy() is defined in ProxyLauncher ...

10 years, 1 month ago (2010-11-09 04:41:52 UTC) #29

Made changes in accordance with comments.

Modified UITestBase so that CreateAutomationProxy() is defined in ProxyLauncher
instead. A CreateProxyLauncher() virtual function is defined in UITestBase to
specify which ProxyLauncher to use. Override this function to specify different
AutomationProxies!

I don't want to put too many more refactoring changes in this CL. It's starting
to balloon to include many extra files, and more refactoring changes should be
split into a separate CL for clarity.

http://codereview.chromium.org/4202004/diff/179001/180001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/4202004/diff/179001/180001#newcode157
chrome/browser/automation/automation_provider.cc:157: std::string chan_id =
channel_id;
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> nit: Abbreviations are discouraged. How about final_channel_id instead? Or
> actual, effective, etc?

Done.

http://codereview.chromium.org/4202004/diff/179001/180001#newcode165
chrome/browser/automation/automation_provider.cc:165: DCHECK(chan_id.length() >
0);
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> We usually avoid DCHECKs in the automation interface. I'd prefer to convert
this
> method to return bool, and return false here if things are bad.

Done.

http://codereview.chromium.org/4202004/diff/179001/180002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/4202004/diff/179001/180002#newcode415
chrome/browser/automation/automation_provider.h:415: bool is_connected_;        
   // True iff is connected to an AutomationProxy
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> nit: Could you put those comments above each member variable instead of on the
> right, and also leave one empty line between the declarations?

Done.

http://codereview.chromium.org/4202004/diff/179001/180002#newcode415
chrome/browser/automation/automation_provider.h:415: bool is_connected_;        
   // True iff is connected to an AutomationProxy
On 2010/11/06 04:05:21, Nirnimesh wrote:
> remove "is".
> End with '.' (also on next 2 lines)

Done.

http://codereview.chromium.org/4202004/diff/179001/180006
File chrome/common/automation_constants.cc (right):

http://codereview.chromium.org/4202004/diff/179001/180006#newcode7
chrome/common/automation_constants.cc:7: namespace automation {
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> nit: Add empty lines similarly to the header.

Done, except for the lint issue above.

http://codereview.chromium.org/4202004/diff/179001/180007
File chrome/common/automation_constants.h (right):

http://codereview.chromium.org/4202004/diff/179001/180007#newcode12
chrome/common/automation_constants.h:12: // JSON value labels for proxy settings
that are passed in via
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> nit: While you're here, add an empty line above.

Done.

http://codereview.chromium.org/4202004/diff/179001/180007#newcode23
chrome/common/automation_constants.h:23: extern const std::string
kNamedInterfacePrefix;
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> This should be extern const char kNamedInterfacePrefix[];

Done.

http://codereview.chromium.org/4202004/diff/179001/180007#newcode24
chrome/common/automation_constants.h:24: }
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> nit: While you're here, add an empty line above.

Gives me a lint error when I do that.

http://codereview.chromium.org/4202004/diff/179001/180011
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/179001/180011#newcode64
chrome/test/ui/ui_test.cc:64: static const std::string kPipePath =
"/var/tmp/ChromeTestingInterface";
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> nit: Again, this should be const char kPipePath[], should be in a separate
block
> to avoid confusion, and should have a comment.

Done.

http://codereview.chromium.org/4202004/diff/179001/180011#newcode94
chrome/test/ui/ui_test.cc:94: class ProxyLauncher {
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> Could you move those ProxyLauncher classes to a separate file? They also need
> more comments.

Done.

http://codereview.chromium.org/4202004/diff/179001/180011#newcode191
chrome/test/ui/ui_test.cc:191: if (use_named_interface_) {
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> Should we move those parts to the ProxyLauncher as well?
> 
> - connecting the IPC channels
> - waiting for the full launch

There are a lot of code files that call LaunchBrowserAndServer() so any
modifications to that function should go into a separate CL.

http://codereview.chromium.org/4202004/diff/179001/180011#newcode847
chrome/test/ui/ui_test.cc:847: else
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> Is it possible we have no AutomationProxy and launch the browser?

It is possible now, if the browser is launched as a server first, and the
AutomationProxy is created and connects afterward.

http://codereview.chromium.org/4202004/diff/179001/180011#newcode921
chrome/test/ui/ui_test.cc:921: return
launcher_->CreateAutomationProxy(execution_timeout, true);
On 2010/11/06 12:17:59, Paweł Hajdan Jr. wrote:
> Should we now change the virtual method in UITestBase so that they can provide
> their own ProxyLauncher?

Done.

Paweł Hajdan Jr.

http://codereview.chromium.org/4202004/diff/179001/180007 File chrome/common/automation_constants.h (right): http://codereview.chromium.org/4202004/diff/179001/180007#newcode24 chrome/common/automation_constants.h:24: } On 2010/11/09 04:41:53, Dave Tu wrote: > On ...

10 years, 1 month ago (2010-11-09 19:03:20 UTC) #30

dtu

The lint error is: "Blank line at the end of a code block. Is this ...

10 years, 1 month ago (2010-11-09 21:02:28 UTC) #31

The lint error is: "Blank line at the end of a code block. Is this needed?"

Someone made a change that moves browser_init.h. This patch set is synced to
that change, but there was a mistake in the following file that wasn't updated
properly:
chrome/browser/tabs/pinned_tab_codec.h

http://codereview.chromium.org/4202004/diff/237002/243002
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/4202004/diff/237002/243002#newcode356
chrome/browser/automation/automation_provider.cc:356: channel_->Send(new
AutomationMsg_Hello(0, version_info.Version().c_str()));
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: Is the c_str needed?

Done.

http://codereview.chromium.org/4202004/diff/237002/243003
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/4202004/diff/237002/243003#newcode92
chrome/browser/automation/automation_provider.h:92: bool InitializeChannel(const
std::string& channel_id);
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: Now add WARN_UNUSED_RESULT from base/compiler_specific.h.

Done.

http://codereview.chromium.org/4202004/diff/237002/243014
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/4202004/diff/237002/243014#newcode16
chrome/test/automation/proxy_launcher.cc:16: return channel_id_;
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> Why this is adding a member and an implemented virtual function to
> ProxyLauncher? It seems this method should be pure virtual in ProxyLauncher.

Done.

http://codereview.chromium.org/4202004/diff/237002/243015
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/4202004/diff/237002/243015#newcode22
chrome/test/automation/proxy_launcher.h:22: // Modifies the automation proxy's
channel
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: This comment is extremely vague, and const sort of contradicts
"modifies".

Done.

http://codereview.chromium.org/4202004/diff/237002/243015#newcode41
chrome/test/automation/proxy_launcher.h:41: // Uses an automation proxy that
communicates of an anonymous socket.
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: of -> over?

Done.

http://codereview.chromium.org/4202004/diff/237002/243015#newcode48
chrome/test/automation/proxy_launcher.h:48: bool disconnect_on_failure_;  //
Whether we disconnect on IPC channel failure.
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: For consistency, add this comment to other classes in this file.
> 
> Moreover "Whether" might create some ambiguity. I recommend "True if we
> disconnect...".

Done.

http://codereview.chromium.org/4202004/diff/237002/243017
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4202004/diff/237002/243017#newcode147
chrome/test/ui/ui_test.cc:147: if (use_named_interface_)
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> Now this should really be a part of the launcher. If people are calling
> LaunchBrowserAndServer, that doesn't seem a problem. We can make it call
> ProxyLauncher or whatever.

Done. ProxyLauncher doesn't have any of the local variables that are needed in
the implementation of LaunchBrowserAndServer() (especially since it doesn't own
the AutomationProxy) so for now I just call LaunchBrowserAndServer() from the
ProxyLauncher. Still, use_named_interface_ is not needed anymore, which is good.

http://codereview.chromium.org/4202004/diff/237002/243017#newcode801
chrome/test/ui/ui_test.cc:801: fds = base::file_handle_mapping_vector();
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: Is this needed? Seems to be just a default initialization?

Done.

http://codereview.chromium.org/4202004/diff/237002/243018
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/4202004/diff/237002/243018#newcode43
chrome/test/ui/ui_test.h:43: class ProxyLauncher;
On 2010/11/09 19:03:21, Paweł Hajdan Jr. wrote:
> nit: sort.

Done.

Paweł Hajdan Jr.

http://codereview.chromium.org/4202004/diff/222003/chrome/browser/ui/browser_init.cc File chrome/browser/ui/browser_init.cc (right): http://codereview.chromium.org/4202004/diff/222003/chrome/browser/ui/browser_init.cc#newcode1038 chrome/browser/ui/browser_init.cc:1038: DCHECK(list); nit: Remove pointless DCHECK. It was there before, ...

10 years, 1 month ago (2010-11-10 11:15:44 UTC) #32

jam

ipc parts should go to one of the people who worked on the posix version ...

10 years, 1 month ago (2010-11-10 17:02:41 UTC) #33

dtu

-jabdelmalek -jam +agl for changes to ipc_channel_posix.cc, ipc_channel.h On 2010/11/10 17:02:41, John Abd-El-Malek wrote: > ...

10 years, 1 month ago (2010-11-10 19:16:57 UTC) #34

dtu

This is taking too long. I know there is a time zone difference, but the ...

10 years, 1 month ago (2010-11-11 00:10:54 UTC) #36

This is taking too long. I know there is a time zone difference, but the full
day it takes to do a feedback cycle is very unproductive. This changelist is
blocking other changes that need to happen, so if there are any medium to large
refactorings that still need to be done, it should be split into another
changelist so that either you can do it yourself more efficiently, or you can
ask me or someone else to do it so that it can be done in parallel without
blocking. The refactorings are getting off track from the scope of this
changelist.

http://codereview.chromium.org/4202004/diff/222003/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/4202004/diff/222003/chrome/browser/ui/browser_...
chrome/browser/ui/browser_init.cc:1038: DCHECK(list);
On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
> nit: Remove pointless DCHECK. It was there before, but let's clean it up while
> we're here.

Done.

http://codereview.chromium.org/4202004/diff/222003/chrome/common/automation_c...
File chrome/common/automation_constants.cc (right):

http://codereview.chromium.org/4202004/diff/222003/chrome/common/automation_c...
chrome/common/automation_constants.cc:17: const char kNamedInterfacePrefix[] =
"NamedTestingInterface:";
On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
> nit: Same here.

Done.

http://codereview.chromium.org/4202004/diff/222003/chrome/common/automation_c...
File chrome/common/automation_constants.h (right):

http://codereview.chromium.org/4202004/diff/222003/chrome/common/automation_c...
chrome/common/automation_constants.h:22: extern const char
kNamedInterfacePrefix[];
On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
> nit: Really add an empty line below. If the linter complains, it is wrong. See
> chrome/common/chrome_constants.h.

Done.

http://codereview.chromium.org/4202004/diff/222003/chrome/common/automation_c...
chrome/common/automation_constants.h:23: }
On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
> nit:  // namespace automation

Done.

http://codereview.chromium.org/4202004/diff/222003/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/4202004/diff/222003/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:31:
ui_test_base->ConnectToRunningBrowser();
On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
> We need to go further. Now it adds another level of indirection with little
> gain. Could you move the implementation of ConnectToRunningBrowser (and also
> LaunchBrowserAndServer) here?
> 
> It may make sense to merge CreateAutomationProxy and InitializeConnection into
> one method, that would return an already connected AutomationProxy, after
> waiting for the full launch of the application.

Those functions (and their helper functions LaunchBrowser(),
WaitForBrowserLaunch(), LaunchBrowserHelper()) have a TON of dependences in
UITestBase. It would uproot half of UITestBase to move these things. This is a
large enough refactoring change to warrant its own changelist.

http://codereview.chromium.org/4202004/diff/222003/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/4202004/diff/222003/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:32: std::string channel_id_;  // Channel
id of automation proxy.
On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
> nit: A member variable doesn't make sense in a class with just pure virtual
> methods.

Done.

Paweł Hajdan Jr.

On Thu, Nov 11, 2010 at 01:10, <dtu@chromium.org> wrote: > Those functions (and their helper ...

10 years, 1 month ago (2010-11-11 10:22:17 UTC) #37

Nirnimesh

It's incredible how long this CL has dragged on. We need to iterate early and ...

10 years, 1 month ago (2010-11-11 11:20:45 UTC) #38

Paweł Hajdan Jr.

On Thu, Nov 11, 2010 at 12:20, <nirnimesh@chromium.org> wrote: > It's incredible how long this ...

10 years, 1 month ago (2010-11-11 18:07:44 UTC) #39

On Thu, Nov 11, 2010 at 12:20, <nirnimesh@chromium.org> wrote:

> It's incredible how long this CL has dragged on. We need to iterate early
> and
> often. What began as a small size CL has ballooned into so much additional
> complexity that I'd prefer the scattered-bool-approach better.
>

As said earlier, it indeed may be worth it to do a cleanup CL based on this
patch first, and then add the named interface parts.

> Based on the comment below I do not think that more refactoring is
> necessary
> (apart from renaming the classes as I suggested). Pawel do you agree?
>

Hmm, I think that now we're in the middle of refactoring (i.e. at the point
when I commented "we need to go further"), and I definitely wouldn't want
that to land.

>
>
http://codereview.chromium.org/4202004/diff/222003/chrome/test/automation/pro...
> chrome/test/automation/proxy_launcher.cc:31:
> ui_test_base->ConnectToRunningBrowser();
> On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
>
>> We need to go further. Now it adds another level of indirection with
>>
> little
>
>> gain. Could you move the implementation of ConnectToRunningBrowser
>>
> (and also
>
>> LaunchBrowserAndServer) here?
>>
>
>  It may make sense to merge CreateAutomationProxy and
>>
> InitializeConnection into
>
>> one method, that would return an already connected AutomationProxy,
>>
> after
>
>> waiting for the full launch of the application.
>>
>
> No, I disagree. We do NOT want this class to start looking like a mini
> UITestBase.
>

I think I do. The problem is that UITestBase is really problematic. It's
fine for PyAuto, but it's too much for WebDriver. So my understanding is
that we do need a mini UITestBase of some kind (maybe a different one that
my comments have been leading to, but still).

After that, it is very probable that PyAuto can switch to the ProxyLauncher,
and we can remove UITestBase, ending up with a better design.

> I like that this class does one thing and one thing only -- do what's
> needed to initialize the automation channel. Maybe these needs to be
> renamed to ChannelInitializer instead of ProxyLauncher, but I don't want
> it be launch the browser, and things like that (also that'd involve it
> manipulating the launch command arguments and so on.. and very soon it
> starts fighting for responsibility with UITestBase).

Note that UITestBase has no clear responsibility in the first place. Now
let's take a closer look at the class that tries to need only one thing
(launch the automation channel). It's very confusing why it needs to ask
UITestBase for the rest of things (waiting for the browser etc). I think
that a class which takes some parameters and returns an automation channel
and launched browser is a nice one, and also with very clear responsibility.

krisr

Pawel, It looks like your requests are becoming too large for this CL and possibly ...

10 years, 1 month ago (2010-11-11 18:13:15 UTC) #40

Pawel,
  It looks like your requests are becoming too large for this CL and
possibly even more than a one man job.  Lets check this in now and then both
you and dtu can finish up the remaining re-factors in parallel.  It is
important that we get this running on the waterfall to make sure we haven't
broken anything.
  We may not all agree, but after two weeks dtu has certainly done a lot of
work to satisfy your comments.

Kris

On Thu, Nov 11, 2010 at 10:07 AM, Paweł Hajdan, Jr. <phajdan.jr@chromium.org
> wrote:

> On Thu, Nov 11, 2010 at 12:20, <nirnimesh@chromium.org> wrote:
>
>> It's incredible how long this CL has dragged on. We need to iterate early
>> and
>> often. What began as a small size CL has ballooned into so much additional
>> complexity that I'd prefer the scattered-bool-approach better.
>>
>
> As said earlier, it indeed may be worth it to do a cleanup CL based on this
> patch first, and then add the named interface parts.
>
>
>> Based on the comment below I do not think that more refactoring is
>> necessary
>> (apart from renaming the classes as I suggested). Pawel do you agree?
>>
>
> Hmm, I think that now we're in the middle of refactoring (i.e. at the point
> when I commented "we need to go further"), and I definitely wouldn't want
> that to land.
>
>
>>
>>
http://codereview.chromium.org/4202004/diff/222003/chrome/test/automation/pro...
>> chrome/test/automation/proxy_launcher.cc:31:
>> ui_test_base->ConnectToRunningBrowser();
>> On 2010/11/10 11:15:44, Paweł Hajdan Jr. wrote:
>>
>>> We need to go further. Now it adds another level of indirection with
>>>
>> little
>>
>>> gain. Could you move the implementation of ConnectToRunningBrowser
>>>
>> (and also
>>
>>> LaunchBrowserAndServer) here?
>>>
>>
>>  It may make sense to merge CreateAutomationProxy and
>>>
>> InitializeConnection into
>>
>>> one method, that would return an already connected AutomationProxy,
>>>
>> after
>>
>>> waiting for the full launch of the application.
>>>
>>
>> No, I disagree. We do NOT want this class to start looking like a mini
>> UITestBase.
>>
>
> I think I do. The problem is that UITestBase is really problematic. It's
> fine for PyAuto, but it's too much for WebDriver. So my understanding is
> that we do need a mini UITestBase of some kind (maybe a different one that
> my comments have been leading to, but still).
>
> After that, it is very probable that PyAuto can switch to the
> ProxyLauncher, and we can remove UITestBase, ending up with a better design.
>
>
>> I like that this class does one thing and one thing only -- do what's
>> needed to initialize the automation channel. Maybe these needs to be
>> renamed to ChannelInitializer instead of ProxyLauncher, but I don't want
>> it be launch the browser, and things like that (also that'd involve it
>> manipulating the launch command arguments and so on.. and very soon it
>> starts fighting for responsibility with UITestBase).
>
>
> Note that UITestBase has no clear responsibility in the first place. Now
> let's take a closer look at the class that tries to need only one thing
> (launch the automation channel). It's very confusing why it needs to ask
> UITestBase for the rest of things (waiting for the browser etc). I think
> that a class which takes some parameters and returns an automation channel
> and launched browser is a nice one, and also with very clear responsibility.
>

Paweł Hajdan Jr.

On Thu, Nov 11, 2010 at 19:13, Kris Rambish <krisr@chromium.org> wrote: > It looks like ...

10 years, 1 month ago (2010-11-11 18:22:30 UTC) #41

krisr

On Thu, Nov 11, 2010 at 10:21 AM, Paweł Hajdan, Jr. <phajdan.jr@chromium.org > wrote: > ...

10 years, 1 month ago (2010-11-11 18:32:21 UTC) #42

On Thu, Nov 11, 2010 at 10:21 AM, Paweł Hajdan, Jr. <phajdan.jr@chromium.org
> wrote:

> On Thu, Nov 11, 2010 at 19:13, Kris Rambish <krisr@chromium.org> wrote:
>
>> It looks like your requests are becoming too large for this CL and
>> possibly even more than a one man job.  Lets check this in now and then both
>> you and dtu can finish up the remaining re-factors in parallel.
>>
>
> Please do the refactors first. Most of them do not depend on the named
> testing interface parts being present anyway, and should have been done
> earlier. The past experience with this code indicates that it's much better
> to get it right before landing. Later there are much less incentives to
> actually fix things.
>

If the refactors are independent of the named testing interface, then they
are out of scope for this CL.  It is good to do cleanup, but we have to find
the right balance.  I think we are too far on the cleanup side now.


>
>
>> It is important that we get this running on the waterfall to make sure we
>> haven't broken anything.
>>
>
> I agree. However, it's also very important that we keep updating the design
> while adding more and more code. In fact, we're already "behind" after
> WebDriver bits have landed, so people who promised to clean things up later
> have already landed.
>

If the WebDriver code that landed needs to be cleaned up, again that is out
of scope for this CL.


>
>
>> We may not all agree, but after two weeks dtu has certainly done a lot of
>> work to satisfy your comments.
>>
>
> Yes, definitely, and I like the progress. However, I'd say this is still in
> the middle, and not ready to land.
>
> How about splitting the chrome/browser/automation parts and ipc/ parts into
> a separate CL, getting that reviewed (should be quick), and then continue
> with the AutomationProxy side, possibly in a series of CLs instead of one
> huge patch?
>

I'll talk with nirnimesh and dtu about this.  If it is just breaking it up
then it shouldn't be, quick, it should be immediate since it is all part of
the same very thoroughly reviewed CL.  I do not want to have this
same conversation over another CL that is simply pieces of this CL.  Again
we need to find the right balance.

Nirnimesh

> > No, I disagree. We do NOT want this class to start looking like ...

10 years, 1 month ago (2010-11-11 18:39:41 UTC) #43

> > No, I disagree. We do NOT want this class to start looking like a mini
> > UITestBase.
> >
> 
> I think I do. The problem is that UITestBase is really problematic. It's
> fine for PyAuto, but it's too much for WebDriver. So my understanding is
> that we do need a mini UITestBase of some kind (maybe a different one that
> my comments have been leading to, but still).

Let's back up to what the objective for this CL was. The refactoring to
ProxyLauncher was an answer to remove the scattered bool approach, which it
does. Period.
The fact that you'd like additional levels of abstraction within UITestBase
cannot be held up against this CL. Please ask webdriver authors to finish the
cleanup they promised.
It's not even clear to me what the objective of the new abstraction layer would
be (which UITestBase currently doesn't already do) and at this point I don't
think it clear to you either.



> 
> After that, it is very probable that PyAuto can switch to the ProxyLauncher,
> and we can remove UITestBase, ending up with a better design.
> 
> 
> > I like that this class does one thing and one thing only -- do what's
> > needed to initialize the automation channel. Maybe these needs to be
> > renamed to ChannelInitializer instead of ProxyLauncher, but I don't want
> > it be launch the browser, and things like that (also that'd involve it
> > manipulating the launch command arguments and so on.. and very soon it
> > starts fighting for responsibility with UITestBase).
> 
> 
> Note that UITestBase has no clear responsibility in the first place.

Feel free to send CLs to fix that, but that is out of scope here. The
responsibility of ProxyLauncher at this point appears very clear to me -- it
initializes the channel, given an instance of UITestBase. What did I miss?

> Now
> let's take a closer look at the class that tries to need only one thing
> (launch the automation channel). It's very confusing why it needs to ask
> UITestBase for the rest of things (waiting for the browser etc).

I see, since ProxyLauncher is a separate independent class you don't want it to
be connected to UITestBase. Would it help if ProxyLauncher classes were private
to UITestBase?

Paweł Hajdan Jr.

On Thu, Nov 11, 2010 at 19:32, Kris Rambish <krisr@chromium.org> wrote: > If the refactors ...

10 years, 1 month ago (2010-11-11 23:08:19 UTC) #44

On Thu, Nov 11, 2010 at 19:32, Kris Rambish <krisr@chromium.org> wrote:

> If the refactors are independent of the named testing interface, then they
> are out of scope for this CL.
>

Right. However, sometimes we need to adjust the design before new code can
be added. I believe this is the case here.

> If the WebDriver code that landed needs to be cleaned up, again that is out
> of scope for this CL.
>

Not exactly. People have been always hacking on the automation, and have
been pushing to get their changes in. At some point we need to stop piling
new code and make things a bit more tidy.

> How about splitting the chrome/browser/automation parts and ipc/ parts into
>> a separate CL, getting that reviewed (should be quick), and then continue
>> with the AutomationProxy side, possibly in a series of CLs instead of one
>> huge patch?
>>
>
> I'll talk with nirnimesh and dtu about this.  If it is just breaking it up
> then it shouldn't be, quick, it should be immediate since it is all part of
> the same very thoroughly reviewed CL.
>

Yeah, for the parts I mentioned you can expect my quick LGTM.

On Thu, Nov 11, 2010 at 19:39, <nirnimesh@chromium.org> wrote:

> Let's back up to what the objective for this CL was. The refactoring to
> ProxyLauncher was an answer to remove the scattered bool approach, which it
> does. Period.
>

Correct. However, it introduces its own little problems. It's the right
direction, but checking in code in the middle of refactoring isn't my
intention.

Note that some parts are quite ready (chrome/browser/automation), but some
are not (AutomationProxy, ProxyLauncher, UITest, etc).

> The fact that you'd like additional levels of abstraction within UITestBase
> cannot be held up against this CL.

I think this is one of the main points. I think it can. At some point we
need to find the right design. When PyAuto was created, it was not obvious
what to do with some parts of UITest, so we added UITestBase. That wasn't
perfect, but was fine. Then for WebDriver we've noticed that UITestBase is
too large (WebDriver doesn't use most of it, and it shouldn't), but it
wasn't obvious what to do either, so the cleanup has been postponed.

We've only determined that we'd need something responsible just for
launching the browser, and very basic automation bits. Now this CL actually
brings the code very close to what I think it should look like. Let's make
it right, and then you can build the rest on a solid foundation.

> Please ask webdriver authors to finish the cleanup they promised.
>

To clarify, the bug about this got assigned to me (
http://code.google.com/p/chromium/issues/detail?id=56865), but this CL does
most of the work needed here.

> It's not even clear to me what the objective of the new abstraction layer
> would
> be (which UITestBase currently doesn't already do) and at this point I
> don't
> think it clear to you either.

The problem is not about things UITestBase doesn't do. It's that it does too
much. I believe that ProxyLauncher is exactly what we need to make it really
right. It got introduced in this CL to get rid of the "scattered bool
checks", and is generally good but has one problem: depends back on the
UITestBase. This means one can't use ProxyLauncher without UITestBase, which
is bad because there is nothing in the ProxyLauncher's responsibility
(launching the browser and setting up the automation) that should require
UITestBase. We've seen it's non-trivial to fix that, so checking in the code
in this state would mean checking in code with known serious design problems
(and I'm afraid there are very little incentives to fix it later). Could you
explain more why it seems to you that the objective is not clear?

John Grabowski

If you add the test described in named_interface_uitest.cc, LGTM. http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/automation_provider.h File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/automation_provider.h#newcode422 chrome/browser/automation/automation_provider.h:422: ...

10 years, 1 month ago (2010-11-12 23:32:53 UTC) #45

dtu

http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/automation_provider.h File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/automation_provider.h#newcode422 chrome/browser/automation/automation_provider.h:422: bool initial_loads_complete_; On 2010/11/12 23:32:53, John Grabowski wrote: > ...

10 years, 1 month ago (2010-11-16 01:30:40 UTC) #46

John Grabowski

LGTM http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_interface_uitest.cc File chrome/test/ui/named_interface_uitest.cc (right): http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_interface_uitest.cc#newcode37 chrome/test/ui/named_interface_uitest.cc:37: On 2010/11/16 01:30:40, Dave Tu wrote: > On ...

10 years, 1 month ago (2010-11-16 01:33:03 UTC) #47

Paweł Hajdan Jr.

Please note that according to http://www.chromium.org/developers/committers-responsibility there are several issues that must be fixed before ...

10 years, 1 month ago (2010-11-16 09:44:49 UTC) #48

Paweł Hajdan Jr.

On Wed, Nov 17, 2010 at 09:25, John Grabowski <jrg@chromium.org> wrote: > -cc chromium-reviews > ...

10 years, 1 month ago (2010-11-17 09:07:07 UTC) #49

On Wed, Nov 17, 2010 at 09:25, John Grabowski <jrg@chromium.org> wrote:

> -cc chromium-reviews
>
> Please note that according to
>> http://www.chromium.org/developers/committers-responsibility there are
>> several
>> issues that must be fixed before this can be committed.
>>
>>
> Are you serious?
>
> We've got a pattern here.  For more than one person, and across many
> different CLs, code review with you drags out for weeks.
>

I'm sorry this takes long. Note however, that this is not the only review
that I add drive-by comments (and to be precise, you have explicitly asked
for my feedback in this review). In fact, there are many reviews in which I
drive-by, the author fixes problems, and I LGTM the fixes (and just the
fixes, not entire CL) even before the original reviewer responds.

I think there is no pattern here other than the fact that some reviews are
more complicated than others, and that we have a timezone difference of
about 8 hours (again, I've not heard concerns about this before).

Furthermore, a long review might indicate the patch is too big, or the scope
is too large. Again, note how I suggested splitting the CL in many possible
ways.

As a committer, It is also your responsibility to provide timely feedback
> which is actionable and reasonable.
>

I really hope I do. If my feedback is not actionable, please say so. I think
it is (i.e. "instead of calling X from UITestBase, move the method to
ProxyLauncher"). If you think my comments are unreasonable, please say so (I
think some of the earlier comments suggest that's the problem). Note that
it's not unreasonable to ask for design changes when checking in new code.
Similarly, it's not unreasonable to comment on design problems introduced by
a class that is added in a CL.

Finally, I believe my ultimate responsibility as a committer is to do what
is good for the project. Many of the flakiness and quality problems with the
test automation are caused by the fact that it was not well maintained for
long time. This is now getting better, and I think a part of it is because
of the code reviews.

> There are many possible reasons why your code reviews might take forever:
> - you do not "complete" feedback timely so there is always "one more thing"
>

The last point could possibly be applied here. Note however, that the fixes
to previous round of comments may generate another round of comments. Try
asking questions like "Could this comment by added in an earlier round of
review?". If the code I'm commenting wasn't present in the earlier round, I
couldn't comment on it earlier. Similarly, "Does this comment contradict an
earlier one?". If no, we're moving forward, even if it takes a bit longer.

> It almost doesn't matter which one of these is true.  The net result is
> that things don't close.
>

Again, let me repeat: most of the reviews I comment close just fine.

> Lack of closure means you run the risk of being ignored completely.
>

This is a risky thing to say. Does it mean "if I don't want to fix the
review comments, I will ignore them"?

> Do what you like.  Find a way to provide value and help drive towards
> closure... or not.
>

Please feel free to comment about my last *review* comments (about code). It
seems to me at some point the discussion got sidetracked, and it is no
longer focused on making the code good.

Paweł Hajdan Jr.

Yesterday we've chatted about this review, and I'd suggest scheduling regular chats for review sessions ...

10 years, 1 month ago (2010-11-18 06:48:15 UTC) #50

dtu

+amit, there are some minor changes to chrome frame automation that seemed to cause test ...

10 years, 1 month ago (2010-11-19 02:10:49 UTC) #51

amit

Overall changes look OK as far as Chrome Frame is concerned. http://codereview.chromium.org/4202004/diff/527003/chrome/test/automation/automation_proxy_uitest.cc File chrome/test/automation/automation_proxy_uitest.cc (right): ...

10 years, 1 month ago (2010-11-19 18:27:10 UTC) #52

dtu

http://codereview.chromium.org/4202004/diff/527003/chrome/test/automation/automation_proxy_uitest.cc File chrome/test/automation/automation_proxy_uitest.cc (right): http://codereview.chromium.org/4202004/diff/527003/chrome/test/automation/automation_proxy_uitest.cc#newcode873 chrome/test/automation/automation_proxy_uitest.cc:873: return new ExternalTabUITestMockLauncher(&mock_); On 2010/11/19 18:27:10, amit wrote: > ...

10 years, 1 month ago (2010-11-19 19:37:08 UTC) #53

Paweł Hajdan Jr.

I've just chatted with Nirnimesh about this CL. Feel free to land, provided the issue ...

10 years, 1 month ago (2010-11-23 20:57:20 UTC) #54

dtu

Thanks guys for resolving that dispute. If everyone is fine with the latest patch set, ...

10 years, 1 month ago (2010-11-24 06:29:22 UTC) #55

amit

10 years, 1 month ago (2010-11-24 13:04:28 UTC) #56

ok

On Tue, Nov 23, 2010 at 10:29 PM, <dtu@chromium.org> wrote:

> Thanks guys for resolving that dispute. If everyone is fine with the latest
> patch set, it passes all the tries and is ready to land. I'm looking
> forward to
> it.
>
>
>
>
>
http://codereview.chromium.org/4202004/diff/527003/chrome/test/automation/aut...
> File chrome/test/automation/automation_proxy_uitest.cc (right):
>
>
>
http://codereview.chromium.org/4202004/diff/527003/chrome/test/automation/aut...
> chrome/test/automation/automation_proxy_uitest.cc:873: return new
> ExternalTabUITestMockLauncher(&mock_);
> On 2010/11/19 19:37:08, Dave Tu wrote:
>
>> On 2010/11/19 18:27:10, amit wrote:
>> > Ugg! Is there any better way to do this?
>> > It's not ideal but perhaps ExternalTabUITest could implement a
>>
> ProxyLauncher.
>
>  Good idea. Even if it's not ideal it's better than this mess.
>>
>
> Actually, it's not working, which i think is because UITest creates a
> scoped_ptr for its ProxyLauncher, and it crashes during cleanup when the
> pointer goes out of scope.
>
>
> http://codereview.chromium.org/4202004/
>

Issue 4202004: Add named testing interface (Closed)

Description

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Patch Set 8 : '' #

Patch Set 9 : '' #

Patch Set 10 : '' #

Patch Set 11 : '' #

Patch Set 12 : '' #

Patch Set 13 : '' #

Patch Set 14 : '' #

Patch Set 15 : '' #

Patch Set 16 : '' #

Patch Set 17 : '' #

Patch Set 18 : '' #

Patch Set 19 : '' #

Messages