Implemnts the commands in webdriver to preform searching of elements on a page.

Implemnts the commands in webdriver to preform searching of elements on a page.
  /session/:sessionId/timeouts/implicit_wait
  /session/:sessionId/element
  /session/:sessionId/elements  
  /session/:sessionId/element/:id/element
  /session/:sessionId/element/:id/elements
  
BUG=none
TEST=webdriver_remote_tests.py

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70107

[Paweł Hajdan Jr., 2010-11-09 11:19:32]

http://codereview.chromium.org/3643002/diff/2001/3002
File chrome/test/webdriver/commands/find_element_commands.cc (right):

http://codereview.chromium.org/3643002/diff/2001/3002#newcode22
chrome/test/webdriver/commands/find_element_commands.cc:22:
kInternalServerError);
nit: align

http://codereview.chromium.org/3643002/diff/2001/3002#newcode103
chrome/test/webdriver/commands/find_element_commands.cc:103:
PlatformThread::Sleep(250);  // Prevent a busy loop that eats the cpu.
This is the bad sleep'n'probe pattern. Can we wait on the browser side instead?

http://codereview.chromium.org/3643002/diff/2001/3005
File chrome/test/webdriver/commands/implicit_wait_command.h (right):

http://codereview.chromium.org/3643002/diff/2001/3005#newcode33
chrome/test/webdriver/commands/implicit_wait_command.h:33: int wait_;
nit: Can this name be improved? It's an int, so is it a count of something?

[Joe, 2010-11-10 02:37:05]

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/find_element_commands.cc (right):

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/find_element_commands.cc:22:
kInternalServerError);
On 2010/11/09 11:19:32, Paweł Hajdan Jr. wrote:
> nit: align

Done.

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/find_element_commands.cc:103:
PlatformThread::Sleep(250);  // Prevent a busy loop that eats the cpu.
From what I understand a script on the page could change the elements on the
page. So a polling search is the only deterministic method that could work for
all cases without requiring every webpage host to modify for a chrome specific
hook.

On 2010/11/09 11:19:32, Paweł Hajdan Jr. wrote:
> This is the bad sleep'n'probe pattern. Can we wait on the browser side
instead?

[Joe, 2010-11-10 06:57:23]

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/implicit_wait_command.h (right):

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/implicit_wait_command.h:33: int wait_;
On 2010/11/09 11:19:32, Paweł Hajdan Jr. wrote:
> nit: Can this name be improved? It's an int, so is it a count of something?

Done.

[Paweł Hajdan Jr., 2010-11-10 12:18:44]

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/find_element_commands.cc (right):

http://codereview.chromium.org/3643002/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/find_element_commands.cc:103:
PlatformThread::Sleep(250);  // Prevent a busy loop that eats the cpu.
On 2010/11/10 02:37:05, Joe wrote:
> From what I understand a script on the page could change the elements on the
> page. So a polling search is the only deterministic method that could work for
> all cases without requiring every webpage host to modify for a chrome specific
> hook.

I don't think so. Let's at least explore our possibilities on waiting properly.
The rendering engine surely is aware of all of those changes. We need to make it
fire a notification that we can listen for. We can even listen for a "DOM
changed" notification, and only then re-evaluate our filter, instead of just
sleeping for a while.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.cc:101: int64 ellapsed_time
= (base::Time::Now() - start_time).InMilliseconds();
nit: elapsed

