Skip to content

[rb] move all guard and zipper tests to unit tests #15717

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 7, 2025
Merged

[rb] move all guard and zipper tests to unit tests #15717

merged 2 commits into from
May 7, 2025

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented May 7, 2025 •
edited by qodo-merge-pro bot
Loading

User description

💥 What does this PR do?

As integration tests this creates test endpoints for all of the browsers, this is more accurately unit tests

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Tests, Other

Description

  • Moved guard tests from integration to unit tests

  • Updated zipper tests to remove unnecessary exclusive guard

  • Cleaned up test organization for clarity

Changes walkthrough 📝

Relevant files
Tests
guard_spec.rb
Remove guard integration tests                                                     

rb/spec/integration/selenium/webdriver/guard_spec.rb

  • Removed all guard-related integration tests
  • Tests relocated to unit test suite for better categorization
    		• +0/-107 
    guards_spec.rb
    Add guard tests as unit tests                                                       

    rb/spec/unit/selenium/webdriver/guards_spec.rb

  • Added comprehensive guard tests as unit tests
  • Covers exclude, flaky, exclusive, only, except, and multiple guard
    scenarios
  • Includes custom guard condition setup for unit testing
    		• +116/-0 
    zipper_spec.rb
    Update zipper unit test description                                           

    rb/spec/unit/selenium/webdriver/zipper_spec.rb

  • Removed exclusive guard from zipper unit test description
  • Ensured zipper tests are pure unit tests without browser dependencies
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added the C-rb Ruby Bindings label May 7, 2025
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 7, 2025

    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 Expectations

    Several tests contain 'raise' statements that are expected to fail, but the test framework might not properly handle these as expected failures. Verify that the test framework correctly interprets these as expected failures rather than actual test failures.

     
          raise 'This code is executed but expected to fail'
    end

    it 'does not guard when value matches', only: { condition: :guarded } do
      # pass
    end
  end

  describe '#except' do
    it 'guards when value matches and test fails', except: { condition: :guarded } do
      raise 'This code is executed but expected to fail'
    end

    it 'does not guard when value does not match and test passes', except: { condition: :not_guarded } do
      # pass
    end
  end
end

context 'when multiple guards' do
  it 'guards if neither only nor except match and test fails', except: { condition: :not_guarded },
     only: { condition: :not_guarded } do
    raise 'This code is executed but expected to fail'
  end

  it 'guards if both only and except match', except: { condition: :guarded }, only: { condition: :guarded } do
    raise 'This code is executed but expected to fail'
  end

  it 'guards if except matches and only does not', except: { condition: :guarded }, only: { condition: :not_guarded } do
    raise 'This code is executed but expected to fail'
  end

  it 'does not guard if only matches and except does not', except: { condition: :not_guarded },
     only: { condition: :guarded } do
    # pass
  end

  it 'gives correct reason', except: [{ condition: :guarded, reason: 'bug1' },
                                      { condition: :not_guarded, reason: 'bug2' }] do
    raise 'This code is executed but expected to fail'
  end

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 7, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Prevent arbitrary method execution

    *The send(results) call is unsafe as it will execute any method name returned in
    results. This could lead to arbitrary method execution. Consider using a more
    explicit approach with a whitelist of allowed methods.

    rb/spec/unit/selenium/webdriver/guards_spec.rb [27-33]

     before do |example|
   guards = described_class.new(example, bug_tracker: 'https://github.com/SeleniumHQ/selenium/issues')
   guards.add_condition(:condition, :guarded)
 
   results = guards.disposition
