-
Notifications
You must be signed in to change notification settings - Fork 76
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
Clean up capabilities of non-W3C capability names #253
base: master
Are you sure you want to change the base?
Conversation
"selenoid:options": { | ||
enableVNC: true, | ||
sessionTimeout: "60m", | ||
labels: { manual: "true" }, |
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.
this is breaking change actually
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 was following another documentation and also the Selenium-Webdriver
implementation.
An even the mozilla mention the list of as:
- acceptInsecureCerts
- browserName
- browserVersion
- platformName
- pageLoadStrategy
- proxy
- setWindowRect
- timeouts
- unhandledPromptBehavior
Therefore limiting the capabilities
just to the allowed properties should not brake anything. I test it locally with out issues. Also while adding firstMatch
or alwaysMatch
is accepted it does create an issue for Microsoft Edge
. The selenoid:options
are still passed, but in the desiredCapabilities
without having the use the reserve name selenoid:options
.
https://developer.mozilla.org/en-US/docs/Web/WebDriver/Capabilities can you point to the document which stands that removed caps are not compliant? More than that it explicitly saying
|
I was following another documentation and also the An even the mozilla mention the list of as:
Therefore limiting the |
I think you misread something in the spec. Spec says exactly the same - and https://www.w3.org/TR/webdriver/#dfn-capabilities-processing Also, there are extensions to the standard caps which are defined in the form of Most probably you're facing the the Edge/IE issue not implementing this spec properly and responding to the So there are 2 ways of fixing this: having a dedicated request for edge/ie, or delegate that job to selenoid. |
So following w3c logic only
are non-w3c-compliant and should not be in the always/first match nodes on the top level (and they are not there) |
I was trying to avoid a conditional logic just to satisfy Edge issues with |
No, we are not going to maintain vendor-specific hacks. There is nothing in the spec saying that vendor-specific capabilities for |
okay, so I would try to fix that in selenoid first and then let's see what happens |
fix for aerokube/selenoid#771 was released, it could help, can you verify that? |
I was not aware of that issue. How can I help? |
basically all we discussed here was fixed in the latest selenoid release |
I was able to test, and I can not start IE or Edge manual sessions. Here are the errors I am getting:
This is my browsers file:
One more thing. I can start a manual session using CURL with the following body:
|
@emilorol thx for the extensive answer, we will try to investigate more on the selenoid side |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #253 +/- ##
=======================================
Coverage 35.15% 35.15%
=======================================
Files 36 36
Lines 640 640
Branches 98 98
=======================================
Hits 225 225
Misses 362 362
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This change will allow the manual start for all major browsers including IE 11 and Edge 18.
Ideally you will keep the
alwaysMatch
, but then Edge will shock on it.Note: I also update the
CURL
sample code to display the change