Skip to content

Add firefox special features using ruby and migrate examples #1744

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 29, 2024

User description

Description

This PR adds documentation for the following special features on firefox:

  • Full page screenshot
  • Context

It also migrates the ruby examples for profile to the correspondent spec file

Motivation and Context

It's important to keep the documentation neat to make it easier for maintainers and users to contribute and read the documentation respectively, it's also important to have full-fledged documentation for users.

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, Enhancement


Description

  • Added new tests for Firefox special features in Ruby, including full page screenshot and context setting.
  • Migrated the Ruby example for profile creation to the corresponding spec file.
  • Updated Firefox documentation in multiple languages (English, Japanese, Portuguese, Chinese) to reference the new Ruby examples.

Changes walkthrough 📝

Relevant files
Enhancement
firefox_spec.rb
Add Firefox special features tests and migrate profile example.

examples/ruby/spec/browsers/firefox_spec.rb

  • Added test for taking full page screenshot.
  • Added test for setting the context.
  • Migrated profile creation example to spec file.
  • +23/-0   
    Documentation
    firefox.en.md
    Update Firefox documentation with new Ruby examples.         

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

  • Updated Ruby example for profile creation to use code block reference.
  • Added references to new Ruby examples for full page screenshot and
    context setting.
  • +4/-7     
    firefox.ja.md
    Update Firefox documentation with new Ruby examples (Japanese).

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

  • Updated Ruby example for profile creation to use code block reference.
  • Added references to new Ruby examples for full page screenshot and
    context setting.
  • +4/-7     
    firefox.pt-br.md
    Update Firefox documentation with new Ruby examples (Portuguese).

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

  • Updated Ruby example for profile creation to use code block reference.
  • Added references to new Ruby examples for full page screenshot and
    context setting.
  • +4/-7     
    firefox.zh-cn.md
    Update Firefox documentation with new Ruby examples (Chinese).

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

  • Updated Ruby example for profile creation to use code block reference.
  • Added references to new Ruby examples for full page screenshot and
    context setting.
  • +4/-7     

    💡 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 29, 2024

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 7a76f91

    @qodo-merge-pro qodo-merge-pro bot added documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 2 labels May 29, 2024
    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly straightforward, involving updates to documentation and test cases in Ruby. The PR is well-organized with clear descriptions of the changes, which simplifies the review process.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The test 'takes full page screenshot' assumes that the screenshot file is always successfully created and is a File object. It would be safer to add checks to ensure the file actually exists and is not empty.

    Hardcoded Path: The test 'creates a new profile' uses a hardcoded path for 'browser.download.dir'. Consider using a configurable path or environment variable to enhance flexibility and compatibility across different environments.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a cleanup step to ensure the temporary directory is removed after the test

    The Dir.mktmpdir block should include a cleanup step to ensure that the temporary
    directory is removed after the test is complete, even if an error occurs. This can be done
    by adding an ensure block.

    examples/ruby/spec/browsers/firefox_spec.rb [124-128]

     Dir.mktmpdir('screenshot_test') do |dir|
    -  screenshot = driver.save_full_page_screenshot(File.join(dir, 'screenshot.png'))
    -
    -  expect(screenshot).to be_a File
    +  begin
    +    screenshot = driver.save_full_page_screenshot(File.join(dir, 'screenshot.png'))
    +    expect(screenshot).to be_a File
    +  ensure
    +    FileUtils.remove_entry(dir)
    +  end
     end
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a best practice to clean up resources in tests, which is crucial to avoid side effects in a testing environment.

    8
    Possible bug
    Add a check to ensure the driver object is not nil before setting the context

    Add a check to ensure that the driver object is not nil before attempting to set the
    context, to avoid potential NoMethodError.

    examples/ruby/spec/browsers/firefox_spec.rb [132-133]

    -driver.context = 'content'
    -expect(driver.context).to eq 'content'
    +if driver
    +  driver.context = 'content'
    +  expect(driver.context).to eq 'content'
    +else
    +  raise 'Driver is not initialized'
    +end
     
    Suggestion importance[1-10]: 7

    Why: This is a valid suggestion to prevent runtime errors due to uninitialized objects, which improves the robustness of the code.

    7
    Possible issue
    Validate the profile directory exists and is writable before setting the download directory

    Add a validation step to ensure the profile directory exists and is writable before
    setting the browser.download.dir property.

    examples/ruby/spec/browsers/firefox_spec.rb [140]

    -profile['browser.download.dir'] = '/tmp/webdriver-downloads'
    +download_dir = '/tmp/webdriver-downloads'
    +if Dir.exist?(download_dir) && File.writable?(download_dir)
    +  profile['browser.download.dir'] = download_dir
    +else
    +  raise "Download directory #{download_dir} does not exist or is not writable"
    +end
     
    Suggestion importance[1-10]: 7

    Why: Ensuring directory existence and write permissions before using it for downloads is a good practice to prevent runtime errors, making the suggestion relevant and useful.

    7
    Maintainability
    Remove the text=true attribute from the Ruby tab header for consistency

    Ensure consistency in the tab headers by removing the text=true attribute from the Ruby
    tab, as it is not present in other tabs.

    website_and_docs/content/documentation/webdriver/browsers/firefox.en.md [125]

    -{{< tab header="Ruby" text=true >}}
    +{{< tab header="Ruby" >}}
     
    Suggestion importance[1-10]: 5

    Why: While this suggestion improves consistency across documentation, it is a minor issue related to documentation formatting and not critical to functionality or performance.

    5

    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 cb2ea86 into SeleniumHQ:trunk May 30, 2024
    9 checks passed
    selenium-ci added a commit that referenced this pull request May 30, 2024
    …deploy site]
    
    * Add all special features examples for firefox using ruby
    
    * Migrate code to examples
    
    ---------
    
    Co-authored-by: aguspe <[email protected]>
    Co-authored-by: Sri Harsha <[email protected]> cb2ea86
    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