[rb] Fix add_cause method not being able to process an array of hashes

[aguspe]

User description

Description

This PR adds the backtrace from remote method again, since when an error originating from a remote server was processed, since it is not an array of strings it failed, so we need to convert the hashes into strings

Motivation and Context

A proper error message is needed for debugging purposes, so this bugfix helps our users get that.
This PR fixes #14415

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

• Added a new method backtrace_from_remote to process and convert backtrace from remote server errors into a readable format.
• Modified existing error handling to incorporate the new backtrace processing method, ensuring proper error messages are generated.
• Added integration tests to verify the correct handling of backtrace when errors originate from a remote server.
• Updated type signatures to reflect the new method and its usage.

---

Changes walkthrough 📝

	Relevant files
Bug fix	response.rb Enhance error handling for remote server backtrace              rb/lib/selenium/webdriver/remote/response.rb Added method backtrace_from_remote to process backtrace from remote server. Modified add_cause to handle array backtrace from remote server. Improved error handling for remote server errors. +21/-1    response.rbs Update type signature for new backtrace method                      rb/sig/lib/selenium/webdriver/remote/response.rbs Added type signature for backtrace_from_remote method. +2/-0
Tests	error_spec.rb Add tests for remote server backtrace handling                      rb/spec/integration/selenium/webdriver/error_spec.rb Added test for backtrace handling when using a remote server. Ensured WebDriverError is raised with a cause. +7/-0

---

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

[codiumai-pr-agent-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Potential Performance Issue The backtrace_from_remote method uses filter_map, which creates a new array. For large backtraces, this could be inefficient. Consider using map with a conditional return instead. Error Handling The add_cause method rescues a specific error type, but it's not clear what happens if a different error is raised. Consider adding a default error handling case.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Use a guard clause for early return to improve code readability Consider using a guard clause to return early if backtrace is not an Array, improving readability and reducing nesting. rb/lib/selenium/webdriver/remote/response.rb [58] -backtrace = backtrace_from_remote(backtrace) if backtrace.is_a?(Array) +return backtrace unless backtrace.is_a?(Array) +backtrace = backtrace_from_remote(backtrace) Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to use a guard clause enhances readability by reducing nesting, making the code easier to follow and maintain.	7
Best practice	Use safe navigation operator to simplify nil and empty checks Consider using &. (safe navigation operator) instead of checking for nil? and empty? separately for the meth variable. rb/lib/selenium/webdriver/remote/response.rb [76] -meth = 'unknown' if meth.nil? || meth.empty? +meth = meth&.empty? ? 'unknown' : meth Apply this suggestion Suggestion importance[1-10]: 6 Why: The use of the safe navigation operator simplifies the code by reducing the need for separate nil and empty checks, improving readability.	6
	Use more specific expectation matcher for error type checking Consider using a more specific expectation for the cause's class, such as be_a_kind_of instead of be_a, to ensure the error is of the exact expected type or its subclass. rb/spec/integration/selenium/webdriver/error_spec.rb [55] -expect(e.cause).to be_a(WebDriver::Error::WebDriverError) +expect(e.cause).to be_a_kind_of(WebDriver::Error::WebDriverError) Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion to use be_a_kind_of provides a more precise check for the error type, which can help ensure the test's accuracy.	6
	Use block syntax with curly braces for single-line blocks to improve readability Consider using then instead of do for the single-line block in filter_map to improve readability. rb/lib/selenium/webdriver/remote/response.rb [66-67] -server_trace.filter_map do |frame| +server_trace.filter_map { |frame| next unless frame.is_a?(Hash) Apply this suggestion Suggestion importance[1-10]: 5 Why: Using curly braces for single-line blocks is a common Ruby convention that enhances readability, though the improvement is minor.	5

[aguspe]

Ready for review @p0deje @ioquatix

[aguspe]

@p0deje I updated to exclude firefox after the pipelines failures, because instead of throwing a session error, firefox throws the following error:

Selenium::WebDriver::Error::InvalidArgumentError:
binary is not a Firefox executable

and unless I set the timeout of the remote server to 1 second, I tried with other options to cause the session error and they throw different types of errors, so I think excluding firefox is the best option on this test

Should this be a bug btw? because I was expecting the same error across all the browsers

[aguspe]

So the issue on local Mac and local Firefox is that it doesn't fail and the guard throws an error, I will look into that

However, in Edge there is this bug, which I can look into but it seems to be outside of the scope of this PR:

Failures:

  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: 0dd8f51... os.arch: 'amd64', os.version: '10.0', java.version: '17.0.11'
       Driver info: driver.version: unknown> with backtrace:
         # ./rb/lib/selenium/webdriver/remote/response.rb:63:in `add_cause'
         # ./rb/lib/selenium/webdriver/remote/response.rb:41:in `error'
         # ./rb/lib/selenium/webdriver/remote/response.rb:52:in `assert_ok'
         # ./rb/lib/selenium/webdriver/remote/response.rb:34:in `initialize'
         # ./rb/lib/selenium/webdriver/remote/http/common.rb:101:in `new'
         # ./rb/lib/selenium/webdriver/remote/http/common.rb:101:in `create_response'
         # ./rb/lib/selenium/webdriver/remote/http/default.rb:103:in `request'
         # ./rb/lib/selenium/webdriver/remote/http/common.rb:67:in `call'
         # ./rb/lib/selenium/webdriver/remote/bridge.rb:685:in `execute'
         # ./rb/lib/selenium/webdriver/remote/features.rb:62:in `downloadable_files'
         # ./rb/lib/selenium/webdriver/common/driver_extensions/has_file_downloads.rb:27:in `downloadable_files'
         # ./rb/spec/integration/selenium/webdriver/remote/driver_spec.rb:87:in `block (3 levels) in <module:Remote>'
         # ./rb/spec/integration/selenium/webdriver/remote/driver_spec.rb:86:in `block (2 levels) in <module:Remote>'
     # ./rb/spec/integration/selenium/webdriver/remote/driver_spec.rb:86:in `block (2 levels) in <module:Remote>'
     ```

[p0deje]

However, in Edge there is this bug, which I can look into but it seems to be outside of the scope of this PR:

Looks like it's failing in the trunk as well, so maybe you could take a look in a separate PR.

[aguspe]

However, in Edge there is this bug, which I can look into but it seems to be outside of the scope of this PR:

Looks like it's failing in the trunk as well, so maybe you could take a look in a separate PR.

Great, I will do

[p0deje]

There are still some failures in RBE tests. I wonder if you could find a different way to test this w/o relying on binary. Maybe try sending a command that would cause a failure (e.g. find non-existent element)?

[aguspe]

There are still some failures in RBE tests. I wonder if you could find a different way to test this w/o relying on binary. Maybe try sending a command that would cause a failure (e.g. find non-existent element)?

That's a good point, I updated the error type now @p0deje

[aguspe]

Ready for re-review @p0deje

[p0deje]

@aguspe Can you please double-check that the spec actually catches the regression you fixed? It's possible that the backtrace is not coming from the remote in this particular case, so we might need to find a different scenario. I know I suggested it in the first place, but I hoped you'd check that by yourself, though I haven't asked for it explicitly.

[aguspe]

@aguspe Can you please double-check that the spec actually catches the regression you fixed? It's possible that the backtrace is not coming from the remote in this particular case, so we might need to find a different scenario. I know I suggested it in the first place, but I hoped you'd check that by yourself, though I haven't asked for it explicitly.

So if we want to have a session-related scenario, we could close the driver earlier, here are 2 screenshots of an example before and after the fix on the same spec locally, I will push my changes now so we do not use the find element and we focus on the session which I think is a better spec

Could you re-run the pipeline @p0deje ?

[aguspe]

@p0deje the failing test in firefox seems not to be related to my PR, but I will make another PR fixing the issue and also the Edge one so the pipelines work again, unless you prefer me to fix it on this one

[aguspe]

@p0deje I just updated the test to cause the session error without quitting the driver, this should avoid quitting driver errors

[aguspe]

Updated to use title now, I tested locally still produces the session error

[p0deje]

@aguspe There are still some failures in RBE tests

[aguspe]

@p0deje Tested locally on chrome, switched to window handle, works as expected, hopefully now the only issue left is the existent edge one

[aguspe]

@p0deje now the test is not using any script (I cannot reproduce the error locally), but I'm modifying the session id to cause an error

[aguspe]

Now the only issue left is the edge bug and with this merge, it's is going to be easier to debug @p0deje, so when this gets merged I will jumpt to work on the edge fix and fix any review comments from #14287