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

[ci][rb] Fix remote tests #14679

Merged
merged 1 commit into from
Oct 30, 2024
Merged

[ci][rb] Fix remote tests #14679

merged 1 commit into from
Oct 30, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Oct 29, 2024

User description

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

It looks like an update for test fixture or something which enable downloads from the beginning, so the expectation in test not happen anymore.
Adding a step to reset the driver state with enable_downloads: false to get expect exception in test

  1) Selenium::WebDriver::Remote::Driver errors when not set
     Failure/Error:
       expect {
         driver.downloadable_files
       }.to raise_exception(Error::WebDriverError,
                            'You must enable downloads in order to work with downloadable files.')

       expected Selenium::WebDriver::Error::WebDriverError with "You must enable downloads in order to work with downloadable files.", got #<Selenium::WebDriver::Error::UnknownError: Cannot find downloads file system for session id: f94b5a8... os.arch: 'amd64', os.version: '10.0', java.version: '17.0.11'
       Driver info: driver.version: unknown> with backtrace:

Motivation and Context

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

  • Fixed a test in driver_spec.rb by adding a step to reset the driver state with enable_downloads: false.
  • Wrapped the expectation in a block to ensure the driver state is reset before checking for exceptions.
  • Ensured the test correctly raises a WebDriverError when downloads are not enabled.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Tests
driver_spec.rb
Fix test by resetting driver state for download settings 

rb/spec/integration/selenium/webdriver/remote/driver_spec.rb

  • Added a step to reset the driver state with enable_downloads: false.
  • Wrapped the expectation in a block to ensure the driver state is
    reset.
  • Ensured the test raises the correct exception when downloads are not
    enabled.
  • +6/-4     

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

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    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

    Test Modification
    The test case for 'errors when not set' has been modified. Verify if the new implementation correctly tests the intended behavior and if the reset_driver! method works as expected with enable_downloads: false.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a test case for enabled downloads to ensure complete functionality coverage

    Consider adding a test case for when downloads are enabled to ensure the method
    works correctly in both scenarios.

    rb/spec/integration/selenium/webdriver/remote/driver_spec.rb [86-91]

     reset_driver!(enable_downloads: false) do |driver|
       expect {
         driver.downloadable_files
       }.to raise_exception(Error::WebDriverError,
                            'You must enable downloads in order to work with downloadable files.')
     end
     
    +reset_driver!(enable_downloads: true) do |driver|
    +  expect(driver.downloadable_files).to be_an(Array)
    +end
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a test case for when downloads are enabled would enhance test coverage and ensure that the method works correctly in both scenarios. This suggestion is relevant and would improve the robustness of the test suite.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 added C-rb C-build Build related issues (bazel and CI) and removed C-rb labels Oct 29, 2024
    @VietND96 VietND96 merged commit 5be3015 into trunk Oct 30, 2024
    23 checks passed
    @VietND96 VietND96 deleted the ci-test branch October 30, 2024 00:42
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    C-build Build related issues (bazel and CI) Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant