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

[py] Expand element to be clickable #9374

Conversation

maxtheaxe
Copy link
Contributor

@maxtheaxe maxtheaxe commented Apr 11, 2021

Previously only supported locator usage, added ability to pass element directly.

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

  • Added the ability to pass an element to element_to_be_clickable(), rather than just a locator.
  • Checks argument type, and then operates accordingly (in the case of a locator arg, it is first converted/resolved to an element)

Motivation and Context

This feature already exists in the Java version, but does not in Python. It adds convenience.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I've never submitted a real pull request to anything before (or worked on a large project), so I'd appreciate any guidance necessary.

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2021

CLA assistant check
All committers have signed the CLA.

@@ -275,6 +278,22 @@ def _predicate(driver):
return _predicate


def element_to_be_clickable(element):
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to have a separate method here, we just need to do a test to check if it is a Tuple or a WebElement and then do the right thing. There are already examples in the file of how to do it. Please also add a test to py/test/selenium/webdriver/support/expected_conditions_tests.py

If you're up for a future PR, where there are duplications in the file for methods but differentiated by element or locator then we should deprecate them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was aiming to match that seemingly-established style of pairs of functions used elsewhere...now I'm curious why some ended up with the unnecessary duplication and some didn't. Regardless, I'm happy to clean that up in a future PR when I get the chance. Do you know off the top of your head if that duplication is limited to expected_conditions, or should I expect to walk through the entire python framework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible that the existing implementations (those that you are suggesting I reference, that is) might benefit from a change in argument name, to reflect that they may be either locators or webElements? Leaving them named "locator" seems confusing, considering that elsewhere "locator" means literally a locator type, whereas here they can also be passed a webElement.

Again, this would be something to address in that future PR you spoke of, but it's just something that briefly threw me off when I was looking for the functions that accept either type.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but we will need to deprecate them and remove them in the future after Selenium 4 has been released. Let's get this patch done and landed and we can take care of the rest later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was having a lot of trouble running the complete tests locally, despite following along here. They should pass this time, and I think we should be all set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, I believe it passed everything it was supposed to. It's awaiting the signature of @p0deje because I synced with upstream partway through--I wasn't aware that it might cause problems.

Copy link
Member

Choose a reason for hiding this comment

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

What's awaiting me?

@maxtheaxe maxtheaxe closed this Apr 12, 2021
@maxtheaxe maxtheaxe force-pushed the py-expectedconditions-clickable-expansion branch from f574690 to 4134935 Compare April 12, 2021 18:54
@maxtheaxe maxtheaxe reopened this Apr 12, 2021
@AutomatedTester
Copy link
Member

@maxtheaxe Could you rebase your PR against trunk so we remove the Ruby changes that are unrelated.

@maxtheaxe maxtheaxe force-pushed the py-expectedconditions-clickable-expansion branch from 1df4c70 to 6d5fc88 Compare April 14, 2021 21:11
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AutomatedTester
Copy link
Member

Thanks for your PR!

@AutomatedTester AutomatedTester merged commit ca2c72a into SeleniumHQ:trunk Apr 20, 2021
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