[rb] Add backtrace locations and cause to errors #14170
Conversation
PR Reviewer Guide 🔍
|
PR Code Suggestions ✨
|
aguspe
changed the title
There was a problem hiding this comment.
I am not sure I follow the implementation and how it works. What we discussed in #13222 was to stop manipulating backtrace, create a nextra error instance and explicitly pass cause for errors created from the remote server in
selenium/rb/lib/selenium/webdriver/remote/response.rb
Lines 41 to 43 in 5ce5555
Does this PR achieve the same in some other manner?
Oh Maybe I miss understood this, because I tried to call both backtrace_locations and case on the ex, and I noticed I couldn't so I implemented the methods that are being inherited from exception, so now when creating an error both the case and backtrace_locations are set automatically So for example this test: it 'has cause' do
driver.find_element(id: 'nonexistent')
rescue WebDriver::Error::NoSuchElementError => e
expect(e.cause).to be_a(WebDriver::Error::NoSuchElementError)
endreturns cause as: And this test : it 'has backtrace locations' do
driver.find_element(id: 'nonexistent')
rescue WebDriver::Error::NoSuchElementError => e
expect(e.backtrace_locations).not_to be_empty
endAlso these are the methods from the exception class that I'm overriding, that returns nil even if the backtrace is not set: # This method is not affected by Exception#set_backtrace().
def backtrace_locations; end
# Returns the previous exception ($!) at the time this exception was raised.
# This is useful for wrapping exceptions and retaining the original exception
# information.
def cause; end |
|
@p0deje I see what you are saying and I updated the implementation by removing the backtrace manipulation, and the test kept working, now I will update the cause, thank you! it's my first time working with exceptions so sorry for the confusion |
…nto fix_backtrace_location
|
This looks like it's going in the right direction! Nice work. |
Thank you so much, hopefully based on my last comment regarding the backtrace and cause we know where to go from here :) and also thank you @p0deje for all the help |
|
@p0deje I looked into the failures, and they seems to be related to the action builder and Firefox on windows, I can try to use parallels on my mac to look into them, but I'm not sure if those tests should be fixed on this PR |
Don't worry about these, they are not related. |
|
You might want to split it if it's not an array of strings - you might want to test it. You can also see the output by using |
| server_trace.split("\n") | ||
| end | ||
|
|
||
| ex.set_backtrace(backtrace + ex.backtrace) |
There was a problem hiding this comment.
This is probably the only line that should have been changed.
There was a problem hiding this comment.
I noticed during testing that when setting the backtrace to the exception a couple of things happened:
-
The cause was not set which was the issue
-
Depending on the type of ex.backtrace this ended up in a type error
-
As the example in the previous conversation and the testing done with the selenium grid, it did not seem to have the effect we expected in the formatting of the backtrace, I added examples and my reasoning here: [rb] Add backtrace locations and cause to errors #14170 (comment)
Let me know if this doesn't make much sense to you and I will gladly make a PR to update this :) thank you so much for the help!
Co-authored-by: aguspe <[email protected]>
User description
Description
The goal of this PR is to provide backtrace_locations and cause for users that need to consume it
Motivation and Context
A great detail explanation of the motivation is described here #13221 and also more information is provided in #13222
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
WebDriverErrorclass by addingbacktrace_locationsmethod to fetch current thread's backtrace locations.WebDriverErrorclass by addingcausemethod to fetch the error information.Englishmodule for improved error handling.NoSuchElementError.Changes walkthrough 📝
error.rb
Add backtrace locations and cause methods to WebDriverError classrb/lib/selenium/webdriver/common/error.rb
backtrace_locationsmethod toWebDriverErrorclass to fetchcurrent thread's backtrace locations.
causemethod toWebDriverErrorclass to fetch the errorinformation.
Englishmodule for error handling.error_spec.rb
Add tests for backtrace locations and cause in NoSuchElementErrorrb/spec/integration/selenium/webdriver/error_spec.rb
NoSuchElementError.NoSuchElementError.