Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit wait fix / adding test for it #83

Closed
wants to merge 5 commits into from
Closed

Implicit wait fix / adding test for it #83

wants to merge 5 commits into from

Conversation

lukeis
Copy link
Contributor

@lukeis lukeis commented Oct 9, 2012

Synchronous checking for elements wasn't working (I think it wasn't yielding to the thread in order to allow the page to be updated). Also some logic around how implicit wait works with findElements (list) wasn't quite right.

detro and others added 5 commits October 8, 2012 15:25
When a request `"Content-Type"` equals `"application/x-www-form-urlencoded"`,
PhantomJS assumes (correctly?) that the POST Body Message is a set of values
if a form URL-Encoded format.
So it parses the content of the body, creating a QVariantMap (JSON),
that is then attached to the `request.post` property.

The "as-is" content is stored in `request.postRaw`.

This workaround overrides `request.post = request.postRaw`,
but the issue should be solved on the Python Binding level that shouldn't
set the wrong "Content-Type".

UPDATE: Based on a conversation with David Burns, the Python bindings in
Selenium 2.26 have that fixed.

Fixes Issue #6.
Implement tests from Issue #77
@lukeis
Copy link
Contributor Author

lukeis commented Oct 9, 2012

you can now run the selenium python tests by starting ghostdriver manually and then from selenium source running the command in trunk:
./go //py:ghostdriver_test:run

// retry if "startTime + implicitTimeout" is
// greater (or equal) than current time
if (searchStartTime + _session.getTimeout(_session.timeoutNames().IMPLICIT) >= new Date().getTime()) {
setTimeout(function(){_locateElementCommand(req, res, locatorMethod, searchStartTime);}, 50);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would yeld!
The timeout will be put in the events queue, and stay there until picked.
The loop I used won't yeld.

I don't see why this is a good idea, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will, kind of the point. It appears that looping with the while consumes the thread, which appears to be the same one that is the UI thread for the page itself (at least that's what it appeared to do to me).

Please give the test a try with and without this change.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look into this. You might be right and I just spoke without thinking through it (I replied this morning, before my espresso, so I don't consider that good/quality thinking :P)

@detro
Copy link
Owner

detro commented Oct 9, 2012

Question: did you add a PhantomJSDriver to the Python bindings? Or are you just using a RemoteWebDriver?

I'm about to commit a PhantomJSDriver to the Selenium project as "official Java bind": @jimevans provides the .Net one. Would be GREAT to have a Python one :) (hint hint ;))

@lukeis
Copy link
Contributor Author

lukeis commented Oct 9, 2012

Yep, I intend to write the same in the python bindings very soon.

Yes, for the tests I ran I just used RemoteWebDriver.

@sebv
Copy link

sebv commented Oct 11, 2012

There is the same timeout issue in webelement_request_handler.js, should probably be fixed at the same time.

@detro
Copy link
Owner

detro commented Oct 11, 2012

Working on merging this: unfortunately I can't just merge it as is, and the fact that I did other things on the master in the meantime, and @lukeis didn't rebase, makes it impossible to do a simple merge.

Anyway, I have worked things out: thanks again @lukeis and @sebv to making me notice.

I think I need to stop this stupid duplication of locateElementMethod() and unify everything in one place.
If it was only easy to pick one ;)

@lukeis
Copy link
Contributor Author

lukeis commented Oct 11, 2012

give me about 2 hours and I'll rebase and maybe try unifying them (on a train to work right now...)

@sebv
Copy link

sebv commented Oct 11, 2012

There is an alternative fix in my fork, a bit easier to read. I've tested it using the wd test suite, will try to see if I can reuse @lukeis Java tests, and will work on the web element fix.

@detro
Copy link
Owner

detro commented Oct 11, 2012

Guys, no worries. It's all committed in my local master.

In few minutes you will be able to test the result, but it seems to work nicely.

@detro
Copy link
Owner

detro commented Oct 11, 2012

Fixed by 1e20d12

@detro detro closed this Oct 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants