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]: add chrome and edge set_permissions docs #2058

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Nov 12, 2024

User description

Thanks for contributing to the Selenium site and documentation!
A PR well described will help maintainers to review and merge it quickly

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, and help reviewers by making them as simple and short as possible.

Description

Add chrome and edge set_permissions docs. A helper function get_permission_state is added to retrieve the permissions currently set.

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

documentation, tests


Description

  • Added tests for setting permissions in Chrome and Edge browsers using Python.
  • Introduced a helper function get_permission_state to retrieve current permission states.
  • Updated documentation for Chrome and Edge across multiple languages to include new Python examples.

Changes walkthrough 📝

Relevant files
Tests
2 files
test_chrome.py
Add permission setting test and helper function for Chrome

examples/python/tests/browsers/test_chrome.py

  • Added a test for setting permissions in Chrome.
  • Introduced a helper function get_permission_state to query permission
    states.
  • +21/-0   
    test_edge.py
    Add permission setting test and helper function for Edge 

    examples/python/tests/browsers/test_edge.py

  • Added a test for setting permissions in Edge.
  • Introduced a helper function get_permission_state to query permission
    states.
  • +21/-0   
    Documentation
    8 files
    chrome.en.md
    Update Chrome documentation with Python example                   

    website_and_docs/content/documentation/webdriver/browsers/chrome.en.md

    • Updated Python code example for Chrome permissions.
    +3/-3     
    chrome.ja.md
    Update Japanese Chrome documentation with Python example 

    website_and_docs/content/documentation/webdriver/browsers/chrome.ja.md

  • Updated Python code example for Chrome permissions in Japanese
    documentation.
  • +3/-3     
    chrome.pt-br.md
    Update Portuguese Chrome documentation with Python example

    website_and_docs/content/documentation/webdriver/browsers/chrome.pt-br.md

  • Updated Python code example for Chrome permissions in Portuguese
    documentation.
  • +3/-3     
    chrome.zh-cn.md
    Update Chinese Chrome documentation with Python example   

    website_and_docs/content/documentation/webdriver/browsers/chrome.zh-cn.md

  • Updated Python code example for Chrome permissions in Chinese
    documentation.
  • +3/-3     
    edge.en.md
    Update Edge documentation with Python example                       

    website_and_docs/content/documentation/webdriver/browsers/edge.en.md

    • Updated Python code example for Edge permissions.
    +3/-3     
    edge.ja.md
    Update Japanese Edge documentation with Python example     

    website_and_docs/content/documentation/webdriver/browsers/edge.ja.md

  • Updated Python code example for Edge permissions in Japanese
    documentation.
  • +3/-3     
    edge.pt-br.md
    Update Portuguese Edge documentation with Python example 

    website_and_docs/content/documentation/webdriver/browsers/edge.pt-br.md

  • Updated Python code example for Edge permissions in Portuguese
    documentation.
  • +3/-3     
    edge.zh-cn.md
    Update Chinese Edge documentation with Python example       

    website_and_docs/content/documentation/webdriver/browsers/edge.zh-cn.md

  • Updated Python code example for Edge permissions in Chinese
    documentation.
  • +3/-3     

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

    Copy link

    netlify bot commented Nov 12, 2024

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit fee50c2

    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

    Error Handling
    The test does not handle potential errors when querying permissions. The async script execution could fail if permissions API is not available or if there are network issues.

    Resource Cleanup
    The driver.quit() call should be in a try-finally block to ensure browser cleanup even if the test fails

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure proper resource cleanup using try-finally block

    Add cleanup in a try-finally block to ensure the driver is always quit, even if the
    test fails.

    examples/python/tests/browsers/test_chrome.py [145-152]

     def test_set_permissions():
         driver = webdriver.Chrome()
    -    driver.get('https://www.selenium.dev')
    -    driver.set_permissions('camera', 'denied')
    -    assert get_permission_state(driver, 'camera') == 'denied'
    -    driver.quit()
    +    try:
    +        driver.get('https://www.selenium.dev')
    +        driver.set_permissions('camera', 'denied')
    +        assert get_permission_state(driver, 'camera') == 'denied'
    +    finally:
    +        driver.quit()
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is a critical improvement for resource management, ensuring the WebDriver is properly closed even if the test fails, preventing resource leaks and hanging browser instances.

    8
    Possible issue
    Add error handling for invalid or unsupported permission settings

    Add error handling for the case when the permission name is invalid or not supported
    by the browser. The current code will fail silently.

    examples/python/tests/browsers/test_chrome.py [149]

    -driver.set_permissions('camera', 'denied')
    +try:
    +    driver.set_permissions('camera', 'denied')
    +except Exception as e:
    +    print(f"Failed to set permission: {e}")
    +    driver.quit()
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential reliability issue by adding error handling for invalid permissions, which could prevent silent failures in production tests.

    7
    Possible bug
    Handle potential failures in asynchronous permission state queries

    Add error handling for the asynchronous script execution which might fail or
    timeout.

    examples/python/tests/browsers/test_chrome.py [155-163]

     def get_permission_state(driver, name):
         script = """
         const callback = arguments[arguments.length - 1];
         navigator.permissions.query({name: arguments[0]}).then(permissionStatus => {
             callback(permissionStatus.state);
    +    }).catch(error => {
    +        callback(null);
         });
         """
    -    return driver.execute_async_script(script, name)
    +    result = driver.execute_async_script(script, name)
    +    if result is None:
    +        raise ValueError(f"Failed to query permission state for: {name}")
    +    return result
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling for asynchronous operations, making the code more robust by properly handling JavaScript errors and providing clear error messages.

    7

    💡 Need additional feedback ? start a PR chat

    @navin772
    Copy link
    Contributor Author

    The safari stable tests are failing due to an issue in selenium - SeleniumHQ/selenium#14698 which should be fixed once a minor release is done

    Copy link
    Member

    @harsha509 harsha509 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 @navin772 !

    @harsha509 harsha509 merged commit 9d66c22 into SeleniumHQ:trunk Nov 14, 2024
    8 of 9 checks passed
    selenium-ci added a commit that referenced this pull request Nov 14, 2024
    add chrome and edge set_permissions docs
    
    Co-authored-by: Sri Harsha <[email protected]> 9d66c22
    @navin772 navin772 deleted the set_permissions branch November 14, 2024 16:54
    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