-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[java] Selenium Manager generates output from argument list #13385
Conversation
Do I need to deprecate, or can I just remove because the class is annotated as |
and should we also rename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would delete since it is market as beta.
My only concern is that we do not have enough tests to properly verify this refactor. Haven't we had the situation in the past where moving some arguments around ended up in some bugs?
Tests are a good idea. 😂 |
04b22d3
to
6499437
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #13385 +/- ##
=======================================
Coverage 58.82% 58.82%
=======================================
Files 86 86
Lines 5272 5272
Branches 219 219
=======================================
Hits 3101 3101
Misses 1952 1952
Partials 219 219 ☔ View full report in Codecov by Sentry. |
java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java
Outdated
Show resolved
Hide resolved
b5cf270
to
af7d1fc
Compare
Ok, refactored along the lines of Simon's suggestions, added tests, they pass, I'm generally happy with the naming I think right now, and it's matching Ruby & Python PRs. This can be merged once approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
af7d1fc
to
5ad09a4
Compare
…f capabilities instance
CI Failure Feedback(Checks updated until commit 0a296f0)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where Configuration options
See more information about the |
Description
DriverFinder
convert theCapabilities
into the arguments required by Selenium Manager binarygetPath()
methodMotivation and Context
getPath()
is no longer an accurate oneUpdates
Updates:
getPath()
togetResult()
inDriverFinder
as well. The name isn't great, but it is the name of the current class and I can't think of anything better. (getPaths()
?SeleniumManagerOutput()
?)browserVersion
is set tonull
in the 3 applicable driver classes instead of DriverFinder, and only if there is a browser path.Status
getResult()
?