[🐛 Bug]: Grid Protocol Converter Insufficient

[titusfortner]

What happened?

Java Bindings and the Grid are maintaining a lot of extra code in order to be backwards compatible with earlier versions of Selenium. Except we don't have any tests to show we've been successful with this. I ran the old Watir-WebDriver tests using the final version of Se2 for the Ruby bindings against the latest Grid, and 40% of them failed.

Updated Update

Found several more issues in Java bindings for OSS code; there are 3 categories:

1. Stuff that was broken in Grid 4 with the Locator Conversions (name, id & class name locators)
2. Stuff that was broken in Server 3.7 when translating JWP/early W3C to W3C (switchToWindow & sendKeysToActiveElement) — broken running Java bindings 2.53.1
3. Stuff that was broken in Server 3.7 when translating pre-W3C JWP to W3C (locators except id & name, implicitlyWait, setScriptTimeout and switchToFrame) — broken running Java bindings 2.45.0 or other bindings 2.53.1

This is now looking like a lot more work to fix; and less likely that people are making as much use of the protocol translations as we've thought.

Update

• The two things definitely affecting the Java bindings actually look like they might be simple to fix
• I've never seen a framework that didn't have a way to check if an element wasn't there, and didn't use id/name/class name to do so, which indicates to me that no one has tried to run their 2.x code on a 4.x server, otherwise they'd have raised an issue.
• No one using Selenium 2 could use Selenium 3 server and switch windows since v3.6 (August 2017), which small sample size, but I've never worked anywhere that I didn't need to switch windows for my test suites.
• I was surprised that Selenium Server 2.53 was able to run latest Chrome version so well. If all you care about is Chrome and/or anything working, there's zero reason to upgrade off of Server 2.53 right now, (which would explain why there are still so many downloads of it).
• Whatever we end up doing, I don't think it is unreasonable that running tests on the latest versions of (non-Chrome) browsers should require updating to a version of Selenium > 3.9 when w3c was pretty much finalized, in February 2018.

Uploaded a repo with 13 failing JWP tests with Grid 4 (and 1 failing JWP test with Server v3 for completeness)
https://github.com/titusfortner/grid4conversion/tree/main/src/test/java/com/titusfortner/grid4conversion

Options

1. File these as bugs and fix/ignore as time allows until we do a non-backward compatible rewrite in Selenium 5
2. Fix these bugs ASAP and then separate out all the protocol conversion code and provide it the same way we do with leg-rc package
3. Consider it a sunk cost, and just delete all of it now so we can simplify the code and make forward progress

Someone suggested to me that we should proceed with option 3 and if any user really wants option 2, they can pay us/someone to do it, and that makes sense to me with limited resources.

Supporting Data

Here are the different combinations I tested with old watir-webdriver specs using Selenium 2.53.4 Ruby bindings with the latest versions of the browsers and the latest versions Selenium (toggling OSS mode on/off in Se 3/4 for Chrome)

Browser	Local	Se 2	Se 3	Se 3 OSS	Se 4	Se 4 OSS	Total Tests
Chrome	3	1	356	3	489	117	1235
Firefox	N/A	N/A	364	N/A	516	N/A	1244
Safari	N/A	N/A	302	N/A	404	N/A	1120

Things that are broken when running Bindings 2.53.x

• Return JavaScriptEnabled as part of capabilities (Ruby only — won't even try to execute JS if not in caps)
• Ensure Window "name" parameter is updated to "handle" (Java & Ruby)
• Delete null values from Cookie arguments (Ruby only — Java deletes null params before sending)
• Translate click actions with bodies into correct actions (e.g., {"button": 2}) (This was broken for Selenium Server 3.x but fixed in Grid 4)
• Translate moveTo commands with x/y offsets (Ruby only — Ruby sends null element for just moveTo, Java omits it)
• Using id or name to locate a missing element will throw the wrong error (Java & Ruby)
• Using class name to locate an element gives unable to determine locator strategy (Java & Ruby)
• Sending Key to Active Element gives UnsupportedCommandException (Java & Ruby)

Things that are broken when running Java Bindings 2.45 (or later for other bindings):

• Using css selector, xpath, tag name, link text, or partial link text values can't be converted to Web Element
• Setting script timeout –> Cannot call non W3C standard command while in W3C mode
• Setting implicit wait timeout —> Cannot call non W3C standard command while in W3C mode
• Switching to Frame by Name —> invalid argument: 'id' can not be string

(Watir doesn't do a whole lot with actions, so it's likely possible a bunch of more things are broken in the translation)

@asolntsev is going to try to help us out by running the same experiment with old Selenide test suite for Java. That might tell us more about how bad the situation is. It's possible that other bindings will have fewer issues.

My guess though is that our assumption that Selenium 2 Bindings users can use Selenium 3 Server or Selenium 4 Grid is misguided, and that people still using Selenium 2 are using Selenium 2 server, and we would be better off removing the translation code rather than fixing it.

How can we reproduce the issue?

Ruby can be reproduced by running watir-webdriver 0.9.3 watirspecs with selenium-webdriver 2.53.4 setting REMOTE, and updating the server start code.
Java with JUnit 5:

    @Test
    void getWindow() {
        Set<String> windowHandles = driver.getWindowHandles();
        String handle = windowHandles.stream().findFirst().get();

        Assertions.assertDoesNotThrow(() -> driver.switchTo().window(handle));
    }

    @Test
    void useIDForMissingElement() {
        NoSuchElementException thrown = Assertions.assertThrows(NoSuchElementException.class, () -> {
            driver.findElement(By.id("not-there"));
        });

        String msg = "no such element: Unable to locate element: {\"method\":\"id\",\"selector\":\"not-there\"}";
        Assertions.assertTrue(thrown.getMessage().contains(msg));
    }

Relevant log output

Caused by: org.openqa.selenium.WebDriverException: Returned value cannot be converted to WebElement: {stacktrace=java.lang.RuntimeException: Unable to execute request for an existing session: An unknown error has occurred

and

org.opentest4j.AssertionFailedError: Unexpected exception thrown: org.openqa.selenium.WebDriverException: invalid argument: 'handle' must be a string

Operating System

MacOS Big Sur

Selenium version

Ruby 2.53.4 & Java 2.45.0 & Java 2.53.1

What are the browser(s) and version(s) where you see this issue?

Chrome, Firefox, Safari

What are the browser driver(s) and version(s) where you see this issue?

Latest

Are you using Selenium Grid?

4.1.2

[asolntsev]

I created a separate project on github for testing grid compatibility:
https://github.com/asolntsev/grid-compatibility-test

You can easily clone it and run commands described in readme.

At the moment this VERY TRIVIAL test shows that all versions of Java bindings (2.x, 3.x, 4.x) do work with all versions of grid (2.x, 3.x, 4.x).

[titusfortner]

I updated the ticket with which of the Ruby errors I've verified are also broken in Java. @asolntsev verified interoperability in Java between the versions based on a single test, but I'm hoping we can get the full Selenide test suite from 2.53 to run on Grid 4 to see if anything else is broken.

[asolntsev]

@titusfortner Now I added the checks you suggested above getWindow and useIDForMissingElement, and they do fail

1. getWindow fails with WebDriverException: invalid argument: 'handle' must be a string
2. useIDForMissingElement fails with expected: <org.openqa.selenium.NoSuchElementException> but was: <org.openqa.selenium.WebDriverException>

[titusfortner]

Ok, just overhauled the original post with all the data. I think option 1 or 3 are reasonable, but 2 is way more effort than we have resources available right now, and 1 is going to continue to make adding new things challenging.

[asolntsev]

@titusfortner I vote for option 3. I am absolutely sure that the backward compatibility of Selenium Grid is not needed at all. When people want to upgrade Selenium, they do upgrade both tests and grid. Period. I don't see absolutely any reasons why someone should need to upgrade only one of them.

[titusfortner]

The primary (and legitimate) use case is going to be Selenium 2 users who want to run tests on the latest Firefox or Safari browsers, which are w3c only. Theoretically they don't have to upgrade their code, they can just update the grid (to 3.9+) and have everything still work.

In practice I think these users don't care about cross browser testing and are content with having *anything work, and that's either a hard coded older version of Firefox, or the latest Chrome.

[titusfortner]

And I made an updated update.

Fun story. I was noting some weird differences between Ruby 2.53 and Java 2.53 with Grid 4.

Turns out that before we created the handshake approach in 3.0, the bindings were slowly converting JSON Wire Protocol things with W3C implementations. Selenium 2.45.0 was completely JSON Wire Protocol, and starting in 2.46.0 some things were getting changed. The changes were fine because Chrome was the only driver that Selenium wasn't maintaining, and Chrome was quick to update things at that time.

Doesn't look like JS did any of this.
Java & .NET just went for it.
Ruby & Python mostly added conditionals for it, which turned into the Handshake implementation. (Element location was the main exception where both OSS & W3C formats were returned before 2.53).

The protocol conversion code that was added in 3.7 didn't consider the actual original JSON Wire Protocol format, though, just the things that changed after the handshake code.

[titusfortner]

Also interesting to look at selenium-java numbers from the past month:

W3C users:
Selenium 4+ — 10%
Selenium 4 pre-release — 7% (I don't know why, either)
Selenium 3.9 - 3.141.59 — 75% (I suspect this is unlikely to change any time soon)

W3C/JWP Handshake
Selenium 3.0 - 3.8.1 — 6% (post-handshake but pre-w3c-finalized); honestly not sure what value protocol translation provides these people

JWP users:
Selenium 2.46.0 - 2.53.1 — 5% (mostly JWP, some W3C as discussed above); protocol translation in 4.x is bad, but 3.x is decent (but I still want to know why no one has complained about window switching if 2.x users are using Grid 3.x)
Selenium 2 - 2.45.0 — 2% (no w3c here); protocol translation isn't helping these at all

[diemol]

This is a case where someone was creating a JWP session but sending W3C commands. Ideally, these cases should be rare but get very confusing due to the nature of it. Based on the comments above, I believe option 3 is the way to go. We need to think how to implement it without breaking things.

[titusfortner]

To be fair, I'm not sure we should be supporting curl requests directly? If they'd sent the request to the Grid for window/current/size it would have converted it to what the driver needed to see window/rect and then returned the size values (I think). That said, Unable to execute request is a "bad" error message, and it would feel so much nicer if we could just not give a session in the first place for desired capabilities.