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] Rename credentials test file so it gets run #15419

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Mar 14, 2025

User description

Motivation and Context

This PR renames credentials_test.py to credentials_tests.py so it gets run when bazel test is called. Previously, these tests were skipped due to the file name. The file is located in: py/test/unit/selenium/webdriver/virtual_authenticator/

Bazel uses the following function in py/private/suite.bzl to determine if a file contains tests when building a test suite:

def _is_test(file):
    return file.startswith("test_") or file.endswith("_tests.py")

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

Bug fix, Tests


Description

  • Renamed credentials_test.py to credentials_tests.py for proper test execution.

  • Added comprehensive unit tests for Credential class functionalities.

  • Ensured compatibility with bazel test by adhering to naming conventions.


Changes walkthrough 📝

Relevant files
Tests
credentials_tests.py
Rename and add tests for `Credential` class                           

py/test/unit/selenium/webdriver/virtual_authenticator/credentials_tests.py

  • Renamed file to ensure it is executed during tests.
  • Added tests for resident and non-resident credentials.
  • Verified to_dict and from_dict methods of Credential class.
  • Included a fixture for reusable test data setup.
  • [link]   

    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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Assertion Style

    The test uses an unusual assertion style with if-else blocks instead of direct assertions. This makes the tests harder to read and understand. Consider using direct assertions like assert credential.is_resident_credential is True instead of conditional blocks.

    if credential.is_resident_credential is True:
        assert True
    Redundant Assertions

    The test contains redundant assertions in multiple places. For example, lines 80-86 use if-else blocks that could be simplified to direct assertions, making the tests more concise and readable.

    if credential.is_resident_credential is False:
        assert True
    else:
        assert False
    if credential.user_handle is None:
        assert True
    else:
        assert False
    Inconsistent Assertion Pattern

    The test uses inconsistent assertion patterns across different test functions. Some use direct assertions while others use if-else blocks with assert True/False statements, which reduces code clarity and maintainability.

    if credential_dict["isResidentCredential"] is True:
        assert True
    else:
        assert False

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Simplify boolean assertions

    Replace the conditional assertion with a direct assertion of the boolean value.
    The current pattern creates a redundant condition that always passes regardless
    of the actual value.

    py/test/unit/selenium/webdriver/virtual_authenticator/credentials_tests.py [62-64]

     assert credential.id == urlsafe_b64encode(bytearray({1, 2, 3, 4})).decode()
    -if credential.is_resident_credential is True:
    -    assert True
    +assert credential.is_resident_credential is True
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies an unnecessarily verbose assertion pattern and proposes a cleaner, more direct assertion. This improves code readability and maintainability without changing functionality.

    Low
    Simplify negative boolean assertions

    Replace the verbose conditional assertion with a direct assertion. The current
    pattern is unnecessarily complex and can be simplified to a single assertion.

    py/test/unit/selenium/webdriver/virtual_authenticator/credentials_tests.py [79-82]

    -if credential.is_resident_credential is False:
    -    assert True
    -else:
    -    assert False
    +assert credential.is_resident_credential is False
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies a verbose and convoluted assertion pattern that can be replaced with a single, direct assertion. This simplification makes the test more readable and maintainable.

    Low
    Simplify None value assertions

    Replace the conditional assertion with a direct assertion of the None value. The
    current pattern is unnecessarily verbose and can be simplified.

    py/test/unit/selenium/webdriver/virtual_authenticator/credentials_tests.py [83-86]

    -if credential.user_handle is None:
    -    assert True
    -else:
    -    assert False
    +assert credential.user_handle is None
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies an unnecessarily complex assertion pattern for checking if a value is None. The proposed direct assertion is cleaner and more maintainable while preserving the same test logic.

    Low
    • More

    @cgoldberg cgoldberg merged commit 9e5c9b8 into SeleniumHQ:trunk Mar 17, 2025
    12 of 17 checks passed
    @cgoldberg cgoldberg deleted the rename-py-creds-test branch March 17, 2025 01:22
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    1 participant