Skip to content

Conversation

sandeepsuryaprasad
Copy link
Contributor

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

Fixed Select class and Launcher tests for Safari.

Motivation and Context

  • Chrome raises NotImplementedError while trying to select item from a list box which is disabled. But Safari doesn't. I have marked all those tests as xfail_safari.
  • Fixed Safari launcher test by passing service object to WebDriver

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.

@titusfortner
Copy link
Member

What is raising NotImplemented errors? I'm wondering if that is the right behavior.

@sandeepsuryaprasad
Copy link
Contributor Author

sandeepsuryaprasad commented Aug 11, 2023

@titusfortner , when we try to operate on a list box which is disabled, in chrome below exception is raised.

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/selenium/webdriver/support/select.py", line 98, in select_by_index
    self._set_selected(opt)
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/selenium/webdriver/support/select.py", line 213, in _set_selected
    raise NotImplementedError("You may not select a disabled option")
NotImplementedError: You may not select a disabled option

In case of Safari, no exception is raised, program execution completes without any exceptions or any messages. But yes, since the list box is disabled nothing gets selected. I think it is better to raise ElementNotInteractableException instead of NotImplementedError

@titusfortner
Copy link
Member

Oh, support classes are special, but we should know why it isn't getting that error before just skipping the tests

@sandeepsuryaprasad
Copy link
Contributor Author

My understanding is that the error should be raised by safaridriver itself and nothing to do with python bindings.

@titusfortner
Copy link
Member

These tests are all about describing intended vs actual behavior. The xfail message needs to reflect how actual behavior differs from intended.

This is the support module where we are working with opinionated behavior. The opinion we are enforcing is that if the user wants to select a disabled option with the Select class, it should throw an error instead of ignoring it. So we check the status and throw the error in the bindings.

Is Safari reporting that the element is not disabled when it is? Why isn't it getting the error that we want it to have?

@sandeepsuryaprasad
Copy link
Contributor Author

I was debugging this issue, and found that in function _set_selected in select.py,

    def _set_selected(self, option) -> None:
        if not option.is_selected():
            if not option.is_enabled():
                raise NotImplementedError("You may not select a disabled option")
            option.click()

option.is_enabled() returns True (Incorrect)for Safari and False for Chrome (Correct). So that's the reason why NotImplementedError is raised in case of Chrome and not in Safari

@titusfortner
Copy link
Member

So that needs to be the message in xfail, "options incorrectly reported as enabled"

@sandeepsuryaprasad
Copy link
Contributor Author

Yes, I have changed it and pushed commit.

@titusfortner titusfortner merged commit e1ef766 into SeleniumHQ:trunk Aug 14, 2023
@sandeepsuryaprasad sandeepsuryaprasad deleted the safari-tests-fix branch November 24, 2024 04:15
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.

2 participants