-  send(*results) if results
+  if results && [:skip, :pending].include?(results.first)
+    send(*results)
+  end
 end
    • Apply / Chat
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a significant security vulnerability by restricting the send(*results) call to only allow specific safe methods (skip and pending). Without this restriction, the code could execute arbitrary methods, potentially leading to security issues.

    		High
    • Update

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 7, 2025

    CI Feedback 🧐

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Format / Check format script run

    Failed stage: Run Bazel [❌]

    Failed test name: //rb:lint

    Failure summary:

    The action failed during the Ruby linting process with Bazel build errors:

    1. Missing target error: The build system could not find the target guard in the package
    rb/spec/integration/selenium/webdriver.
    2. Specifically, the error occurred because the file
    /home/runner/work/selenium/selenium/rb/spec/integration/selenium/webdriver/BUILD.bazel does not
    declare a target named 'guard'.
    3. This missing target is referenced in
    /home/runner/work/selenium/selenium/rb/spec/BUILD.bazel at line 13.
    4. The analysis of target
    '//rb:lint' failed, causing the entire build to abort.

    Relevant error logs: 
    1:  ##[group]Operating System
2:  Ubuntu
...

925:  Package 'php-sql-formatter' is not installed, so not removed
926:  Package 'php8.3-ssh2' is not installed, so not removed
927:  Package 'php-ssh2-all-dev' is not installed, so not removed
928:  Package 'php8.3-stomp' is not installed, so not removed
929:  Package 'php-stomp-all-dev' is not installed, so not removed
930:  Package 'php-swiftmailer' is not installed, so not removed
931:  Package 'php-symfony' is not installed, so not removed
932:  Package 'php-symfony-asset' is not installed, so not removed
933:  Package 'php-symfony-asset-mapper' is not installed, so not removed
934:  Package 'php-symfony-browser-kit' is not installed, so not removed
935:  Package 'php-symfony-clock' is not installed, so not removed
936:  Package 'php-symfony-debug-bundle' is not installed, so not removed
937:  Package 'php-symfony-doctrine-bridge' is not installed, so not removed
938:  Package 'php-symfony-dom-crawler' is not installed, so not removed
939:  Package 'php-symfony-dotenv' is not installed, so not removed
940:  Package 'php-symfony-error-handler' is not installed, so not removed
941:  Package 'php-symfony-event-dispatcher' is not installed, so not removed
...

1119:  Package 'php-twig-html-extra' is not installed, so not removed
1120:  Package 'php-twig-i18n-extension' is not installed, so not removed
1121:  Package 'php-twig-inky-extra' is not installed, so not removed
1122:  Package 'php-twig-intl-extra' is not installed, so not removed
1123:  Package 'php-twig-markdown-extra' is not installed, so not removed
1124:  Package 'php-twig-string-extra' is not installed, so not removed
1125:  Package 'php8.3-uopz' is not installed, so not removed
1126:  Package 'php-uopz-all-dev' is not installed, so not removed
1127:  Package 'php8.3-uploadprogress' is not installed, so not removed
1128:  Package 'php-uploadprogress-all-dev' is not installed, so not removed
1129:  Package 'php8.3-uuid' is not installed, so not removed
1130:  Package 'php-uuid-all-dev' is not installed, so not removed
1131:  Package 'php-validate' is not installed, so not removed
1132:  Package 'php-vlucas-phpdotenv' is not installed, so not removed
1133:  Package 'php-voku-portable-ascii' is not installed, so not removed
1134:  Package 'php-wmerrors' is not installed, so not removed
1135:  Package 'php-xdebug-all-dev' is not installed, so not removed
...

1861:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/example/headless.js 4ms (unchanged)
1862:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/example/logging.js 7ms (unchanged)
1863:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/firefox.js 41ms (unchanged)
1864:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/http/index.js 19ms (unchanged)
1865:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/http/util.js 10ms (unchanged)
1866:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/ie.js 15ms (unchanged)
1867:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/index.js 24ms (unchanged)
1868:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/io/exec.js 7ms (unchanged)
1869:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/io/index.js 13ms (unchanged)
1870:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/io/zip.js 8ms (unchanged)
1871:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/jsdoc_conf.json 2ms (unchanged)
1872:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/lib/atoms/make-atoms-module.js 2ms (unchanged)
1873:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/lib/by.js 17ms (unchanged)
1874:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/lib/capabilities.js 20ms (unchanged)
1875:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/lib/command.js 10ms (unchanged)
1876:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/lib/error.js 27ms (unchanged)
1877:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/lib/fedcm/account.js 3ms (unchanged)
...

