Skip to content

Update JavaScript bindings for Edge Chromium #7977

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

Closed
wants to merge 11 commits into from

Conversation

bwalderman
Copy link
Contributor

@bwalderman bwalderman commented Jan 28, 2020

Description

This change updates the 'edge module in the JavaScript library with support for Edge Chromium. The edge.Options type has been updated to support the various Chromium capabilities. This uses a similar strategy to the Java and C# library where a 'chromium' base module is factored out, and 'chrome' and 'edge' derive from this module.

The existing Edge (EdgeHTML) is still used by default to maintain backward compatibility. Users can opt in to Chromium by calling the new edge.Options.useEdgeChromium(true) method. Docs have been updated with these changes and up-to-date download links.

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.

@bwalderman bwalderman requested a review from corevo January 28, 2020 21:53
@corevo
Copy link
Member

corevo commented Feb 11, 2020

Looks good, I'm only concerned with the breaking change regarding the default change from EdgeHTML to ChromiumEdge.

I've noticed a similar PR is open in the .net bindings, what are your thoughts, technically the bindings are in alpha (not for this reason...), but I'd like to keep the status quo, will the Java & .net break and change the default to ChromiumEdge?

\cc @shs96c @jimevans

@bwalderman
Copy link
Contributor Author

@corevo, regarding the breaking changes, we would like to move all bindings to the new Edge by default. Our team's position is that Edge Chromium is just Edge so testing it should continue to be as straightforward as testing Edge currently is. Of course, the reality is that "legacy" Edge still exists and will take some time to phase out so I made sure we have the environment variable and legacy Edge service as a fallback. Keeping existing legacy tests running should be as simple as flipping the environment variable on. I hope that's enough to mitigate the breaking nature of this change.

@AutomatedTester
Copy link
Member

AutomatedTester commented Feb 12, 2020

@bwalderman

What is the current status of roll out of Edge? If it is not at a large enough number* then we definitely can't switch over until it is a large enough number*.

Using an environment variable for a breaking change won't work when deployed to a grid and they will be using remote webdriver there.

My suggestion is to have this new Edge Chromium as not the default for a couple releases but allow people to turn it on via capabilities. Have each of the bindings, and this will allow remote webdriver over grid to work too, understand edge-chromium:true capability.

We can then document the change, encourage people to turn it on and see what breaks. Since there is a change to the browser engines we can not guarantee that they will work exactly the same. They can update their tests, leave the capability on, and then we move to having edge-chromium: true being the default in 2 releases but have the fall back to EdgeHTML with edge-chromium: false in the bindings

*Happy to debate the number, then perhaps we need to have a mechanism

@bwalderman
Copy link
Contributor Author

@AutomatedTester, thanks for the feedback. I've backed out the portion of this change that switches to Chromium by default so this should no longer be a breaking change. After discussing with the team, our first priority is to get Chromium support checked in for all languages.

I agree having a capability to control this would be useful for the remote scenario. I'd prefer to do that in a separate change though.

@corevo
Copy link
Member

corevo commented Feb 13, 2020

I'm glad we've found common ground, I like David's idea of using a capability rather than an environment variable, this way it can be consistent between edge chromium throughout the bindings, and consistent how other vendors do it.

For example safaridriver's useSimulator capability.

@bwalderman
Copy link
Contributor Author

@corevo, @AutomatedTester I may have misunderstood where this new capability should go. I assumed this capability would be handled by a remote WebDriver server (e.g. Grid) and the server would use the capability to determine which type of Edge service to create.

Something like Safari's useSimulator capability wouldn't work for Edge. Chromium and EdgeHTML based-Edge use different WebDriver executables (MsEdgeDriver and MicrosoftWebDriver respectively) so it is not possible to handle the engine switch at this layer. The decision needs to be made before the WebDriver executable is launched.

I considered handling the switch in edge.Driver.createSession(). It could check the capabilities and create the appropriate service. The problem is we would need to add similar switching logic to every language binding. Is there a potentially better way to do this?

@AutomatedTester
Copy link
Member

We are going to have to do the duplication in each binding to pick the right service based on the capabilities but the one side affect of doing that is the flipping can all be done in 1 commit to change the defaults.

@bwalderman
Copy link
Contributor Author

@AutomatedTester @corevo , based on your feedback, I've updated the Edge JS bindings to use the 'browserName' capability to switch engines. The existing value 'MicrosoftEdge' will launch a legacy driver, and the new value 'msedge' will launch the Chromium driver. The EdgeOptions class has a useEdgeChromium() method to set the correct browserName. This is based on how Safari Tech Preview works in the Java binding. Looks like they have a similar setTechnicalPreview() method that manipulates the browserName value and causes the driver to choose either the tech preview or release driver binary.

Please review. I'll also update my other PR to use the same pattern.

@corevo
Copy link
Member

corevo commented Feb 24, 2020

@bwalderman sounds good

@AutomatedTester do you want to ping all the bindings?

@corevo
Copy link
Member

corevo commented Feb 24, 2020

@bwalderman users will have to use the EdgeService to instantiate a Chromium Edge driver, do you want to add a case to handle msedge, it'll allow users to also instantiate based on capabilities alone.

E.g. .usingCapabilities({ browserName: 'msedge' })

@bwalderman
Copy link
Contributor Author

@bwalderman users will have to use the EdgeService to instantiate a Chromium Edge driver, do you want to add a case to handle msedge, it'll allow users to also instantiate based on capabilities alone.

E.g. .usingCapabilities({ browserName: 'msedge' })

@corevo I'm concerned about adding a second name for Edge throughout Selenium. There was some past discussion on this in #7142 (review). The new 'msedge' name is meant to be an implementation detail for the Edge module only. Users shouldn't need to think about it and should instead use the new UseChromiumEdge method which will do the right thing for them.

Instantiating Chromium Edge from capabilities alone should already be possible with this approach. The user would pass either a raw Capabilities object or an edge.Options containing browserName='msedge', and then edge.Driver.createSession would create the correct EdgeService in createServiceFromCapabilities.

One caveat is if a user configures an EdgeService using .setEdgeService(). In this case, the service will be Legacy-only or Chromium-only depending on which driver executable the user provides. The user will need to be careful to send only matching requests to the EdgeService. We don't consider running Legacy and Chromium Edge side-by-side on the same machine to be a mainline scenario. Installing Edge Chromium Stable on a machine will actually hide Edge Legacy and override all entry points. Therefore, having the EdgeService only able to handle one version of Edge at a time seems acceptable.

@bwalderman
Copy link
Contributor Author

Thought about this some more. I realized it still would have been necessary to add a new case for "msedge" to the Builder API, otherwise the Builder wouldn't know to create an EdgeDriver at all.

Based on this, and the earlier discussion I linked above, I've decided to use a new capability instead of browserName. The new capability is named "ms:edgeChromium". So if a user wants to instantiate an EdgeDriver using pure capabilities, they could do so with:

.withCapabilities({
    'browserName': 'MicrosoftEdge',
    'ms:edgeChromium': true
});

I've also added this configuration to testing/index.js in the latest commit.

@bwalderman bwalderman requested a review from corevo February 28, 2020 20:38
Copy link
Member

@corevo corevo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a small change to make sure that the driver is located on the system before attempting to build it

@corevo
Copy link
Member

corevo commented Mar 4, 2020

Merged as dbeafd2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-nodejs JavaScript Bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants