HENNGE / arsenic

Async WebDriver implementation for asyncio and asyncio-compatible frameworks
Other
350 stars 53 forks source link

added a few apis to be able to switch between window handles #21

Closed tarekziade closed 6 years ago

tarekziade commented 6 years ago

It's a work in progress, as I plan to add test. Just want to make sure this is how we want the API.

Notice that we could have a generic api in the session class so we can call an arbitrary URL from the https://w3c.github.io/webdriver/webdriver-spec.html spec, using session._request

ojii commented 6 years ago

Sorry for the extreme delay, somehow this PR completely flew under my radar. Had to kick CI a few times, but it's happy now. Speaking of which, any chance you can add a test or two for this?

The new APIs should also be added to https://arsenic.readthedocs.io/en/latest/reference/session.html

tarekziade commented 6 years ago

Sure! Any thoughts on the generic API @ojii ?

tarekziade commented 6 years ago

@ojii I've added request() as well.

ojii commented 6 years ago

Sure! Any thoughts on the generic API @ojii ?

Thanks!

Is there a need to have both _request and request? They seem to do the same thing.

The window tests seem to fail on phantomjs/chromedriver, could you look into that?

tarekziade commented 6 years ago

@ojii Is there a need to have both _request and request? They seem to do the same thing.

This is mostly a question of public API for the Session class. and whether the Session class might add in the future some custom code in request() before the RequestHelper._request() method gets called. We could always turn it in a property in the future if that happens, I dunno. Up to you.

The window tests seem to fail on phantomjs/chromedriver, could you look into that?

yes, looks like the webdriver implementation diverges a bit there, I'll investigate

ojii commented 6 years ago

This is mostly a question of public API for the Session class. and whether the Session class might add in the future some custom code in request() before the RequestHelper._request() method gets called. We could always turn it in a property in the future if that happens, I dunno. Up to you.

That's basically the reasoning why there was no public, generic request api so far. I just wasn't too sure how it has to look. I think having the dual setup for now is fine, dropping the internal wouldn't be a big issue.

tarekziade commented 6 years ago

ok. I guess the need for that public API is to make the lib work even for the API we did not implement yet (and for the changes in the future)

tarekziade commented 6 years ago

@ojii how do you run your tests locally? the fix seemed ok on my laptop but circleci is hunappy.

ojii commented 6 years ago

@ojii how do you run your tests locally? the fix seemed ok on my laptop but circleci is hunappy.

I thought I documented this but I guess not. The easiest (though also slowest) way is to install circleci local and run circleci build. Unfortunately this is not 100% the same as online, but it's close enough.

To run for a specific browser using your local (non-dockerized) environment, set these two env variables:

Both take the class name of either a service or a browser as a value, with optionally query-string/json encoded arguments to pass to that class.

For example to run against chrome (using chromedriver) in headless mode, set them to:

Then run pytest in the root directory.

If you have the required tools installed, I recommend the non-docker version because of speed and debugability.

tarekziade commented 6 years ago

Maybe a good addition would be a Makefile so we can run "make test" and have it pull everything that's required locally and not worry too much.

ojii commented 6 years ago

Maybe a good addition would be a Makefile so we can run "make test" and have it pull everything that's required locally and not worry too much.

That's basically what circle build is supposed to be. But I'm not really happy with the current CI setup...

tarekziade commented 6 years ago

e.g.:

/usr/local/bin/circleci: 
    sudo curl -o /usr/local/bin/circleci https://circle-downloads.s3.amazonaws.com/releases/build_agent_wrapper/circleci
    sudo chmod +x /usr/local/bin/circleci

test: /usr/local/bin/circleci
    circleci config validate -c .circleci/config.yml
    circlebi build
tarekziade commented 6 years ago

@oji ok the branch should be good to go now

ojii commented 6 years ago

@tarekziade thank you, this looks fantastic.