Skip to content

[py] Fix FedCM tests leaking state #15583

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

Merged
merged 2 commits into from
Apr 7, 2025

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Apr 7, 2025

User description

🔗 Related Issues

Fixes #15581

💥 What does this PR do?

This fixes an issue where FedCM tests were failing in the internal Python test suite located in py/test/selenium/webdriver/common/fedcm_tests.py.

The tests would open the FedCM dialog, but never close it... which would lead to subsequent tests failing in odd ways. This PR adds code to dismiss the dialog in several tests after the test completes. Now state is not leaked and all tests pass.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Fixes FedCM tests leaking state by dismissing dialogs.

  • Adds dialog.dismiss() to multiple test cases to ensure cleanup.

  • Resolves test failures caused by unclosed FedCM dialogs.


Changes walkthrough 📝

Relevant files
Tests
fedcm_tests.py
Add dialog dismissal to FedCM tests                                           

py/test/selenium/webdriver/common/fedcm_tests.py

  • Added dialog.dismiss() to ensure dialogs are closed after tests.
  • Updated multiple test cases to prevent state leakage.
  • Improved test reliability by cleaning up dialog state.
  • +6/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Apr 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @selenium-ci selenium-ci added the C-py Python Bindings label Apr 7, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Complete dialog cancellation test

    The test_dialog_cancel method is missing the actual dialog cancellation action
    and assertion to verify the cancellation behavior. Also, it doesn't dismiss the
    dialog, which could lead to state leakage.

    py/test/selenium/webdriver/common/fedcm_tests.py [95-97]

     def test_dialog_cancel(self, driver):
         driver.execute_script("triggerFedCm();")
         dialog = driver.fedcm_dialog()
    +    dialog.cancel()
    +    # Add appropriate assertion here to verify cancellation
    +    # For example: assert some_condition
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion identifies an incomplete test method that's missing both the actual cancellation action and verification. The improvement adds the missing dialog.cancel() call and suggests adding appropriate assertions, which would significantly improve test coverage and prevent potential state leakage.

    Medium
    General
    Remove commented code

    The commented out dialog.click_continue() line suggests an incomplete test
    implementation. Either remove the commented code if it's not needed or implement
    the continue functionality if it's part of the test's intent.

    py/test/selenium/webdriver/common/fedcm_tests.py [87-93]

     def test_select_account(self, driver):
         driver.execute_script("triggerFedCm();")
         dialog = driver.fedcm_dialog()
         dialog.select_account(1)
         driver.fedcm_dialog()  # Wait for dialog to become interactable
    -    # dialog.click_continue()
         dialog.dismiss()
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    __

    Why: The suggestion correctly identifies and removes commented code that appears to be unused. This improves code cleanliness and readability by eliminating potentially confusing commented code, though it has minimal impact on functionality.

    Low
    • Update

    Copy link
    Member

    @navin772 navin772 left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @cgoldberg
    Copy link
    Contributor Author

    The test failures in CI have nothing to do with this PR... I will investigate them separately.

    @cgoldberg cgoldberg merged commit 55d0708 into SeleniumHQ:trunk Apr 7, 2025
    4 checks passed
    @cgoldberg cgoldberg deleted the py-fedcm-dismiss-dialog branch April 7, 2025 02:42
    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.

    [🐛 Bug]: [py] FedCM dialog not opening
    3 participants