Skip to content

Add permissions examples and update the log examples #1731

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

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented May 20, 2024

User description

Description

This PR adds examples for permissions in Chrome using Ruby, it also updates and organizes the logs test so they are under the same describe group

Motivation and Context

The documentation must be up to date so users can find concrete examples and be aware of all the possibilities that Selenium has to offer

Right now the example looks like this:

Screenshot 2024-05-20 at 17 40 55

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

Enhancement, Documentation


Description

  • Added a new test for setting permissions in Chrome using Ruby.
  • Refactored the existing test for getting browser logs in Chrome.
  • Introduced a helper method permission to check permission states in tests.
  • Updated code block references for Ruby examples in Chrome documentation across multiple languages (EN, JA, PT-BR, ZH-CN).

Changes walkthrough 📝

Relevant files
Enhancement
chrome_spec.rb
Add and refactor tests for Chrome browser permissions and logs

examples/ruby/spec/browsers/chrome_spec.rb

  • Added a new test for setting permissions in Chrome.
  • Refactored the existing test for getting browser logs.
  • Introduced a helper method permission to check permission states.
  • +22/-7   
    Documentation
    chrome.en.md
    Update Ruby code block references in Chrome documentation (EN)

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

    • Updated code block references for Ruby examples.
    +2/-2     
    chrome.ja.md
    Update Ruby code block references in Chrome documentation (JA)

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

    • Updated code block references for Ruby examples.
    +2/-2     
    chrome.pt-br.md
    Update Ruby code block references in Chrome documentation (PT-BR)

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

    • Updated code block references for Ruby examples.
    +2/-2     
    chrome.zh-cn.md
    Update Ruby code block references in Chrome documentation (ZH-CN)

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

    • Updated code block references for Ruby examples.
    +2/-2     

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

    Copy link

    netlify bot commented May 20, 2024

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 8caf46a

    @qodo-merge-pro qodo-merge-pro bot added documentation Improvements or additions to documentation enhancement New feature or request labels May 20, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (8caf46a)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly straightforward with the addition of new tests and minor updates to documentation. The code is well-structured and mainly involves standard API usage and test assertions.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The permission method uses an asynchronous script to query permissions but does not handle potential errors or rejections from the navigator.permissions.query method. This could lead to unhandled promise rejections if the query fails.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Replace sleep with Selenium's wait functionality to make the test more reliable

    Instead of using sleep 1 to wait for the page to load, consider using Selenium's built-in
    wait functionality to wait for a specific condition, such as the presence of an element or
    a specific state.

    examples/ruby/spec/browsers/chrome_spec.rb [140-141]

    -sleep 1
    +wait = Selenium::WebDriver::Wait.new(timeout: 10)
    +wait.until { @driver.find_element(tag_name: 'body') }
     logs = @driver.logs.get(:browser)
     
    Suggestion importance[1-10]: 8

    Why: Replacing sleep with Selenium's wait functionality improves test reliability and efficiency by waiting for specific conditions rather than a fixed time.

    8
    Add proper cleanup to ensure the browser is closed after the test

    Ensure that the @driver instance is properly quit after the test to avoid leaving the
    browser open.

    examples/ruby/spec/browsers/chrome_spec.rb [138-143]

     @driver = Selenium::WebDriver.for :chrome
    -@driver.navigate.to 'https://www.selenium.dev/selenium/web/'
    -sleep 1
    -logs = @driver.logs.get(:browser)
    -expect(logs.first.message).to include 'Failed to load resource'
    +begin
    +  @driver.navigate.to 'https://www.selenium.dev/selenium/web/'
    +  wait = Selenium::WebDriver::Wait.new(timeout: 10)
    +  wait.until { @driver.find_element(tag_name: 'body') }
    +  logs = @driver.logs.get(:browser)
    +  expect(logs.first.message).to include 'Failed to load resource'
    +ensure
    +  @driver.quit
    +end
     
    Suggestion importance[1-10]: 7

    Why: Ensuring that the @driver instance is properly quit after the test is a best practice to avoid resource leakage, making this suggestion valuable.

    7
    Possible issue
    Add error handling to the permission method to manage potential failures in permission queries

    Add error handling for the permission method to manage cases where the permission query
    might fail or return unexpected results.

    examples/ruby/spec/browsers/chrome_spec.rb [166-167]

    -@driver.execute_async_script('callback = arguments[arguments.length - 1];' \
    -                             'callback(navigator.permissions.query({name: arguments[0]}));', name)['state']
    +result = @driver.execute_async_script('callback = arguments[arguments.length - 1];' \
    +                                      'navigator.permissions.query({name: arguments[0]}).then(callback).catch(callback);', name)
    +result['state'] rescue 'unknown'
     
    Suggestion importance[1-10]: 7

    Why: Adding error handling to the permission method is crucial for robustness, especially when dealing with asynchronous operations that might fail.

    7
    Enhancement
    Combine multiple add_permissions calls into one for conciseness

    Combine the two add_permissions calls into one to make the code more concise and
    efficient.

    examples/ruby/spec/browsers/chrome_spec.rb [149-150]

    -@driver.add_permission('camera', 'denied')
    -@driver.add_permissions('clipboard-read' => 'denied', 'clipboard-write' => 'prompt')
    +@driver.add_permissions('camera' => 'denied', 'clipboard-read' => 'denied', 'clipboard-write' => 'prompt')
     
    Suggestion importance[1-10]: 6

    Why: Combining the add_permissions calls into one enhances code conciseness and reduces redundancy, although it's a relatively minor enhancement.

    6

    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 @aguspe !

    @harsha509 harsha509 merged commit 7b6f8c2 into SeleniumHQ:trunk May 21, 2024
    9 checks passed
    selenium-ci added a commit that referenced this pull request May 21, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants