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

[🚀 Feature] [py]: Support FedCM commands for python #14710

Merged
merged 25 commits into from
Nov 15, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Nov 4, 2024

User description

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

Addresses the python bindings related issue in #13449 and #12088.
Adds support for FedCM automation in python bindings.

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.

PR Type

Enhancement, Tests


Description

  • Introduced support for Federated Credential Management (FedCM) in Selenium's Python bindings.
  • Added HasFedCmDialog mixin to the Chrome WebDriver for handling FedCM dialogs.
  • Implemented Account and Dialog classes to represent and interact with FedCM dialogs.
  • Added new commands and HTTP endpoint mappings for FedCM operations in the remote WebDriver.
  • Included comprehensive unit and integration tests to ensure the functionality of FedCM features.

Changes walkthrough 📝

Relevant files
Enhancement
7 files
webdriver.py
Integrate FedCM dialog handling in Chrome WebDriver           

py/selenium/webdriver/chrome/webdriver.py

  • Added HasFedCmDialog mixin to WebDriver class.
+2/-1     
has_fedcm_dialog.py
Add FedCM dialog management extension                                       

py/selenium/webdriver/common/driver_extensions/has_fedcm_dialog.py

  • Introduced HasFedCmDialog class for FedCM-specific functionality.
  • Added methods for managing FedCM dialog and delay.
  • +62/-0   
    account.py
    Define FedCM account representation class                               

    py/selenium/webdriver/common/fedcm/account.py

  • Created Account class to represent FedCM account details.
  • Defined properties for account attributes.
  • +71/-0   
    dialog.py
    Implement FedCM dialog interaction class                                 

    py/selenium/webdriver/common/fedcm/dialog.py

  • Implemented Dialog class for FedCM dialog interactions.
  • Added methods to interact with FedCM dialog elements.
  • +64/-0   
    command.py
    Add FedCM commands for remote WebDriver                                   

    py/selenium/webdriver/remote/command.py

    • Added FedCM-related commands for dialog management.
    +10/-0   
    remote_connection.py
    Map FedCM commands to HTTP endpoints                                         

    py/selenium/webdriver/remote/remote_connection.py

    • Mapped FedCM commands to HTTP endpoints.
    +9/-0     
    webdriver.py
    Extend remote WebDriver with FedCM operations                       

    py/selenium/webdriver/remote/webdriver.py

    • Added methods for FedCM dialog operations in remote WebDriver.
    +49/-0   
    Tests
    3 files
    fedcm_tests.py
    Add integration tests for FedCM dialog                                     

    py/test/selenium/webdriver/chrome/fedcm_tests.py

    • Added integration tests for FedCM dialog functionality.
    +141/-0 
    account_tests.py
    Add unit tests for FedCM account properties                           

    py/test/unit/selenium/webdriver/common/fedcm/account_tests.py

    • Added unit tests for Account class properties.
    +44/-0   
    dialog_tests.py
    Add unit tests for FedCM dialog methods                                   

    py/test/unit/selenium/webdriver/common/fedcm/dialog_tests.py

    • Added unit tests for Dialog class methods.
    +76/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 4, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The wait_for_fedcm_dialog method may need additional error handling or timeout logic to prevent indefinite waiting.

    Type Annotations
    Some methods are missing return type annotations, which could improve code clarity and maintainability.

    Test Coverage
    The test suite could benefit from additional edge cases and error scenarios to ensure robust FedCM functionality.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 4, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add input validation to prevent errors from invalid account indices

    Consider adding input validation for the index parameter in the select_account
    method to ensure it's a non-negative integer within the valid range of account
    indices.

    py/selenium/webdriver/common/fedcm/dialog.py [54-56]

     def select_account(self, index: int) -> None:
         """Selects an account from the dialog by index."""
    +    if not isinstance(index, int) or index < 0:
    +        raise ValueError("Index must be a non-negative integer")
    +    accounts = self.get_accounts()
    +    if index >= len(accounts):
    +        raise IndexError("Account index out of range")
         self._driver.select_fedcm_account(index)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding input validation for the index parameter would prevent potential runtime errors and improve the robustness of the select_account method.

    8
    Use a more specific exception type for better error handling and clarity

    Consider using a more specific exception type instead of NoAlertPresentException in
    the check_fedcm function. A custom exception like FedCMDialogNotPresentException
    would be more appropriate and informative.

    py/selenium/webdriver/common/driver_extensions/has_fedcm_dialog.py [55-59]

     def check_fedcm():
         try:
             return self.fedcm_dialog if self.fedcm_dialog.type else None
    -    except NoAlertPresentException:
    +    except FedCMDialogNotPresentException:
             return None
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more specific exception type like FedCMDialogNotPresentException would improve error handling and make the code more informative and easier to debug.

    7
    Enhance test coverage by including a test for the click_continue method

    Consider adding a test case for the click_continue method of the FedCM dialog to
    ensure complete coverage of the dialog's functionality.

    py/test/selenium/webdriver/chrome/fedcm_tests.py [98-126]

     def test_fedcm_dialog_flow(self, driver):
         driver.execute_script("triggerFedCm();")
         dialog = driver.wait_for_fedcm_dialog()
     
         # Test dialog properties
         assert dialog.type == "AccountChooser"
         assert dialog.title == "Sign in to localhost with localhost"
         assert dialog.subtitle is None
     
         # Test account list
         accounts = dialog.get_accounts()
         assert len(accounts) > 0
         assert accounts[0].name == "John Doe"
     
         # Test account selection
         dialog.select_account(1)
     
         # Wait for the dialog to become interactable
         driver.wait_for_fedcm_dialog()
     
    -    # # Test dialog button click
    -    # dialog.click_continue()
    -    # driver.wait_for_fedcm_dialog()
    +    # Test dialog button click
    +    dialog.click_continue()
    +    driver.wait_for_fedcm_dialog()
     
         # Test dialog cancellation
         dialog.cancel()
         with pytest.raises(NoAlertPresentException):
             dialog.title
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Including a test for the click_continue method would improve test coverage and ensure the complete functionality of the FedCM dialog is verified.

    7
    Improve method documentation for better code readability and understanding

    Consider adding a docstring to the get_fedcm_subtitle method to explain its purpose
    and return value, maintaining consistency with other methods in the class.

    py/selenium/webdriver/remote/webdriver.py [1231-1233]

     def get_fedcm_subtitle(self) -> Optional[str]:
    -    """Gets the subtitle of the dialog."""
    +    """
    +    Gets the subtitle of the FedCM dialog.
    +
    +    Returns:
    +        Optional[str]: The subtitle of the dialog if present, otherwise None.
    +    """
         return self.execute(Command.GET_FEDCM_TITLE)["value"].get("subtitle")
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a more detailed docstring to the get_fedcm_subtitle method would improve code documentation and maintain consistency with other methods in the class.

    5

    💡 Need additional feedback ? start a PR chat

    @navin772
    Copy link
    Contributor Author

    navin772 commented Nov 4, 2024

    Bazel targets to test :

    bazel query //py:all | grep fedcm

    py/test/selenium/webdriver/chrome/fedcm_tests.py Outdated Show resolved Hide resolved
    py/selenium/webdriver/chrome/webdriver.py Outdated Show resolved Hide resolved
    py/selenium/webdriver/remote/webdriver.py Outdated Show resolved Hide resolved
    py/selenium/webdriver/remote/webdriver.py Outdated Show resolved Hide resolved
    py/selenium/webdriver/remote/webdriver.py Outdated Show resolved Hide resolved
    @navin772
    Copy link
    Contributor Author

    navin772 commented Nov 7, 2024

    @AutomatedTester the python remote integration tests are failing because we are using firefox in remote (expected error) - https://github.com/SeleniumHQ/selenium/actions/runs/11710156327/job/32616712125?pr=14710#step:16:3053

    How can I skip it in remote, I am already using xfail_firefox to skip them in RBE tests, but remote integration test execute it

    @pytest.mark.xfail_firefox(reason="FedCM not supported")

    @AutomatedTester
    Copy link
    Member

    You just need to use @pytest.mark.xfail_remote

    @VietND96 VietND96 merged commit d3d8070 into SeleniumHQ:trunk Nov 15, 2024
    16 of 17 checks passed
    @navin772 navin772 deleted the py-fedcm branch November 15, 2024 02:53
    jkim2492 pushed a commit to jkim2492/selenium that referenced this pull request Nov 17, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants