Skip to content

[rb] Use rbs trace #15686

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

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

[rb] Use rbs trace #15686

wants to merge 5 commits into from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Apr 30, 2025

User description

💥 What does this PR do?

This PR adds RBS trace to make it easier to create, maintain, and add RBS-related comments on execution

🔧 Implementation Notes

I implemented at the RSpec level, so by executing all the tests, all the tested code will have RBS comments added

💡 Additional Considerations

I think is important for us to decide which types of comments we want to keep

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Documentation


Description

  • Add RBS trace integration to test suite

    • Automatically generates RBS comments during test execution
    • Saves RBS comments after suite completion
  • Annotate Ruby source and test files with @rbs type signatures

  • Add rbs-trace gem dependency to Gemfile

  • Update test exclusion for Chrome in action builder spec


Changes walkthrough 📝

Relevant files
Dependencies
1 files
Gemfile
Add rbs-trace gem dependency                                                         
+2/-0     
Documentation
35 files
webdriver.rb
Add RBS type annotation for logger method                               
+1/-0     
chrome.rb
Add RBS type annotation for path method                                   
+1/-0     
driver.rb
Add RBS type annotations for initialize and browser methods
+2/-0     
features.rb
Add RBS type annotations for command_list and commands methods
+2/-0     
options.rb
Add RBS type annotation for binary_path method                     
+1/-0     
service.rb
Add RBS type annotation for log method                                     
+1/-0     
features.rb
Add RBS type annotation for send_command method                   
+1/-0     
options.rb
Add RBS type annotations for initialize, process_browser_options, and
camelize? methods
+3/-0     
child_process.rb
Add RBS type annotations for process methods                         
+7/-0     
driver.rb
Add RBS type annotations for driver methods                           
+8/-0     
has_cdp.rb
Add RBS type annotation for execute_cdp method                     
+1/-0     
driver_finder.rb
Add RBS type annotations for driver finder methods             
+6/-0     
error.rb
Add RBS type annotation for for_error method                         
+1/-0     
local_driver.rb
Add RBS type annotations for local driver methods               
+2/-0     
logger.rb
Add RBS type annotations for logger methods                           
+2/-0     
navigation.rb
Add RBS type annotations for navigation methods                   
+2/-0     
options.rb
Add RBS type annotations for options methods                         
+9/-0     
platform.rb
Add RBS type annotations for platform methods                       
+15/-0   
port_prober.rb
Add RBS type annotations for port prober methods                 
+2/-0     
selenium_manager.rb
Add RBS type annotations for selenium manager methods       
+6/-0     
service.rb
Add RBS type annotations for service methods                         
+5/-0     
service_manager.rb
Add RBS type annotations for service manager methods         
+14/-0   
socket_lock.rb
Add RBS type annotations for socket lock methods                 
+7/-0     
socket_poller.rb
Add RBS type annotations for socket poller methods             
+8/-0     
bridge.rb
Add RBS type annotations for remote bridge methods             
+13/-0   
capabilities.rb
Add RBS type annotations for capabilities methods               
+11/-0   
common.rb
Add RBS type annotations for HTTP common methods                 
+5/-0     
default.rb
Add RBS type annotations for HTTP default methods               
+10/-0   
response.rb
Add RBS type annotations for remote response methods         
+5/-0     
guards.rb
Add RBS type annotations for guards methods                           
+7/-0     
guard.rb
Add RBS type annotations for guard methods                             
+5/-0     
guard_condition.rb
Add RBS type annotations for guard condition methods         
+2/-0     
helpers.rb
Add RBS type annotations for helper methods                           
+3/-0     
rack_server.rb
Add RBS type annotations for rack server methods                 
+5/-0     
test_environment.rb
Add RBS type annotations for test environment methods       
+14/-0   
Tests
1 files
action_builder_spec.rb
Update test exclusion to Chrome for scroll_by test             
+1/-1     
Enhancement
1 files
spec_helper.rb
Integrate RBS trace in RSpec suite lifecycle                         
+7/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Apr 30, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit db643da)

    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

    RBS Trace Configuration

    The RBS trace is enabled for the entire test suite but there's no configuration for filtering which files or methods to trace. This might generate excessive type annotations for third-party libraries or test code.

    trace = RBS::Trace.new
    
    c.before(:suite) do
      GlobalTestEnv.remote_server.start if GlobalTestEnv.driver == :remote && ENV['WD_REMOTE_URL'].nil?
      GlobalTestEnv.print_env
      trace.enable
    end
    
    c.after(:suite) do
      GlobalTestEnv.quit_driver
      trace.disable
      trace.save_comments
    Incomplete Type Signature

    The type signature for execute_script method indicates it returns a String, but the actual implementation can return various types (WebDriver::Element, Integer, Float, Boolean, NilClass, String, or Array) as documented in the method comment.

    # @rbs (String, *nil) -> String
    def execute_script(script, *args)

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 30, 2025

    PR Code Suggestions ✨

    Latest suggestions up to db643da
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect test exclusion

    The test was previously excluded for Firefox but was changed to exclude Chrome.
    This appears to be an unintentional change that could lead to test failures.
    Revert the exclusion back to Firefox to maintain the original test behavior.

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

     it 'scrolls by given amount',
    -   exclude: {driver: :chrome, reason: 'inconsistent behavior between versions'} do
    +   exclude: {driver: :firefox, reason: 'inconsistent behavior between versions'} do
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that the test exclusion was changed from :firefox to :chrome. Given the PR's focus on RBS types, this change might be unintentional and warrants review to ensure the correct browser is excluded.

    Low
    • More

    Previous suggestions

    Suggestions up to commit db643da
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect test exclusion

    The test was previously excluded for Firefox but was changed to exclude Chrome.
    This appears to be an unintentional change that could lead to test failures.
    Revert the exclusion back to Firefox to maintain the original test behavior.

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

     it 'scrolls by given amount',
    -   exclude: {driver: :chrome, reason: 'inconsistent behavior between versions'} do
    +   exclude: {driver: :firefox, reason: 'inconsistent behavior between versions'} do
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that the excluded driver in the test changed from :firefox to :chrome. It reasonably questions if this change was intentional, given the PR's focus on RBS types, and suggests reverting it.

    Low
    Learned
    best practice
    Ensure resource cleanup with begin/ensure

    The RBS trace is enabled in the before(:suite) hook but might not be properly
    disabled if an exception occurs. Wrap the trace operations in a begin/ensure
    block to guarantee cleanup regardless of execution path.

    rb/spec/integration/selenium/webdriver/spec_helper.rb [55-67]

     trace = RBS::Trace.new
     
     c.before(:suite) do
       GlobalTestEnv.remote_server.start if GlobalTestEnv.driver == :remote && ENV['WD_REMOTE_URL'].nil?
       GlobalTestEnv.print_env
    -  trace.enable
    +  begin
    +    trace.enable
    +  rescue => e
    +    puts "Failed to enable RBS trace: #{e.message}"
    +  end
     end
     
     c.after(:suite) do
    -  GlobalTestEnv.quit_driver
    -  trace.disable
    -  trace.save_comments
    +  begin
    +    GlobalTestEnv.quit_driver
    +  ensure
    +    trace.disable
    +    trace.save_comments
    +  end
     end
    Suggestion importance[1-10]: 6
    Low

    @selenium-ci selenium-ci added C-rb Ruby Bindings B-support Issue or PR related to support classes labels Apr 30, 2025
    @selenium-ci
    Copy link
    Member

    Thank you, @aguspe for this code suggestion.

    The support packages contain example code that many users find helpful, but they do not necessarily represent
    the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

    We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
    If you have any questions, please contact us

    @diemol diemol reopened this Apr 30, 2025
    @diemol
    Copy link
    Member

    diemol commented Apr 30, 2025

    I guess the automation needs some tuning.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-support Issue or PR related to support classes C-rb Ruby Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants