Add methods to options classes for w3c compliant capabilities

[titusfortner]

Description

Adds methods and tests for:

• setBrowserVersion()
• setPlatformName()
• setTimeouts()
  edit:
• setImplicitWaitTimeout()
• setPageLoadTimeout()
• setScriptTimeout()

Timeouts has user-facing Duration that gets converted to millis when adding it to capabilities.

Motivation and Context

It makes sense to have specific methods for known capabilities, and this is backwards compatible.

[shs96c]

Prefer a setImplicitTimeout, setPageLoadTimeout and setScriptTimeout, and then we don't need to do any validation, and it's really hard to get wrong.

[titusfortner]

Much better implementation.

[shs96c]

I think this has the potential to get confusing --- somewhere in a framework the implicit timeout is set, and then user code sets the page load timeout: what do we expect to happen to the implicit timeout? I think people will expect both to be set.

[titusfortner]

splitting up the timeouts as suggested above fixes this issue nicely.

[shs96c]

These are really meant to be the keys in the capabilities, not the keys of values within the capabilities

[shs96c]

Perhaps:

ChromeOptions chromeOptions = new ChromeOptions();
chromeOptions.setImplicitWaitTimeout(Duration.ofSeconds(1));

assertThat(chromeOptions.getCapability(TIMEOUTS)).isEqualTo(Map.of("implicit", 1000L);

chromeOptions.setPageLoadTimeout(Duration.ofSeconds(2));
assertThat(chromeOptions.getCapability(TIMEOUTS)).isEqualTo(Map.of("implicit", 1000L, "page", 2000L);

[shs96c]

There's no need to convert the options to an intermediate map.

[titusfortner]

agreed, doesn't make sense if needing to look at existing capabilities

[shs96c]

I'd be tempted to avoid this variable. Consider a user doing:

options.setCapability(TIMEOUTS, Map.of("implicit", 1000));
options.setPageLoadTimeout(Duration.ofSeconds(10));

I think a user would expect both timeouts to have been set, but the current implementation would only set one. I also think the opposite case (where the timeouts capability is set after calling the specific method) would mean that the user would expect the timeout map they passed in to be used.

One drawback to doing this is that you can't expect the Map the user passes in to be mutable. You'll need a check like:

Map<String, Long> newTimeouts = new HashMap<>();
Object raw = getCapability(TIMEOUTS);
if (!(raw instanceof Map)) {
  ((Map<?, ?>) raw).foreach((key, value) -> {
    if (key instanceof String && value instanceof Long) {
      newTimeouts.put((String) key, (Long) value);
    }
  }
  newTimeouts.put("implicit", timeout.toMillis());
  setCapability(TIMEOUTS, Collections.immutableMap(newTimeouts));
))

Finally, it's good practice to make the collection "unmodifiable" when we set it, to prevent users from changing state in unexpected ways.

[titusfortner]

Ok, I put this in a private getCapabilities() method. I'm not sure the reason for instanceof Map conditional. What alternative object is valid that will start a session?

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
0.0% Duplication

[shs96c]

You need to check that raw is actually a Map before casting to it. If not, if the user has set some garbage value (eg: setCapability(TIMEOUTS, "cheese") chaos will follow.