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

Initialize Alert object by calling alert.text #1863

Merged
merged 3 commits into from
Dec 6, 2017
Merged

Initialize Alert object by calling alert.text #1863

merged 3 commits into from
Dec 6, 2017

Conversation

rouke-broersma
Copy link

this is how it's done in the Java client, and makes sense as you would want to fail, or have your alert reference as early as possible.

Fixes #1822

this is how it's done in the Java client, and makes sense as you would want to fail, or have your alert reference as early as possible.
@lukeis
Copy link
Member

lukeis commented Apr 7, 2016

personally i don't like the java client approach and prefer how python does it by not forcing the user to make an extra api call if they don't want to. Also, if you really want to be in line with the java client, then text should be a private variable in the Alert class that is cached, instead of fetched every time (since as is with your change, there would always be two api calls to get the alert text if you wanted to get the text)

Also, this change (if it is indeed accepted) would need to update the ExpectedConditions class and possibly related tests-
https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/support/expected_conditions.py#L264-L266

but in general I'm 👎 on this change

@rouke-broersma
Copy link
Author

No api call is made at all though, currently, which is just plain weird. You're asking for the alert, and you're getting an empty object back. This does not make sense to me. If I ask for an alert, and there is no alert, I would expect an exception.

I cannot find where Java caches the alert text in the alert, I have based my approach on:
https://github.com/SeleniumHQ/selenium/blob/master/java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java#L1065-1096

Also caching does not make sense to me, because when the alert disappears after getting the reference to the Alert, and I ask for the text again, I would expect an exception, because the alert no longer exists.

I do agree that I should not have submitted the pr before updating the ExpectedConditions class and checking if any tests need updating, I apologize for that.

@lukeis
Copy link
Member

lukeis commented Apr 7, 2016

looks like that changed a while ago in java:
4d3f89c

@rouke-broersma
Copy link
Author

I would argue initializing alert is also more in line with the switch_to api in general. All other switch_to calls execute a server command.

@rouke-broersma
Copy link
Author

Also I have checked all the tests and cannot find any test that needs a change for this pr. Please correct me if I'm wrong.

@lukeis lukeis added the C-py label Apr 13, 2016
@lmtierney lmtierney merged commit eab402f into SeleniumHQ:master Dec 6, 2017
@lmtierney
Copy link
Member

Merging to align with other bindings, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants