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

Fix for #1333 | Python alert automation #1470

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

eaccmk
Copy link
Contributor

@eaccmk eaccmk commented Sep 9, 2023

Description

This PR fixes issue #1333

Motivation and Context

Bug fix and contribution to Selenium community 🎉

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)
  • Bug Fix

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

@netlify
Copy link

netlify bot commented Sep 9, 2023

👷 Deploy request for selenium-dev pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 3d7a027

@eaccmk eaccmk changed the title Fix for #1333 | Python alet automation Fix for #1333 | Python alert automation Sep 9, 2023
@titusfortner
Copy link
Member

First, thanks for the PR.

The intention of the code here is not to thoroughly test a page, but to provide examples that can be displayed here: https://www.selenium.dev/documentation/webdriver/interactions/alerts/#alerts

So we only need 3 working examples that we can link to, and they should be complete examples instead of abstracting out the common pieces. The point is to look at the complete method.

Finally, I'm confused about the sleeps. We shouldn't need to hard code sleeps. Is the Python Selenium code not doing what we expect it to?

@eaccmk
Copy link
Contributor Author

eaccmk commented Sep 11, 2023

First, thanks for the PR.

The intention of the code here is not to thoroughly test a page, but to provide examples that can be displayed here: https://www.selenium.dev/documentation/webdriver/interactions/alerts/#alerts

So we only need 3 working examples that we can link to, and they should be complete examples instead of abstracting out the common pieces. The point is to look at the complete method.

Finally, I'm confused about the sleeps. We shouldn't need to hard code sleeps. Is the Python Selenium code not doing what we expect it to?

Sure, I can update the PR.

How about 3 examples, like this one ( I used this Alert example )

# Click the link to activate the alert
driver.find_element(By.ID, "alert").click()

# Store the alert in a variable
alert = driver.switch_to.alert

# Store the alert text in a variable
text = alert.text

#assert alert text
assert alert.text == "cheese"

# Press the OK button
alert.accept()

On

We shouldn't need to hard code sleeps.

It was the Alert pop up from browser that was taking time when I was hitting all the alert links. If I am putting 3 basic examples, hardcoded sleep is not required and works fine.

@eaccmk
Copy link
Contributor Author

eaccmk commented Sep 12, 2023

Done ✅
@titusfortner, ready for review.

@titusfortner
Copy link
Member

That test gets fixed by #1472

@titusfortner
Copy link
Member

But also, we want to run these with pytest in our CI, so we want to put these into their own test methods.

@titusfortner
Copy link
Member

and replace it with this:

{{< gh-codeblock path="examples/python/tests/interactions/test_alerts.py#LXX-LYY" >}}
  • Finally we're looking to copy that block of code in the en file to the other translation markdown files as well.

If you want to discuss or collaborate, come find us in the #selneium-docs channel in the chat room: https://www.selenium.dev/support/#ChatRoom

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

Successfully merging this pull request may close these issues.

None yet

3 participants