1946:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/fingerprint_test.js 3ms (unchanged)
1947:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/firefox/addon_test.js 5ms (unchanged)
1948:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/firefox/contextSwitching_test.js 3ms (unchanged)
1949:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/firefox/full_page_screenshot_test.js 3ms (unchanged)
1950:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/firefox/options_test.js 10ms (unchanged)
1951:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/frame_test.js 7ms (unchanged)
1952:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/http/http_test.js 14ms (unchanged)
1953:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/http/util_test.js 7ms (unchanged)
1954:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/ie/options_test.js 5ms (unchanged)
1955:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/io/io_test.js 29ms (unchanged)
1956:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/io/zip_test.js 9ms (unchanged)
1957:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/lib/api_test.js 2ms (unchanged)
1958:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/lib/by_test.js 8ms (unchanged)
1959:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/lib/capabilities_test.js 13ms (unchanged)
1960:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/lib/credentials_test.js 7ms (unchanged)
1961:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/lib/error_test.js 13ms (unchanged)
1962:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/lib/form_submit_test.js 3ms (unchanged)
...

1983:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/select_test.js 12ms (unchanged)
1984:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/stale_element_test.js 4ms (unchanged)
1985:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/upload_test.js 9ms (unchanged)
1986:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/virtualAuthenticator_test.js 17ms (unchanged)
1987:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/webComponent_test.js 5ms (unchanged)
1988:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/test/window_test.js 12ms (unchanged)
1989:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/testing/index.js 14ms (unchanged)
1990:  ../../../../../../../../../../work/selenium/selenium/javascript/selenium-webdriver/tools/init_jasmine.js 1ms (unchanged)
1991:  �[0m- Ruby
1992:  rubocop
1993:  �[32mComputing main repo mapping:�[0m 
1994:  �[32mLoading:�[0m 
1995:  �[32mLoading:�[0m 0 packages loaded
1996:  �[32mAnalyzing:�[0m target //rb:lint (1 packages loaded, 0 targets configured)
1997:  �[32mAnalyzing:�[0m target //rb:lint (1 packages loaded, 0 targets configured)
1998:  �[31m�[1mERROR: �[0m/home/runner/work/selenium/selenium/rb/spec/integration/selenium/webdriver/BUILD.bazel: no such target '//rb/spec/integration/selenium/webdriver:guard': target 'guard' not declared in package 'rb/spec/integration/selenium/webdriver' defined by /home/runner/work/selenium/selenium/rb/spec/integration/selenium/webdriver/BUILD.bazel
1999:  �[31m�[1mERROR: �[0m/home/runner/work/selenium/selenium/rb/spec/BUILD.bazel:13:11: no such target '//rb/spec/integration/selenium/webdriver:guard': target 'guard' not declared in package 'rb/spec/integration/selenium/webdriver' defined by /home/runner/work/selenium/selenium/rb/spec/integration/selenium/webdriver/BUILD.bazel and referenced by '//rb/spec:spec'
2000:  �[31m�[1mERROR: �[0mAnalysis of target '//rb:lint' failed; build aborted: Analysis failed
2001:  �[32mINFO: �[0mElapsed time: 0.543s, Critical Path: 0.04s
2002:  �[32mINFO: �[0m1 process: 1 internal.
2003:  �[31m�[1mERROR: �[0mBuild did NOT complete successfully
2004:  �[31m�[1mFAILED:�[0m 
2005:  �[31m�[1mERROR: �[0mBuild failed. Not running target
2006:  �[0m
2007:  ##[error]Process completed with exit code 1.
2008:  Post job cleanup.

    @titusfortner titusfortner merged commit 80a8879 into trunk May 7, 2025
    22 checks passed
    @titusfortner titusfortner deleted the rb_specs branch May 7, 2025 19:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    C-rb Ruby Bindings Review effort 2/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @titusfortner @selenium-ci