Closed aguspe closed 4 days ago
โฑ๏ธ Estimated effort to review [1-5] | 3 |
๐งช Relevant tests | Yes |
๐ Security concerns | No |
โก Key issues to review |
Possible Bug: The method url in WebDriverError class manipulates class names to generate URLs. This assumes a specific naming convention and structure for error class names which might not always hold true or could lead to incorrect URLs if the class names are not perfectly aligned with the expected format. |
Refactoring Impact: The removal of hardcoded URLs and reliance on dynamically generated URLs could impact existing documentation or external references that expect static URLs. This needs careful review to ensure it doesn't break existing user flows or documentation links. |
Category | Suggestion | Score |
Test coverage |
Add a test case to verify URL generation for
___
**Add a test case to verify the URL generation for another error type, such as | 8 |
Enhancement |
Simplify the
___
**The | 7 |
Performance |
Memoize the
___
**Consider memoizing the | 7 |
Best practice |
Freeze the string constants to ensure they remain immutable___ **Usefreeze on the SUPPORT_MSG and ERROR_URL constants to prevent them from being modified, ensuring they remain immutable.** [rb/lib/selenium/webdriver/common/error.rb [37-38]](https://github.com/SeleniumHQ/selenium/pull/14174/files#diff-942df34e1c3e79a3a6ada80fc46e4ef30a799c7a133efdff252f8b3cce4d7ea1R37-R38) ```diff -SUPPORT_MSG = 'For documentation on this error, please visit:' -ERROR_URL = 'https://www.selenium.dev/documentation/webdriver/troubleshooting/errors' +SUPPORT_MSG = 'For documentation on this error, please visit:'.freeze +ERROR_URL = 'https://www.selenium.dev/documentation/webdriver/troubleshooting/errors'.freeze ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Freezing constants is a good practice in Ruby to prevent accidental modifications. The suggestion is correct and applies to the constants defined in the PR. | 6 |
I see that you dynamically generate URLs from a class name of the error. I have a feeling it might be fragile and prone to subtle bugs if somebody makes a typo in a URL while working on #1276. It also make it really hard to refactor the website and make sure URLs are used correctly in all bindings. The problem is that this can't be grepped anymore.
I'd suggest we instead have a hash of error => URL mapping that will explicitly allow us to mark which errors already have URLs and add them as we go.
URLS = { NoSuchElementError => '#no-such-element-exception', ... }
Looking forward to what @titusfortner thinks on that?
That sounds like a really nice idea, and it will definitely be better than my approach of just having urls that redirect to the error page
If @titusfortner agrees with this, I can update this PR tomorrow since there are only a couple of urls and then keep making small PRs for the rest of them
@p0deje I changed my implementation to match your suggestion and I fixed the rubocop issues, also I simplified the tests so I hope now this PR is on the right track :)
Thank you for the contribution!
User description
Description
This PR adds a URLs constant on the error module to be able to obtain the URL based on a symbol for each WebDriver error subclass
Motivation and Context
Based on #11512 the goal is to provide more information in the error message by linking to the selenium documentation
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
url
method to theWebDriverError
class to generate URLs dynamically based on the class name.NoSuchElementError
,StaleElementReferenceError
,InvalidSelectorError
, andNoSuchDriverError
.WebDriverError
class.Changes walkthrough ๐
error.rb
Add dynamic URL generation for error messages.
rb/lib/selenium/webdriver/common/error.rb
url
method toWebDriverError
class to generate URLs based onclass names.
error.rbs
Update type signatures for new methods in `WebDriverError`.
rb/sig/lib/selenium/webdriver/common/error.rbs
WebDriverError
.error_spec.rb
Add tests for dynamic URL generation in error messages.
rb/spec/integration/selenium/webdriver/error_spec.rb
NoSuchElementError
to verify URL generation.