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

Refactor ChromeDriverFunctionalTest: #14398

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

manuelsblanco
Copy link
Contributor

@manuelsblanco manuelsblanco commented Aug 14, 2024

User description

Remove redundant permission cons…tants

Removed the CLIPBOARD_READ and CLIPBOARD_WRITE constants from the class level in ChromeDriverFunctionalTest to avoid redundancy. These constants are now defined within the canSetPermission method, reducing unnecessary visibility and improving code cohesion.

This change simplifies maintenance and enhances code clarity without altering existing functionality.

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

In this commit, I refactored the ChromeDriverFunctionalTest class by removing the redundant CLIPBOARD_READ and CLIPBOARD_WRITE constants from the class level. These constants are now defined within the canSetPermission method where they are used, which reduces unnecessary visibility and improves the cohesion of the code. This change helps to simplify the structure and readability of the test code without impacting its functionality.

Motivation and Context

The change is required to improve the maintainability and clarity of the ChromeDriverFunctionalTest code. By removing the redundant constants from the class level and defining them within the specific method where they are used, we reduce unnecessary global visibility and enhance the code's cohesion. This refactor makes the code easier to understand and maintain, ensuring that constants are only accessible where they are needed, which is a better practice for clean and organized code.

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.
  • [x ] All new and existing tests passed.

PR Type

enhancement


Description

  • Refactored ChromeDriverFunctionalTest by removing redundant class-level constants CLIPBOARD_READ and CLIPBOARD_WRITE.
  • Defined these constants within the canSetPermission method to improve code cohesion.
  • Simplified the code structure, enhancing maintainability and clarity without altering functionality.

Changes walkthrough 📝

Relevant files
Enhancement
ChromeDriverFunctionalTest.java
Refactor to remove redundant permission constants               

java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java

  • Removed class-level constants CLIPBOARD_READ and CLIPBOARD_WRITE.
  • Defined CLIPBOARD_READ and CLIPBOARD_WRITE within the canSetPermission
    method.
  • Improved code cohesion and reduced unnecessary visibility.
  • +2/-3     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    …tants
    
    Removed the CLIPBOARD_READ and CLIPBOARD_WRITE constants from the class level in ChromeDriverFunctionalTest to avoid redundancy. These constants are now defined within the canSetPermission method, reducing unnecessary visibility and improving code cohesion.
    
    This change simplifies maintenance and enhances code clarity without altering existing functionality.
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Move constants back to class level for better code organization and reusability

    Consider moving the CLIPBOARD_READ and CLIPBOARD_WRITE constants back to class
    level, as they seem to be used in multiple methods and are unlikely to change.

    java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java [109-112]

    -String CLIPBOARD_READ = "clipboard-read";
     assumeThat(checkPermission(driver, CLIPBOARD_READ)).isEqualTo("prompt");
    -String CLIPBOARD_WRITE = "clipboard-write";
     assumeThat(checkPermission(driver, CLIPBOARD_WRITE)).isEqualTo("granted");
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Moving the constants back to the class level is a good practice as they are used in multiple places and are unlikely to change, improving code organization and reusability.

    9
    Maintainability
    Extract permission checking logic into a separate method for better code organization

    Consider extracting the permission checking logic into a separate method to improve
    code readability and maintainability.

    java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java [110-112]

    -assumeThat(checkPermission(driver, CLIPBOARD_READ)).isEqualTo("prompt");
    -assumeThat(checkPermission(driver, CLIPBOARD_WRITE)).isEqualTo("granted");
    +assertClipboardPermissions(driver, CLIPBOARD_READ, "prompt");
    +assertClipboardPermissions(driver, CLIPBOARD_WRITE, "granted");
     
    +// Add this method to the class:
    +private void assertClipboardPermissions(WebDriver driver, String permission, String expectedStatus) {
    +    assumeThat(checkPermission(driver, permission)).isEqualTo(expectedStatus);
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Extracting the permission checking logic into a separate method improves code readability and maintainability by reducing duplication and clarifying the intent of the code.

    8
    Enhancement
    Use static imports for assertion methods to improve code readability

    Consider using static imports for assumeThat to improve code readability and reduce
    verbosity.

    java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java [110-112]

    +import static org.assertj.core.api.Assumptions.assumeThat;
    +
    +// In the method:
     assumeThat(checkPermission(driver, CLIPBOARD_READ)).isEqualTo("prompt");
     assumeThat(checkPermission(driver, CLIPBOARD_WRITE)).isEqualTo("granted");
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using static imports for assertion methods can enhance code readability by reducing verbosity, although the improvement is minor.

    7

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @manuelsblanco!

    @diemol diemol merged commit 25c8376 into SeleniumHQ:trunk Aug 19, 2024
    9 of 11 checks passed
    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.

    2 participants