Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4202004:
Add named testing interface (Closed)
|
Created:
10 years, 1 month ago by dtu Modified:
9 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org, darin-cc_chromium.org, ben+cc_chromium.org, sky, krisr Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd named testing interface. This allows you to connect to a pre-existing Chrome process and run tests on it. This is an addition to the low level interface underlying testing frameworks like PyAuto and WebDriver.
Normally, test frameworks communicate with Chrome over an unnamed socket pair on POSIX. The test creates the socket pair and then launches the browser as a child process, passing an open file descriptor for one end of the socket to the browser. This change adds a command line switch that, when passed to the browser, causes it to listen on a named socket instead, eliminating this parent/child process requirement. Therefore, you can potentially connect any number of tests to a preexisting browser process.
For ChromeOS, this allows you to run tests on the instance of Chrome that is launched on startup, which controls things like the login and lock screens, the battery meter, the wireless UI, etc. Currently there is no way to run tests on a pre-existing Chrome instance. Eventually this will also allow you to connect both PyAuto and WebDriver to the same Chrome instance and run both in the same test.
If you pass the browser the following command line switch:
./chrome --testing-channel=NamedTestingInterface:/path/to/file
This causes the browser to listen for incoming connections. An AutomationProxy can connect to the browser by connecting a Unix domain socket to the specified path and control the browser over the socket.
This is currently only for POSIX. Windows support will come in a future change. Also, this initial change only allows one connection; multiple connection support will come in a future change.
BUG=chromium-os:8512
TEST=Run Chrome with --testing-interface=/var/tmp/NamedTestingInterface, then run NamedInterfaceTest.BasicNamedInterface under ui_tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67300
Patch Set 1 #
Total comments: 11
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 20
Patch Set 4 : '' #
Total comments: 39
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 7
Patch Set 7 : '' #
Total comments: 23
Patch Set 8 : '' #
Total comments: 20
Patch Set 9 : '' #
Total comments: 28
Patch Set 10 : '' #
Total comments: 18
Patch Set 11 : '' #
Total comments: 13
Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 9
Patch Set 14 : '' #
Total comments: 3
Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #
Total comments: 3
Patch Set 18 : '' #Patch Set 19 : '' #Messages
Total messages: 56 (0 generated)
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: NamedTestingInterface:some_path http://codereview.chromium.org/4202004/diff/1/5 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/4202004/diff/1/5#newcode2301 chrome/chrome_tests.gypi:2301: 'target_name': 'fifo_test', put this inside ['OS=="linux" or OS=="mac"', { ] http://codereview.chromium.org/4202004/diff/1/5#newcode2303 chrome/chrome_tests.gypi:2303: 'msvs_guid': '', remove it http://codereview.chromium.org/4202004/diff/1/6 File chrome/test/automation/automation_proxy.cc (right): http://codereview.chromium.org/4202004/diff/1/6#newcode147 chrome/test/automation/automation_proxy.cc:147: void AutomationProxy::InitializeChannel(const std::string& channel_id) { add arg: bool as_server and use if/else http://codereview.chromium.org/4202004/diff/1/9 File chrome/test/ui/ui_test.cc (right): http://codereview.chromium.org/4202004/diff/1/9#newcode63 chrome/test/ui/ui_test.cc:63: static const char kPipeName[] = "/var/tmp/ChromeTestingInterface"; kPipePath http://codereview.chromium.org/4202004/diff/1/9#newcode215 chrome/test/ui/ui_test.cc:215: LaunchBrowser(launch_arguments_, clear_profile_); remove http://codereview.chromium.org/4202004/diff/1/9#newcode222 chrome/test/ui/ui_test.cc:222: // Set up IPC testing interface server. not a server anymore http://codereview.chromium.org/4202004/diff/1/9#newcode228 chrome/test/ui/ui_test.cc:228: PlatformThread::Sleep(sleep_timeout_ms()); rm http://codereview.chromium.org/4202004/diff/1/9#newcode732 chrome/test/ui/ui_test.cc:732: command_line.AppendSwitchASCII(switches::kTestingChannelID, channel_id_); if (use_fifo_) command_line.Append..(NamedTestingInterface:...) http://codereview.chromium.org/4202004/diff/1/10 File chrome/test/ui/ui_test.h (right): http://codereview.chromium.org/4202004/diff/1/10#newcode75 chrome/test/ui/ui_test.h:75: void LaunchServerAndBrowser(); rename http://codereview.chromium.org/4202004/diff/1/11 File ipc/ipc_channel_posix.cc (right): http://codereview.chromium.org/4202004/diff/1/11#newcode278 ipc/ipc_channel_posix.cc:278: (!channel_id.empty() && channel_id[0] == '/')), Add another c'tor with use_fifo
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 ? IPC::Channel::MODE_SERVER : I'd put the ?: on a line by itself if possible to make it more readable http://codereview.chromium.org/4202004/diff/22001/23001#newcode325 chrome/browser/automation/automation_provider.cc:325: LOG(INFO) << "Test"; A "hello message" should be named "hello", yes? Not Test. http://codereview.chromium.org/4202004/diff/22001/23002 File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/22001/23002#newcode89 chrome/browser/automation/automation_provider.h:89: void InitializeChannel(const std::string& channel_id, bool is_server); Provide a lot more detail. E.g. if is_server, use a named pipe. If not, use an anonymous pipe, so provider/proxy must have a parent/child relationship. http://codereview.chromium.org/4202004/diff/22001/23003 File chrome/browser/browser_init.cc (right): http://codereview.chromium.org/4202004/diff/22001/23003#newcode1026 chrome/browser/browser_init.cc:1026: // IPC and listen on it, rather than trying to connect to an existing IPC period at end of sentence http://codereview.chromium.org/4202004/diff/22001/23004 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/4202004/diff/22001/23004#newcode2957 chrome/chrome_tests.gypi:2957: ['OS=="mac" or OS=="linux"', { Add TODO(your-name): write for windows ( so people know who to ask about this) http://codereview.chromium.org/4202004/diff/22001/23004#newcode2986 chrome/chrome_tests.gypi:2986: # explanation (crbug.com/43791 - libwebcore.a is too large to mmap). >80 http://codereview.chromium.org/4202004/diff/22001/23004#newcode2999 chrome/chrome_tests.gypi:2999: 'configurations': { Are all these conditions needed? E.g. LinkIncremental, tcmalloc, etc? http://codereview.chromium.org/4202004/diff/22001/23006 File chrome/test/automation/automation_proxy.h (right): http://codereview.chromium.org/4202004/diff/22001/23006#newcode70 chrome/test/automation/automation_proxy.h:70: // and waits for an incoming connection. More doc on history http://codereview.chromium.org/4202004/diff/22001/23008 File chrome/test/ui/ui_test.cc (right): http://codereview.chromium.org/4202004/diff/22001/23008#newcode882 chrome/test/ui/ui_test.cc:882: AutomationProxy* UITest::CreateAutomationProxy(int execution_timeout) { How is this different than UITestBase::CreateAutomationProxy? http://codereview.chromium.org/4202004/diff/22001/23009 File chrome/test/ui/ui_test.h (right): http://codereview.chromium.org/4202004/diff/22001/23009#newcode72 chrome/test/ui/ui_test.h:72: void LaunchBrowserAndServer(); I would guess you want the original API to default to the old behavior for compatibility reasons. Is there a reason why you wouldn't want to do this?
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 20:43:55, John Grabowski wrote: > I'd put the ?: on a line by itself if possible to make it more readable > Done. http://codereview.chromium.org/4202004/diff/22001/23001#newcode325 chrome/browser/automation/automation_provider.cc:325: LOG(INFO) << "Test"; On 2010/10/28 20:43:55, John Grabowski wrote: > A "hello message" should be named "hello", yes? Not Test. > Done. http://codereview.chromium.org/4202004/diff/22001/23002 File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/22001/23002#newcode89 chrome/browser/automation/automation_provider.h:89: void InitializeChannel(const std::string& channel_id, bool is_server); On 2010/10/28 20:43:55, John Grabowski wrote: > Provide a lot more detail. E.g. > if is_server, use a named pipe. > If not, use an anonymous pipe, so provider/proxy must have a parent/child > relationship. > Done. http://codereview.chromium.org/4202004/diff/22001/23003 File chrome/browser/browser_init.cc (right): http://codereview.chromium.org/4202004/diff/22001/23003#newcode1026 chrome/browser/browser_init.cc:1026: // IPC and listen on it, rather than trying to connect to an existing IPC On 2010/10/28 20:43:55, John Grabowski wrote: > period at end of sentence Done. http://codereview.chromium.org/4202004/diff/22001/23004 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/4202004/diff/22001/23004#newcode2957 chrome/chrome_tests.gypi:2957: ['OS=="mac" or OS=="linux"', { On 2010/10/28 20:43:55, John Grabowski wrote: > Add TODO(your-name): write for windows > ( so people know who to ask about this) > Done. http://codereview.chromium.org/4202004/diff/22001/23004#newcode2986 chrome/chrome_tests.gypi:2986: # explanation (crbug.com/43791 - libwebcore.a is too large to mmap). On 2010/10/28 20:43:55, John Grabowski wrote: > >80 Done. http://codereview.chromium.org/4202004/diff/22001/23004#newcode2999 chrome/chrome_tests.gypi:2999: 'configurations': { On 2010/10/28 20:43:55, John Grabowski wrote: > Are all these conditions needed? > E.g. LinkIncremental, tcmalloc, etc? > Done. Removed. http://codereview.chromium.org/4202004/diff/22001/23006 File chrome/test/automation/automation_proxy.h (right): http://codereview.chromium.org/4202004/diff/22001/23006#newcode70 chrome/test/automation/automation_proxy.h:70: // and waits for an incoming connection. On 2010/10/28 20:43:55, John Grabowski wrote: > More doc on history Done. Not sure what history you wanted to include, but I changed the doc to match the corresponding documentation for AutomationProvider::InitializeChannel(). http://codereview.chromium.org/4202004/diff/22001/23008 File chrome/test/ui/ui_test.cc (right): http://codereview.chromium.org/4202004/diff/22001/23008#newcode882 chrome/test/ui/ui_test.cc:882: AutomationProxy* UITest::CreateAutomationProxy(int execution_timeout) { On 2010/10/28 20:43:55, John Grabowski wrote: > How is this different than UITestBase::CreateAutomationProxy? > This version disconnects the channel on the first error (see comment on line 888). This is the original behavior and is just for compatibility with the original API. http://codereview.chromium.org/4202004/diff/22001/23009 File chrome/test/ui/ui_test.h (right): http://codereview.chromium.org/4202004/diff/22001/23009#newcode72 chrome/test/ui/ui_test.h:72: void LaunchBrowserAndServer(); On 2010/10/28 20:43:55, John Grabowski wrote: > I would guess you want the original API to default to the old behavior for > compatibility reasons. Is there a reason why you wouldn't want to do this? > The default behavior is unchanged. LaunchBrowserAndServer() is unchanged and LaunchClient() is a new addition to the API.
+Scott as reviewer, for reviewing the change to ipc_channel_posix.cc (or please suggest someone else who could provide suggestions) This is looking great so far. 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, nit: you probably should remove the space before ? and : http://codereview.chromium.org/4202004/diff/1011/34002 File chrome/browser/automation/automation_provider.h (right): 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 The context to 'parent' is not clear. re-word in terms of client/server end of the testing IPC channel 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 put quotes around "NamedTestingInterface:" 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. IPC server and listen on it, else connect as client to an existing IPC server 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()); assert pipe_path.length() > 0 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: Please add some comments explaining this test http://codereview.chromium.org/4202004/diff/1011/34008 File chrome/test/ui/ui_test.cc (right): 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_); This should be done in CreateAutomationProxy above and below 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] == '/')), 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.
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?
On Thu, Oct 28, 2010 at 11:40 PM, <nirnimesh@chromium.org> wrote: > +Scott as reviewer, for reviewing the change to ipc_channel_posix.cc (or > please > suggest someone else who could provide suggestions) I'm not the right person to review that. Maybe Scott Hess or Evan Martin? -Scott > This is looking great so far. > > > 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, > nit: you probably should remove the space before ? and : > > http://codereview.chromium.org/4202004/diff/1011/34002 > File chrome/browser/automation/automation_provider.h (right): > > 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 > The context to 'parent' is not clear. re-word in terms of client/server > end of the testing IPC channel > > 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 > put quotes around "NamedTestingInterface:" > > 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. > IPC server and listen on it, else connect as client to an existing IPC > server > > 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()); > assert pipe_path.length() > 0 > > 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: > Please add some comments explaining this test > > http://codereview.chromium.org/4202004/diff/1011/34008 > File chrome/test/ui/ui_test.cc (right): > > 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_); > This should be done in CreateAutomationProxy above and below > > 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] == > '/')), > 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. > > http://codereview.chromium.org/4202004/show >
At a higher level, Pawel has a good thought that a better description of the goals (even just in the CL description) would be helpful. I personally wouldn't call this a 3rd testing interface (e.g. "pyauto, webdriver, and the new named_testing_interface"); the work here is more to allow the 1st two to coexist together. Doc, or communication with Pawel, would help clarify that. Finally, we need to make sure we don't break other tests that depend on the automation proxy. Can you run some quick tests on your machine (e.g. the page cycler and maybe another one) to confirm happiness? In general I like this change and where it is going but want to make sure Pawel understands the goals before landing. http://codereview.chromium.org/4202004/diff/1011/34002 File chrome/browser/automation/automation_provider.h (right): 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 Please fix this TODO now, not later.
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.
-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 : public UITest { On 2010/11/01 19:56:34, Dave Tu wrote: > 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. I guess you could still include this file in the ui_tests target rather than creating a new one.
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, Nirnimesh wrote: > On 2010/11/01 19:56:34, Dave Tu wrote: > > 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. > > I guess you could still include this file in the ui_tests target rather than > creating a new one. Done.
Code I commented LGTM, after the one comment below. Let's wait for someone for the ipc_channel_posix changes, and for Pawel's response. 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', follow the pattern, use _uitest.cc suffix.
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.
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 >
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?
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 >
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?
> 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?
>
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 pattern, use _uitest.cc suffix. Done. 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 On 2010/11/02 08:16:31, Paweł Hajdan Jr. wrote: > nit: Space between _interface and ? . Done. 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_) On 2010/11/02 08:16:31, Paweł Hajdan Jr. wrote: > 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. I've moved all of these things into a subclass. I hope this is more acceptable.
+jabdelmalek for ipc comments Nirnimesh, thanks for explaining, especially the perf test parts. Please note that we can still have a launcher that records the times, and then drive in the server mode (i.e. the browser acting as the server). I'd really prefer that approach, especially that it would help extracting a better launcher for WebDriver (currently it misuses UITestBase). 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); 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. 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(); Why don't we make it part of InitializeChannel? GenerateChannelID and InitializeChannel seem to be always called together. 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 nit: Would this comment make more sense directly above TEST_F? http://codereview.chromium.org/4202004/diff/98003/77012#newcode21 chrome/test/ui/named_interface_uitest.cc:21: void RunNamedInterfaceTest(int num_tabs) { nit: Why not put this code directly inside TEST_F? This method has only one caller now. 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())); nit: Explicitly #include TestTimeouts. Also indent +4. http://codereview.chromium.org/4202004/diff/98003/77013#newcode14 chrome/test/ui/ui_named_test.cc:14: ASSERT_EQ(AUTOMATION_SUCCESS, connection_->WaitForAppLaunch()) This duplicates code from UITestBase::LaunchBrowserAndServer. Please share it. http://codereview.chromium.org/4202004/diff/98003/77013#newcode20 chrome/test/ui/ui_named_test.cc:20: AutomationProxy* UINamedTest::CreateAutomationProxy(int execution_timeout) { This duplicates code from UITestBase::CreateAutomationProxy. http://codereview.chromium.org/4202004/diff/98003/77014 File chrome/test/ui/ui_named_test.h (right): http://codereview.chromium.org/4202004/diff/98003/77014#newcode11 chrome/test/ui/ui_named_test.h:11: class UINamedTest : public UITest { That *must* have a comment explaining what it is. 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:"; This constant is now duplicated throughout the code base. That's bad. http://codereview.chromium.org/4202004/diff/98003/77015#newcode704 chrome/test/ui/ui_test.cc:704: std::string channel_id; nit: huh? http://codereview.chromium.org/4202004/diff/98003/77015#newcode784 chrome/test/ui/ui_test.cc:784: wait, process); nit: indentation.
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.
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.
On Thu, Nov 4, 2010 at 12:58 AM, <phajdan.jr@chromium.org> wrote: > +jabdelmalek for ipc comments > > Nirnimesh, thanks for explaining, especially the perf test parts. Please > note > that we can still have a launcher that records the times, and then drive in > the > server mode (i.e. the browser acting as the server). I'd really prefer that > approach, especially that it would help extracting a better launcher for > WebDriver (currently it misuses UITestBase). > 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? 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)? A good example is navigating to a web page that has a click-to-play infobar for plugins. We can navagate/interact with WebDriver, but click-to-play (or change a Chrome pref, or get the list of Omnibox completions, or whatever is specifically not controllable from the DOM) with PyAuto. It does not make sense to plan to replace all automation provider communication with this mechanism until we get some trigger time and see how it goes. Easy steps then analysis. If you have a problem with how WebDriver "misuses UITestBase", that is outside the context of this change. This thread has become incredibly long and there are many many issues that have been raised. I'm not sure what you still have issues with. It's been over a week and I think there is value in getting this landed. Can you please give a summary of the issues you still feel are outstanding? jrg
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_?
On Fri, Nov 5, 2010 at 8:18 AM, <phajdan.jr@chromium.org> wrote: > 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. Possibly. Another way to see it is that PyAuto is the "thin launcher" used by WebDriver, and this is step one in such a unification. Factoring out the "launcher" capabilities of PyAuto and using them for UI Tests is out of scope here. > 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. > Yes, you certainly can, and perhaps should. However, unless you want ChromeDriver to not use the automation proxy at all, then it it a bit out of scope. > 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. <snipped> All the comments you added seem reasonable. David, do you think you can address them? Pawel, if he addresses them all, do you feel comfortable with this landing? jrg > > > http://codereview.chromium.org/4202004/show >
On 2010/11/05 17:27:11, John Grabowski wrote: > On Fri, Nov 5, 2010 at 8:18 AM, <mailto:phajdan.jr@chromium.org> wrote: > > On 2010/11/05 04:32:55, John Grabowski wrote: > > > > 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. > > > <snipped> > > All the comments you added seem reasonable. David, do you think you can > address them? > Pawel, if he addresses them all, do you feel comfortable with this landing? > > jrg > It looks like we are getting close, these are mostly small things that I can fix. The only issue is the same one Pawel said is the most important one - I understand that the scattered if (use_named_interface) checks in ui_test.cc are a problem, but I don't know how to fix it. One thing I tried was to make the default behavior of UITestBase the same as use_named_interface = false, and then move all the use_named_interface = true behavior into a subclass called UINamedTestBase. However, this doesn't work well, because then there's no corresponding UINamedTest that has the gtest functionality that UITest has.
jrg gave me some basic pointers and I was able to refactor out the scattered checks. Please let me know if it still needs more work. 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 15:18:26, Paweł Hajdan Jr. wrote: > 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. Done. Apparently there's already a chrome/common/automation_constants.h that's being used for things that are in both AutomationProvider and AutomationProxy. 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. On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: This is an implementation detail. In the header file just say something > like "Called when the initial set of tabs has finished loading.". Done. http://codereview.chromium.org/4202004/diff/144001/145002#newcode100 chrome/browser/automation/automation_provider.h:100: void SetLoadsComplete(); On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: Could you rename to OnInitialLoadsComplete? Please rename the member > variable accordingly. Done. http://codereview.chromium.org/4202004/diff/144001/145002#newcode416 chrome/browser/automation/automation_provider.h:416: bool connected_; On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > Please comment both member variables. Done. 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) { On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > Could you move this logic to InitializeChannel? Done. 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 On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: "It creates..." - that's an implementation detail. If you want to keep that > comment, please move it to the .cc file. Done. 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_) On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > Those scattered checks are a no-go, as said earlier. Done. http://codereview.chromium.org/4202004/diff/144001/145009#newcode207 chrome/test/ui/ui_test.cc:207: void UITestBase::LaunchClient() { On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: Could you rename it to "ConnectToRunningBrowser"? Done. http://codereview.chromium.org/4202004/diff/144001/145009#newcode214 chrome/test/ui/ui_test.cc:214: void UITestBase::WaitForBrowser() { On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: "WaitForBrowserLaunch"? Done. http://codereview.chromium.org/4202004/diff/144001/145009#newcode889 chrome/test/ui/ui_test.cc:889: AutomationProxy *proxy = new AutomationProxy(execution_timeout, true); On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: star on the wrong side. Done. 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_; On 2010/11/05 15:18:26, Paweł Hajdan Jr. wrote: > nit: Why not just automation_proxy_? 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 remove "is". End with '.' (also on next 2 lines)
This is going in a good direction. Thank you for the work on that and good advice for the patch author. 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; nit: Abbreviations are discouraged. How about final_channel_id instead? Or actual, effective, etc? http://codereview.chromium.org/4202004/diff/179001/180001#newcode165 chrome/browser/automation/automation_provider.cc:165: DCHECK(chan_id.length() > 0); 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. 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 nit: Could you put those comments above each member variable instead of on the right, and also leave one empty line between the declarations? 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 { nit: Add empty lines similarly to the header. 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 nit: While you're here, add an empty line above. http://codereview.chromium.org/4202004/diff/179001/180007#newcode23 chrome/common/automation_constants.h:23: extern const std::string kNamedInterfacePrefix; This should be extern const char kNamedInterfacePrefix[]; http://codereview.chromium.org/4202004/diff/179001/180007#newcode24 chrome/common/automation_constants.h:24: } nit: While you're here, add an empty line above. 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"; nit: Again, this should be const char kPipePath[], should be in a separate block to avoid confusion, and should have a comment. http://codereview.chromium.org/4202004/diff/179001/180011#newcode94 chrome/test/ui/ui_test.cc:94: class ProxyLauncher { Could you move those ProxyLauncher classes to a separate file? They also need more comments. http://codereview.chromium.org/4202004/diff/179001/180011#newcode191 chrome/test/ui/ui_test.cc:191: if (use_named_interface_) { Should we move those parts to the ProxyLauncher as well? - connecting the IPC channels - waiting for the full launch http://codereview.chromium.org/4202004/diff/179001/180011#newcode847 chrome/test/ui/ui_test.cc:847: else Is it possible we have no AutomationProxy and launch the browser? http://codereview.chromium.org/4202004/diff/179001/180011#newcode921 chrome/test/ui/ui_test.cc:921: return launcher_->CreateAutomationProxy(execution_timeout, true); Should we now change the virtual method in UITestBase so that they can provide their own ProxyLauncher?
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.
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 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. Hmm, what lint error? http://codereview.chromium.org/4202004/diff/179001/180011 File chrome/test/ui/ui_test.cc (right): http://codereview.chromium.org/4202004/diff/179001/180011#newcode847 chrome/test/ui/ui_test.cc:847: else On 2010/11/09 04:41:53, Dave Tu wrote: > 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. That deserves a comment. 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())); nit: Is the c_str needed? 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); nit: Now add WARN_UNUSED_RESULT from base/compiler_specific.h. 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_; Why this is adding a member and an implemented virtual function to ProxyLauncher? It seems this method should be pure virtual in ProxyLauncher. 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 nit: This comment is extremely vague, and const sort of contradicts "modifies". 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. nit: of -> over? 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. nit: For consistency, add this comment to other classes in this file. Moreover "Whether" might create some ambiguity. I recommend "True if we disconnect...". 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_) 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. http://codereview.chromium.org/4202004/diff/237002/243017#newcode801 chrome/test/ui/ui_test.cc:801: fds = base::file_handle_mapping_vector(); nit: Is this needed? Seems to be just a default initialization? 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; nit: sort.
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.
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); nit: Remove pointless DCHECK. It was there before, but let's clean it up while we're here. 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:"; nit: Same here. 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[]; nit: Really add an empty line below. If the linter complains, it is wrong. See chrome/common/chrome_constants.h. http://codereview.chromium.org/4202004/diff/222003/chrome/common/automation_c... chrome/common/automation_constants.h:23: } nit: // namespace automation 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(); 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. 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. nit: A member variable doesn't make sense in a class with just pure virtual methods.
ipc parts should go to one of the people who worked on the posix version of ipc_channel. maybe evan/agl? btw please my chromium account next time (jam@chromium.org)
-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: > ipc parts should go to one of the people who worked on the posix version of > ipc_channel. maybe evan/agl? > > btw please my chromium account next time (mailto:jam@chromium.org)
ipc_channel_posix.cc, ipc_channel.h: LGTM
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.
On Thu, Nov 11, 2010 at 01:10, <dtu@chromium.org> wrote: > 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. We can't support more test infrastructure changes without cleaning it up. Feel free to split the cleanup to a separate CL, but we need to commit the cleanup *before* this NamedTestingInterface change.
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. 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? One day per feedback cycle is very not working out. If you disagree please list out what you feel the purpose of the new classes should be. Let's try to finish this within the week. 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. No, I disagree. We do NOT want this class to start looking like a mini UITestBase. 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).
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.
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. >
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. > 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. > 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?
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.
> > 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?
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?
If you add the test described in named_interface_uitest.cc, LGTM. http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/... File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/... chrome/browser/automation/automation_provider.h:422: bool initial_loads_complete_; Document that caller can use SetExpectedTabCount(0) to set this to true immediately. http://codereview.chromium.org/4202004/diff/252003/chrome/common/automation_c... File chrome/common/automation_constants.cc (right): http://codereview.chromium.org/4202004/diff/252003/chrome/common/automation_c... chrome/common/automation_constants.cc:17: const char kNamedInterfacePrefix[] = "NamedTestingInterface:"; Document briefly why we have a NamedTestingInterface. (Maybe not here, but somewhere near where this constant is used) http://codereview.chromium.org/4202004/diff/252003/chrome/test/automation/pro... File chrome/test/automation/proxy_launcher.h (right): http://codereview.chromium.org/4202004/diff/252003/chrome/test/automation/pro... chrome/test/automation/proxy_launcher.h:13: // Subclass from this class to use a different implementation of AutomationProxy DISALLOW_COPY_AND_ASSIGN(classname) inside a private: for all classes here http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_inter... File chrome/test/ui/named_interface_uitest.cc (right): http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_inter... chrome/test/ui/named_interface_uitest.cc:37: Add a test which launches browser, then connects, then disconnects, then has a NEW connection form (in the same test) that still works.
http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/... File chrome/browser/automation/automation_provider.h (right): http://codereview.chromium.org/4202004/diff/252003/chrome/browser/automation/... chrome/browser/automation/automation_provider.h:422: bool initial_loads_complete_; On 2010/11/12 23:32:53, John Grabowski wrote: > Document that caller can use SetExpectedTabCount(0) to set this to true > immediately. > Done. http://codereview.chromium.org/4202004/diff/252003/chrome/common/automation_c... File chrome/common/automation_constants.cc (right): http://codereview.chromium.org/4202004/diff/252003/chrome/common/automation_c... chrome/common/automation_constants.cc:17: const char kNamedInterfacePrefix[] = "NamedTestingInterface:"; On 2010/11/12 23:32:53, John Grabowski wrote: > Document briefly why we have a NamedTestingInterface. (Maybe not here, but > somewhere near where this constant is used) Done. http://codereview.chromium.org/4202004/diff/252003/chrome/test/automation/pro... File chrome/test/automation/proxy_launcher.h (right): http://codereview.chromium.org/4202004/diff/252003/chrome/test/automation/pro... chrome/test/automation/proxy_launcher.h:13: // Subclass from this class to use a different implementation of AutomationProxy On 2010/11/12 23:32:53, John Grabowski wrote: > DISALLOW_COPY_AND_ASSIGN(classname) inside a private: for all classes here Done. http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_inter... File chrome/test/ui/named_interface_uitest.cc (right): http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_inter... chrome/test/ui/named_interface_uitest.cc:37: On 2010/11/12 23:32:53, John Grabowski wrote: > Add a test which launches browser, then connects, then disconnects, then has a > NEW connection form (in the same test) that still works. This is not implemented yet and looks to be a little trickier than anticipated, so I'm deferring it to another changelist. There's an issue for it: crosbug.com/8514
LGTM http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_inter... File chrome/test/ui/named_interface_uitest.cc (right): http://codereview.chromium.org/4202004/diff/252003/chrome/test/ui/named_inter... chrome/test/ui/named_interface_uitest.cc:37: On 2010/11/16 01:30:40, Dave Tu wrote: > On 2010/11/12 23:32:53, John Grabowski wrote: > > Add a test which launches browser, then connects, then disconnects, then has a > > NEW connection form (in the same test) that still works. > > This is not implemented yet and looks to be a little trickier than anticipated, > so I'm deferring it to another changelist. There's an issue for it: > crosbug.com/8514 Fine to defer. Can you put a TODO(dtu) in here with a reference to the bug?
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. http://codereview.chromium.org/4202004/diff/458001/chrome/test/automation/pro... File chrome/test/automation/proxy_launcher.cc (right): http://codereview.chromium.org/4202004/diff/458001/chrome/test/automation/pro... chrome/test/automation/proxy_launcher.cc:33: if (launch_browser_) { My earlier comments about this still apply (we should move the UITestBase methods we call here to ProxyLauncher. http://codereview.chromium.org/4202004/diff/458001/chrome/test/automation/pro... chrome/test/automation/proxy_launcher.cc:37: // Wait for browser to be ready for connections nit: Dot at end of comment. http://codereview.chromium.org/4202004/diff/458001/chrome/test/automation/pro... chrome/test/automation/proxy_launcher.cc:39: while (stat(kInterfacePath, &file_info)) This is another no-go. Please wait for an event instead of looping.
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.
Yesterday we've chatted about this review, and I'd suggest scheduling regular chats for review sessions (i.e. when you have a new patch set ready for review, ask me on chat to take a look, and then we would discuss the comments and possible solutions). I know you'd like to check this patch in as soon as possible. We have a problem that has to be solved anyway (breaking ChromeFrame). Hopefully you now how a machine you can debug this on Windows. Finally, I'd really like to at the very least try moving parts of UITestBase to ProxyLauncher, and looking if there are any problems with that, instead of just saying now "it will take too much time, let's skip it". ProxyLauncher doesn't need to be perfect after that move. It will probably have a lot of accessors and setters for all the member variables moved from UITestBase. I'm fine with that for now, as long as ProxyLauncher doesn't depend on UITestBase. Let me know if you have any questions, etc. Feel free to ping me on IM any time you see I'm there.
+amit, there are some minor changes to chrome frame automation that seemed to cause test failures locally. Also, changes to AutomationProxy that chrome frame automation relies on.
Overall changes look OK as far as Chrome Frame is concerned. 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_); Ugg! Is there any better way to do this? It's not ideal but perhaps ExternalTabUITest could implement a ProxyLauncher.
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 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.
I've just chatted with Nirnimesh about this CL. Feel free to land, provided the issue with ProxyLauncher's dependency on UITestBase gets fixed within few days of landing as the top priority. If there is any trouble, please revert the changes. So good luck, and please let me know if you have any questions about what should be done with ProxyLauncher.
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.
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/ > |