[John Grabowski, 2010-11-11 21:43:06]

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/find_element_commands.cc (right):

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.cc:46: root_element_id_ =
GetPathVariable(4);
what happens if we don't have >=4 path variables?

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.cc:103:
PlatformThread::Sleep(250);  // Prevent a busy loop that eats the cpu.
I'd drop this down to 50ms, which would be good enough at not eating the CPU. 
Else we might waste far too much time if we often "just miss" elements/nodes.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/find_element_commands.h (right):

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.h:16: class
FindElementCommand : public WebDriverCommand {
Add description over each class.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.h:18: inline
FindElementCommand(const std::vector<std::string>& path_segments,
inline keyword redundant

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:99: class
TestFindElement(RemoteWebDriverTest):
2 more tests:
1. timeout 0, load a page, wait for it to finish loading, then make sure element
is returned
2. timeout long (2 seconds?), load a page, wait for it to finish loading, then
make sure an element that ISN'T there doesn't get found

[Joe, 2010-11-30 12:07:02]

Before vacation I worked at seeing the work at changing the find element to use
an asynchronous method rather than polling.  It will take a significant amount
of time and would delay webdriver plus we need to make sure behavior stays the
same on IE, Firefox, and Opera.  So for now I think we should just use polling
for now.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/find_element_commands.cc (right):

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.cc:46: root_element_id_ =
GetPathVariable(4);
The url would not match  and we would not instantiate this class.  In this cases
position 4 matches to the ID.  So if the user just requested the url
/session/A32892S/element  we would actually run this command
http://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/e...

On 2010/11/11 21:43:06, John Grabowski wrote:
> what happens if we don't have >=4 path variables?

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.cc:101: int64 ellapsed_time
= (base::Time::Now() - start_time).InMilliseconds();
On 2010/11/10 12:18:45, Paweł Hajdan Jr. wrote:
> nit: elapsed

Done.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.cc:103:
PlatformThread::Sleep(250);  // Prevent a busy loop that eats the cpu.
On 2010/11/11 21:43:06, John Grabowski wrote:
> I'd drop this down to 50ms, which would be good enough at not eating the CPU. 
> Else we might waste far too much time if we often "just miss" elements/nodes.

Done.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/find_element_commands.h (right):

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.h:16: class
FindElementCommand : public WebDriverCommand {
On 2010/11/11 21:43:06, John Grabowski wrote:
> Add description over each class.

Done.

http://codereview.chromium.org/3643002/diff/16001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/find_element_commands.h:18: inline
FindElementCommand(const std::vector<std::string>& path_segments,
On 2010/11/11 21:43:06, John Grabowski wrote:
> inline keyword redundant

Done.

[Paweł Hajdan Jr., 2010-11-30 18:23:37]

Well, can we at least make the loop wait for DOM to change instead of sleeping?
A more "fine-grained" wait would indeed be harder, but this should be relatively
easy.

[shs_google.com, 2010-12-01 11:33:27]

Which existing mechanism are you proposing Joe hooks into? I'm not
sure that one exists yet.

This CL is now almost 2 months old, and has been blocking a lot of
other work (including the testing of Chrome) If this code works, and
there's no extant alternative that can be used for being reliably
notified of changes from the DOM, I suggest that making the suggested
changes are out of scope for this CL.

On Tue, Nov 30, 2010 at 6:23 PM,  <phajdan.jr@chromium.org> wrote:
> Well, can we at least make the loop wait for DOM to change instead of
> sleeping?
> A more "fine-grained" wait would indeed be harder, but this should be
> relatively
> easy.
>
> http://codereview.chromium.org/3643002/
>

[Paweł Hajdan Jr., 2010-12-01 15:24:48]

On Wed, Dec 1, 2010 at 12:33, Simon Stewart <shs@google.com> wrote:

> Which existing mechanism are you proposing Joe hooks into? I'm not
> sure that one exists yet.
>

We may need to do some plumbing for automation, but I'd expect that it's
relatively easy to wait for a DOM change.

Have you asked people more familiar with webkit for suggestions?


> This CL is now almost 2 months old, and has been blocking a lot of
> other work (including the testing of Chrome)


It seems two weeks have passed from my last comment to last author's
comment, so I'm not sure what the above suggests.


> If this code works, and there's no extant alternative that can be used for
> being reliably
> notified of changes from the DOM


Have you really considered the alternatives? If so, could you post more
details why you think they're not feasible?


> I suggest that making the suggested changes are out of scope for this CL.


Arguably, they're in the scope. This change implements a feature in a *known
* flaky way. Flaky means it works most of the time, so after this CL goes
in, there's little incentive to fix the problems. That's why I'd really
prefer to make the code wait properly.

[shs_google.com, 2010-12-01 19:57:28]

On Wed, Dec 1, 2010 at 3:24 PM, Paweł Hajdan, Jr.
<phajdan.jr@chromium.org> wrote:
> On Wed, Dec 1, 2010 at 12:33, Simon Stewart <shs@google.com> wrote:
>>
>> Which existing mechanism are you proposing Joe hooks into? I'm not
>> sure that one exists yet.
>
> We may need to do some plumbing for automation, but I'd expect that it's
> relatively easy to wait for a DOM change.
> Have you asked people more familiar with webkit for suggestions?

I'm asking you for suggestions: this is something that you want.

>> This CL is now almost 2 months old, and has been blocking a lot of
>> other work (including the testing of Chrome)
>
> It seems two weeks have passed from my last comment to last author's
> comment, so I'm not sure what the above suggests.

The initial patch for this CL was submitted almost two months ago.

>> If this code works, and there's no extant alternative that can be used for
>> being reliably
>> notified of changes from the DOM
>
> Have you really considered the alternatives? If so, could you post more
> details why you think they're not feasible?

I don't think it's feasible because no-one suggested a concrete
alternative. Please suggest a concrete alternative that we could use
today.

>> I suggest that making the suggested changes are out of scope for this CL.
>
> Arguably, they're in the scope. This change implements a feature in a known
> flaky way. Flaky means it works most of the time, so after this CL goes in,
> there's little incentive to fix the problems. That's why I'd really prefer
> to make the code wait properly.

Then please suggest the concrete mechanism that already exists that we
can use. If that mechanism does not exist, it's totally unreasonable
to continue blocking this CL.

[Paweł Hajdan Jr., 2010-12-01 20:54:20]

On Wed, Dec 1, 2010 at 20:57, Simon Stewart <shs@google.com> wrote:

> On Wed, Dec 1, 2010 at 3:24 PM, Paweł Hajdan, Jr.
> <phajdan.jr@chromium.org> wrote:
> > On Wed, Dec 1, 2010 at 12:33, Simon Stewart <shs@google.com> wrote:
> >>
> >> Which existing mechanism are you proposing Joe hooks into? I'm not
> >> sure that one exists yet.
> >
> > We may need to do some plumbing for automation, but I'd expect that it's
> > relatively easy to wait for a DOM change.
> > Have you asked people more familiar with webkit for suggestions?
>
> I'm asking you for suggestions: this is something that you want.


This is a bit more complicated, because I'm not familiar with WebKit enough
to suggest any implementation. However, comment like that could apply to my
other flakiness-related reviews, and people have managed to find a better
way to wait.

Ideally I'd like to wait for exactly one event: the element appears.

If we can't do that, I think it's still worth it to wait for DOM changes
instead of sleeping.

Could you ask people who know WebKit about some implementation ideas? Maybe
it's even possible to do that with JS and DOM automation.


>  >> This CL is now almost 2 months old, and has been blocking a lot of
> >> other work (including the testing of Chrome)
> >
> > It seems two weeks have passed from my last comment to last author's
> > comment, so I'm not sure what the above suggests.
>
> The initial patch for this CL was submitted almost two months ago.


That's correct. I think what I was trying to say is that the delays aren't
on my side. Please just ask for help with the implementation, if needed.


> I don't think it's feasible because no-one suggested a concrete
> alternative. Please suggest a concrete alternative that we could use
> today.


Again, I suggested a general approach. Have you tried asking people who may
have more implementation ideas?


> Then please suggest the concrete mechanism that already exists that we
> can use. If that mechanism does not exist, it's totally unreasonable
> to continue blocking this CL.
>

I'm not sure if that's a valid point. If we need to create another mechanism
to make it solid, I believe we should do it. At the very least we should see
how hard that would be. What would you do if it wasn't test code?

[shs_google.com, 2010-12-02 10:30:25]

On Wed, Dec 1, 2010 at 8:53 PM, Paweł Hajdan, Jr.
<phajdan.jr@chromium.org> wrote:
> On Wed, Dec 1, 2010 at 20:57, Simon Stewart <shs@google.com> wrote:
>>
>> On Wed, Dec 1, 2010 at 3:24 PM, Paweł Hajdan, Jr.
>> <phajdan.jr@chromium.org> wrote:
>> > On Wed, Dec 1, 2010 at 12:33, Simon Stewart <shs@google.com> wrote:
>> >>
>> >> Which existing mechanism are you proposing Joe hooks into? I'm not
>> >> sure that one exists yet.
>> >
>> > We may need to do some plumbing for automation, but I'd expect that it's
>> > relatively easy to wait for a DOM change.
>> > Have you asked people more familiar with webkit for suggestions?
>>
>> I'm asking you for suggestions: this is something that you want.
>
> This is a bit more complicated, because I'm not familiar with WebKit enough
> to suggest any implementation. However, comment like that could apply to my
> other flakiness-related reviews, and people have managed to find a better
> way to wait.

That would have been the perfect place to offer some help and
examples. You've yet to explain why polling like this will lead to
flakiness, and I'd appreciate that explanation. To me, it sounds like
you're conflating "efficiency" with "stability": this code is designed
to wait until an element is present on the page. It does that by
polling the page. Switching to an async model and only checking the
page on mutation events just reduces the number of checks that need to
be made. In the worst case, it may lead to _more_ checks as mutation
events can fire very rapidly than the polling interval.

In short: I don't see how your suggested change improves stability.

> Ideally I'd like to wait for exactly one event: the element appears.
> If we can't do that, I think it's still worth it to wait for DOM changes
> instead of sleeping.
> Could you ask people who know WebKit about some implementation ideas? Maybe
> it's even possible to do that with JS and DOM automation.

If you feel it's important enough to block this CL _you_ should be
helping to resolve the issue. Telling the person who happens to be
working on this CL that it's their responsibility is shirking yours.

>>
>> >> This CL is now almost 2 months old, and has been blocking a lot of
>> >> other work (including the testing of Chrome)
>> >
>> > It seems two weeks have passed from my last comment to last author's
>> > comment, so I'm not sure what the above suggests.
>>
>> The initial patch for this CL was submitted almost two months ago.
>
> That's correct. I think what I was trying to say is that the delays aren't
> on my side. Please just ask for help with the implementation, if needed.

I'm asking you for help.

>> I don't think it's feasible because no-one suggested a concrete
>> alternative. Please suggest a concrete alternative that we could use
>> today.
>
> Again, I suggested a general approach. Have you tried asking people who may
> have more implementation ideas?

I've been asking you. This is something you want. The least you could
do is get involved with the solution.

>> Then please suggest the concrete mechanism that already exists that we
>> can use. If that mechanism does not exist, it's totally unreasonable
>> to continue blocking this CL.
>
> I'm not sure if that's a valid point. If we need to create another mechanism
> to make it solid, I believe we should do it.

Joe is using an approach that is already used in other implementations
of this exact code for other browsers. They do not suffer from the
instability you're worried about. I disagree with your contention
about stability, and the data I have backs this up.

> At the very least we should see
> how hard that would be. What would you do if it wasn't test code?

What I'd do is notice that a pattern is being repeated and then
implement a new feature, possibly raising a feature request and
associated bugs against existing code so that existing usages can be
examined to see if the new approach is appropriate.

[Paweł Hajdan Jr., 2010-12-02 11:34:20]

On Thu, Dec 2, 2010 at 11:30, Simon Stewart <shs@google.com> wrote:

> You've yet to explain why polling like this will lead to
> flakiness, and I'd appreciate that explanation. To me, it sounds like
> you're conflating "efficiency" with "stability": this code is designed
> to wait until an element is present on the page. It does that by
> polling the page. Switching to an async model and only checking the
> page on mutation events just reduces the number of checks that need to
> be made. In the worst case, it may lead to _more_ checks as mutation
> events can fire very rapidly than the polling interval.
>
> In short: I don't see how your suggested change improves stability.


Oh well, seems like I'll have to write some sort of FAQ for this. ;-)

The short story is that:

1) The history of problems with test flakiness shows that replacing Sleeps
and loops with waiting for an event fixes intermittent test failures.
2) Loop'n'probe is not the same thing as waiting for an event. For example,
let's say the system is in state A, and you're waiting for state B. P means
you're checking the state of the system. Consider the sequence of events
like A P B A P. That has just missed an occurrence of B, which would not
happen when waiting for an event. This example does not apply very well to
this particular case, but is intended to show that probing in a loop is not
the same as waiting properly.

By the way, Mozilla is doing similar thing for its tests:
https://wiki.mozilla.org/User:Ctalbert/WarOnOrange#Core_Code_Changes

Finally, it's not unimportant how long the tests take to run. Also, when
waiting just for DOM changes, we can throttle the updates if they happen too
frequently, if that becomes a problem.


> If you feel it's important enough to block this CL _you_ should be
> helping to resolve the issue. Telling the person who happens to be
> working on this CL that it's their responsibility is shirking yours.


I suggested that you ask people more familiar with WebKit. To be more
precise, some people I know working on WebKit are Darin Fisher, Dimitri
Glazkov, Eric Seidel. Or just ask a good question on chromium-dev (i.e. not
"should I fix the review comments?" and get an answer "um... yes?", but
something more like "I'm looking for a way to wait for an element to be
available in DOM. How do I do that? Alternatively, I can wait for any DOM
change.").

Have you tried to follow those suggestions? This is not the first time I
make them.

I've tried some simple searches, and found this:
https://developer.mozilla.org/en/DOM_Events . I'm not sure if Chrome
supports those events, and whether it's the right direction (again, that's
why I suggested asking), but if you can get it to run in an isolate webpage
without automation, I will help you do the necessary plumbing for
automation.

If it turns out that technically waiting for an event here has important
problems, that's fine for me. However, we can't determine that without
trying.


> I'm asking you for help.
>

See above. I will help you with the automation side, but first we need to
figure out the general idea.


> I've been asking you. This is something you want. The least you could
> do is get involved with the solution.


Again, I suggested actions to be done.


>  >> Then please suggest the concrete mechanism that already exists that we
> >> can use. If that mechanism does not exist, it's totally unreasonable
> >> to continue blocking this CL.
> >
> > I'm not sure if that's a valid point. If we need to create another
> mechanism
> > to make it solid, I believe we should do it.
>
> Joe is using an approach that is already used in other implementations
> of this exact code for other browsers. They do not suffer from the
> instability you're worried about. I disagree with your contention
> about stability, and the data I have backs this up.


If we're talking about data... how do you determine that the implementations
of the command to find an element on a page in other browser do not suffer
from flakiness problems? Note that 100 or 1000 runs without errors is not
enough. If you can get 10000 or 100000 iterations without single problem,
that's what I'd call solid. Furthermore, many problems with flakiness
reproduce much more frequently on the bots than on local machines.

For another example, see
http://build.chromium.org/f/chromium/flakiness-report/

There are tests that fail between 0.5-1.5% of the time, but it's enough to
fail about 30 times per two weeks. Those numbers add up the more tests we
have, and that makes it very hard to keep the tree green, while also keeping
test coverage high.


> > At the very least we should see
> > how hard that would be. What would you do if it wasn't test code?
>
> What I'd do is notice that a pattern is being repeated and then
> implement a new feature, possibly raising a feature request and
> associated bugs against existing code so that existing usages can be
> examined to see if the new approach is appropriate.
>

That sounds very general to me. I bet that a CL that tried to "wait" that
way (Sleep in a loop) in non-test code would be rejected by any reviewer.

[Paweł Hajdan Jr., 2010-12-02 18:14:55]

I've just asked Darin Fisher on IM, and he also mentioned DOM mutation events.
It should be possible to do.

[Joe, 2010-12-02 23:41:04]

The situation you have described is not what Webdriver is designed for
and I think the issue here is not you aren't familiar with it's
purpose.  Webdriver is only meant to allow web application developers
to test their web pages accross multiple browsers without changing
their test code.
As an example think of Bank of America that wants to test to make sure
their web page works for all browsers.  They would issue the following
commands to login

Step 1 - Create Session
Step 2 - Open page "www.bankofamerica.com"
Step 3 - Find user name textbox element
Step 4 - enter text in user name
Step 5 - find sign in button
Step 6 - move mouse and cick on sign in button

The test must move from one state to another one step at a time, this
is by design.  You cannot run 2 or 3  steps on the client side and
return (say enter text, find the sign button, and click).  If the page
reloads or state is changed after an action is done, say step 3, the
issue needs to be dealt in BofA's test code and not WebDriver.

You are also forgetting that behavior needs to stay the same across
browsers so any proposal needs to work in IE, Firefox, Opera, iPhone,
etc.  The reason we poll is that it's guaranteed consistent behavior
among all platforms and will always work.  I am unfamiliar with DOM
mutation events but I am pretty sure it's non-trival to write the code
and tests to verify IE 6-9, Firefox, Opera, and iPhone all work in the
same manner.

On Thu, Dec 2, 2010 at 10:14 AM,  <phajdan.jr@chromium.org> wrote:
> I've just asked Darin Fisher on IM, and he also mentioned DOM mutation
> events.
> It should be possible to do.
>
> http://codereview.chromium.org/3643002/
>

[Paweł Hajdan Jr., 2010-12-03 11:00:14]

On Fri, Dec 3, 2010 at 00:40, Joe Mikhail <jmikhail@google.com> wrote:

> The situation you have described is not what Webdriver is designed for
> and I think the issue here is not you aren't familiar with it's
> purpose.


Could you explain which situation are you referring to?


> Webdriver is only meant to allow web application developers
> to test their web pages accross multiple browsers without changing
> their test code.
> As an example think of Bank of America that wants to test to make sure
> their web page works for all browsers.  They would issue the following
> commands to login
>
> Step 1 - Create Session
> Step 2 - Open page "www.bankofamerica.com"
> Step 3 - Find user name textbox element
> Step 4 - enter text in user name
> Step 5 - find sign in button
> Step 6 - move mouse and cick on sign in button
>
> The test must move from one state to another one step at a time, this
> is by design.  You cannot run 2 or 3  steps on the client side and
> return (say enter text, find the sign button, and click).  If the page
> reloads or state is changed after an action is done, say step 3, the
> issue needs to be dealt in BofA's test code and not WebDriver.
>

I'm not sure what that is meant to show. My point is that if WebDriver's
infrastructure for step #3 is flaky (find element), then the test will be
flaky.


> You are also forgetting that behavior needs to stay the same across
> browsers so any proposal needs to work in IE, Firefox, Opera, iPhone,
> etc.  The reason we poll is that it's guaranteed consistent behavior
> among all platforms and will always work.


Well, that's one of my points.
http://code.google.com/p/selenium/wiki/JsonWireProtocol is still a draft. If
the spec forces flaky behavior, it's going to be much harder to fix.

By the way, I couldn't find any requirement for polling in the spec. "Search
for an element on the page, starting from the document root. The located
element will be returned as a WebElement JSON object." doesn't rule out
using DOM mutation events. Going back to the example you've presented, if
the tests rely on implementation details, that's also bad.


> I am unfamiliar with DOM mutation events but I am pretty sure it's
> non-trival to write the code
> and tests to verify IE 6-9, Firefox, Opera, and iPhone all work in the
> same manner.
>

Could you present some data for the above? I still can't understand how
implementing automation infrastructure in a known flaky way can be better.

[Joe, 2010-12-06 06:43:41]

Could you explain which situation are you referring to?

The scenario of "Consider the sequence of events like A P B A P."


My point is that if WebDriver's infrastructure for step #3 is flaky
(find element), then the test will be flaky.

I don't see why you think this must be flaky.  While you may have
encountered many cases of polling being flaky in your experience;
there isn't a one to one correspondence between the two, it's just a
method of programming.  Please provide an example I can work with, the
python code to write tests is available both in the tree and on the
webdriver website.


Well, that's one of my points.
http://code.google.com/p/selenium/wiki/JsonWireProtocol is still a
draft. If the spec forces flaky behavior, it's going to be much harder
to fix.

The cost of redoing the json API would be exceptionally high and
fixing it would be way beyond my scope and time.  We would have to
re-implement numerous drivers for each language and browser.  It is
better to just deal with what we have.


I still can't understand how implementing automation infrastructure in
a known flaky way can be better.

It's not a known flaky way, you haven't proven that to me.



If you really do not want polling that is fine by me, but please let
me check this in so that I can unblock multiple product teams and
continue my work.  I am the only one working on the chrome
implementation of webdriver.  In parallel you can make the necessary
changes to use DOM mutation events, write the tests, and make whatever
changes you like to the JSON protocol.


On Fri, Dec 3, 2010 at 2:59 AM, Paweł Hajdan, Jr.
<phajdan.jr@chromium.org> wrote:
> On Fri, Dec 3, 2010 at 00:40, Joe Mikhail <jmikhail@google.com> wrote:
>>
>> The situation you have described is not what Webdriver is designed for
>> and I think the issue here is not you aren't familiar with it's
>> purpose.
>
> Could you explain which situation are you referring to?
>
>>
>> Webdriver is only meant to allow web application developers
>> to test their web pages accross multiple browsers without changing
>> their test code.
>> As an example think of Bank of America that wants to test to make sure
>> their web page works for all browsers.  They would issue the following
>> commands to login
>>
>> Step 1 - Create Session
>> Step 2 - Open page "www.bankofamerica.com"
>> Step 3 - Find user name textbox element
>> Step 4 - enter text in user name
>> Step 5 - find sign in button
>> Step 6 - move mouse and cick on sign in button
>>
>> The test must move from one state to another one step at a time, this
>> is by design.  You cannot run 2 or 3  steps on the client side and
>> return (say enter text, find the sign button, and click).  If the page
>> reloads or state is changed after an action is done, say step 3, the
>> issue needs to be dealt in BofA's test code and not WebDriver.
>
> I'm not sure what that is meant to show. My point is that if WebDriver's
> infrastructure for step #3 is flaky (find element), then the test will be
> flaky.
>
>>
>> You are also forgetting that behavior needs to stay the same across
>> browsers so any proposal needs to work in IE, Firefox, Opera, iPhone,
>> etc.  The reason we poll is that it's guaranteed consistent behavior
>> among all platforms and will always work.
>
> Well, that's one of my points.
> http://code.google.com/p/selenium/wiki/JsonWireProtocol is still a draft. If
> the spec forces flaky behavior, it's going to be much harder to fix.
>
> By the way, I couldn't find any requirement for polling in the spec. "Search
> for an element on the page, starting from the document root. The located
> element will be returned as a WebElement JSON object." doesn't rule out
> using DOM mutation events. Going back to the example you've presented, if
> the tests rely on implementation details, that's also bad.
>
>>
>> I am unfamiliar with DOM mutation events but I am pretty sure it's
>> non-trival to write the code
>> and tests to verify IE 6-9, Firefox, Opera, and iPhone all work in the
>> same manner.
>
> Could you present some data for the above? I still can't understand how
> implementing automation infrastructure in a known flaky way can be better.
>

[Paweł Hajdan Jr., 2010-12-06 09:10:24]

On Mon, Dec 6, 2010 at 07:43, Joe Mikhail <jmikhail@google.com> wrote:

> Could you explain which situation are you referring to?
>
> The scenario of "Consider the sequence of events like A P B A P."


Oh, I only included it for completeness. The idea was to show that polling
is not the same as waiting.


> My point is that if WebDriver's infrastructure for step #3 is flaky
> (find element), then the test will be flaky.
>
> I don't see why you think this must be flaky.  While you may have
> encountered many cases of polling being flaky in your experience;
> there isn't a one to one correspondence between the two, it's just a
> method of programming.


Again, would you "wait" that way in non-test code?


> Please provide an example I can work with, the
> python code to write tests is available both in the tree and on the
> webdriver website.


Feel free to write some sample test yourself. All I'd like to see is trying
to find an element that initially is not a part of the page. Ideally, there
should be some action triggering the addition of that element, and not a
timeout. For example, make the test click a button on the page, make that
button's onclick handler request something via XHR (probably from local
testserver), and add the element we're looking for when it receives
response.

Then I'd like to see that test passing consistently 100000 times on the
bots. That will convince me this CL can be checked in in the current state.


>  Well, that's one of my points.
> http://code.google.com/p/selenium/wiki/JsonWireProtocol is still a
> draft. If the spec forces flaky behavior, it's going to be much harder
> to fix.
>
> The cost of redoing the json API would be exceptionally high and
> fixing it would be way beyond my scope and time.  We would have to
> re-implement numerous drivers for each language and browser.  It is
> better to just deal with what we have.


I'm not suggesting redoing the API. What I meant was that the spec shouldn't
force flaky behavior. It should only specify the result of a call, not that
the implementation should poll. Similarly, clients shouldn't rely on the
fact that some drivers use polling.


> I still can't understand how implementing automation infrastructure in
> a known flaky way can be better.
>
> It's not a known flaky way, you haven't proven that to me.
>

Oh well. Let me look up a few cases where waiting properly was critical to
making a test non-flaky:

http://code.google.com/p/chromium/issues/detail?id=3791
http://code.google.com/p/chromium/issues/detail?id=19361
http://code.google.com/p/chromium/issues/detail?id=5671
http://code.google.com/p/chromium/issues/detail?id=19395
http://code.google.com/p/chromium/issues/detail?id=49680

I can look up more CLs if needed (those above were just from bugs with
owner=me).


> If you really do not want polling that is fine by me, but please let
> me check this in so that I can unblock multiple product teams and
> continue my work.


I'm not sure if keeping arguing whether to apply review comments is
productive. I have suggested a way to convince me that polling is fine in
this CL.


> In parallel you can make the necessary
> changes to use DOM mutation events, write the tests, and make whatever
> changes you like to the JSON protocol.
>

I'm sorry, but that doesn't scale. After fixing a few obvious flakes (like
the bugs cited above), I started doing drive-by reviews to prevent more
flaky code entering the tree. It's much more effective to fix issues with
flakiness before they are committed.

By the way, it seems to be working. At the beginning, the top flaky tests
were just poorly written. Now, most of the time flaky tests show real bugs
in Chrome. By eliminating the known and "obvious" flakiness patterns, we can
focus on fixing real bugs.

[mal, 2010-12-21 22:41:44]

Since this isn't being used on the waterfall, I don't think it's critical to
hold up landing it until everything's perfect. 

Please let's get this checked in so others can work on improving it and getting
webdriver tests running.

Someone should file a bug to address the polling/flakiness issue, which I'd like
to see resolved (or demonstrate that it isn't flaky) before we can add webdriver
tests to the waterfall. 

Meanwhile this should get checked in as there are other bugs to fix and other
people who want to work on the code, but no one can move with this mountain of
code not checked in.

(Pawel, I appreciate the diligence in keeping flaky patterns out of the tree, so
apologies for steamrolling you here. My goal is to unblock some engineers who
want to work on this, and I trust they'll work on making sure these tests are
not flaky too.)

[Paweł Hajdan Jr., 2010-12-22 08:20:43]

On Tue, Dec 21, 2010 at 23:41, <mal@google.com> wrote:

> Since this isn't being used on the waterfall, I don't think it's critical
> to
> hold up landing it until everything's perfect.
>
> Please let's get this checked in so others can work on improving it and
> getting
> webdriver tests running.
>

ACK. No more blocking comments from me (LGTM), and sorry this got delayed.

[Peter Kasting, 2010-12-23 01:37:14]

Win trybot compile failed, yet you landed anyway and broke the tree?  Bad form.

[John Grabowski, 2010-12-23 01:39:18]

You are correct this was bad form.

Nevertheless, Joe was around and reverted immediately.

jrg


On Wed, Dec 22, 2010 at 5:37 PM, <pkasting@chromium.org> wrote:

> Win trybot compile failed, yet you landed anyway and broke the tree?  Bad
> form.
>
>
> http://codereview.chromium.org/3643002/
>

[Peter Kasting, 2010-12-23 01:39:58]

Yes, thank you for the very fast revert :)

[Joe, 2010-12-23 01:41:06]

Sorry I hit control C and canceled the submit into the tree, since it
asked for my email I thought it didn't go trough.

On Wed, Dec 22, 2010 at 5:39 PM,  <pkasting@chromium.org> wrote:
> Yes, thank you for the very fast revert :)
>
> http://codereview.chromium.org/3643002/
>