Skip to content

[rb] Fix firefox pipeline by removing guards #14277

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
merged 9 commits into from
Jul 24, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jul 19, 2024

User description

Description

This PR focuses on removing guards for successful tests where the expectation is for the test to fail, so not have the pipeline failing even if the tests are succeding

Motivation and Context

It's important for releasing and provide a clear overview that the reason for the failure or success of the tests is clear and understandable

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


Description

  • Removed guards that were causing Firefox-specific tests to be skipped.
  • Ensured that tests 'moves to element with offset' and 'scrolls by given amount' run on Firefox without exclusions.

Changes walkthrough 📝

Relevant files
Bug fix
action_builder_spec.rb
Remove guards for Firefox-specific test cases                       

rb/spec/integration/selenium/webdriver/action_builder_spec.rb

  • Removed guards for Firefox on 'moves to element with offset' test.
  • Removed guards for Firefox on 'scrolls by given amount' test.
  • +2/-6     

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

    Sorry, something went wrong.

    Verified

    This commit was signed with the committer’s verified signature. The key has expired.
    chrischdi Christian Schlotter
    @qodo-merge-pro qodo-merge-pro bot added P-bug fix PR addresses a known issue Review effort [1-5]: 2 labels Jul 19, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Test Coverage
    The PR removes guards for Firefox-specific tests but does not include any new tests or modifications to existing tests to ensure that the previously guarded behavior now functions as expected across all platforms.

    Test Coverage
    Similar to the previous issue, the removal of guards for the 'scrolls by given amount' test for Firefox on macOSX requires verification through updated or new tests to confirm that the issue previously noted has been resolved.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 19, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add verification to ensure the 'move to element with offset' action is performed correctly

    Ensure that the test for moving to an element with an offset includes proper error
    handling or a specific assertion to verify the behavior, especially since the
    previous version of the test excluded Firefox due to resolution issues. This could
    involve checking the position of the destination element after the action is
    performed.

    rb/spec/integration/selenium/webdriver/action_builder_spec.rb [205-208]

     it 'moves to element with offset' do
       driver.navigate.to url_for('javascriptPage.html')
       origin = driver.find_element(id: 'keyUpArea')
       destination = driver.find_element(id: 'clickField')
    +  driver.action.move_to(destination, 10, 10).perform
    +  expect(destination.location.x).to be > origin.location.x
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds a verification step to ensure the action is performed correctly, which is important given the previous exclusion of Firefox due to resolution issues. This improves the robustness of the test.

    8
    Ensure the scroll functionality is verified with an appropriate assertion

    Add an assertion to check the scroll functionality in the 'scrolls by given amount'
    test, particularly since the previous version excluded Firefox on macOS due to
    insufficient scrolling. This could involve verifying the final position of the
    footer or another element within the view.

    rb/spec/integration/selenium/webdriver/action_builder_spec.rb [320-323]

     it 'scrolls by given amount' do
       driver.navigate.to url_for('scrolling_tests/frame_with_nested_scrolling_frame_out_of_view.html')
       footer = driver.find_element(tag_name: 'footer')
       delta_y = footer.rect.y.round
    +  driver.execute_script('window.scrollTo(0, arguments[0])', delta_y)
    +  expect(driver.find_element(tag_name: 'footer').location.y).to eq(delta_y)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding an assertion to verify the scroll functionality addresses the previous issue with Firefox on macOS and ensures the test accurately reflects the intended behavior. This enhances the reliability of the test.

    8

    Sorry, something went wrong.

    @pujagani pujagani requested review from p0deje and titusfortner and removed request for p0deje July 24, 2024 04:47
    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 24, 2024

    @p0deje Good morning, I just ran the pipeline for Firefox locally on my M1 with Sonoma and both the action_builder integration spec and the driver spec run fine, and the first time this pipeline ran it was green, so can we just run it one more time to double-check?

    Thank you so much!

    aguspe and others added 3 commits July 24, 2024 13:16
    @p0deje p0deje merged commit 5d1b216 into SeleniumHQ:trunk Jul 24, 2024
    23 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    P-bug fix PR addresses a known issue Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants