[java] implement Sauce Configs for Chrome #243

titusfortner · 2020-11-30T01:38:08Z

Ok, I was definitely making this too hard... We were trying to decide between option 4 & option 6 from this list, and this PR I think takes the best from each of them and does it in a rather light weight solution.

The point of these changes is to provide a way for a user to only have access to the methods/parameters that make sense for the type of test being run. (e.g. Chrome tests can't set IE Driver Version, etc)
This isn't as much of an issue with just browser implementations, but will be essential for dealing with Mobile & Visual, etc.

So, SauceOptions is going to store the state of *everything, but access to the everything is going to be controlled by the config classes.

Config classes will only have setters, no getters and is only going to store state within a SauceOptions instance (which means we can't use lombok setters since we aren't setting state on fields directly). We're also using generics to return an instance of the subclass rather than having it return the superclass. Then the user calls build() to generate the SauceOptions instance, so they don't ever need to know which class of configs they are using under the hood.

I've marked the configs class constructor as "Deprecated" to encourage users to use it from the static method in SauceOptions, but if someone *does use the config class directly, or doesn't call build() on it, the code still lets them pass it into a SauceSession constructor and the constructor will just call the build method on it.

The capabilities are defined based on: https://docs.google.com/spreadsheets/d/1u_28C3lqLlmZElBDb6KuG6qSYuCEK32R40RyjOvWnBE/edit

Hierarchy:
SauceConfigs
----> SauceConfigsBrowser
--------> SauceConfigsChrome

There isn't any way that we're going to be able to avoid all duplication going forward (especially once we add in mobile things), but this approach should do a fair job of removing most of the duplication.

Next steps:

Implement Edge, Firefox, IE & Safari based on which of these are allowed per browser:

seleniumVersion
chromedriverVersion
iedriverVersion
geckodriverVersion
avoidProxy
extendedDebugging

Override methods in subclass to ensure incompatible values aren't being set (e.g. setPlatformName(SaucePlatform:MAC_SIERRA) on a SauceConfigsChrome instance).
Add Visual support in SauceOptions and SauceBrowserConfigs
Add Mobile support in SauceOptions and new subclasses of SauceConfigs

Considerations:

We could deprecate the other constructors of SauceOptions to ensure everyone goes through the Configs classes
We could put the static methods in a SauceOptionsFactory class instead of SauceOptions if that makes more sense

titusfortner · 2020-11-30T14:47:34Z

Ok, actually we might want to update this implementation to distinguish w3c from sauce settings like #237 , which will be useful when we add visual and mobile. I think we can literally merge the implementation of 237 and then add this on top

nadvolod · 2020-12-01T14:57:15Z

java/src/main/java/com/saucelabs/saucebindings/configs/SauceConfigsChrome.java

+        sauceOptions = new SauceOptions(chromeOptions);
+    }
+
+    public SauceConfigsChrome setCapturePerformance(Boolean bool) {


I did document a bug related to this #241

latest commit throws an exception if setCapturePerformance() is called before setName() we can remove this if/when we implement the automatic naming idea for the test runners.

nadvolod · 2020-12-01T15:05:46Z

java/src/main/java/com/saucelabs/saucebindings/configs/SauceConfigsBrowser.java

+    }
+
+
+    public T setVideoUploadOnPass(Boolean bool) {


I'm not sure if this is the time to do it, but I've been wondering if it makes sense to be passing Booleans in our setters. All of these capabilities are either enabled or disabled by default. So our methods only really do one of the options. . Does it make sense for a user to do setVideoUploadOnPass(true) when the only reasonable option is disableVideoUploadOnPass()?

Yes! I like this! Passing in Boolean parameters has always seemed clunky to me.

Ok, updated the code to use disable when the default is true and set when the default is false; neither takes an argument

nadvolod

Overall I think that this makes sense. I've left a few minor comments unrelated to the main API.

nadvolod · 2020-12-01T15:12:07Z

java/src/main/java/com/saucelabs/saucebindings/configs/SauceConfigsBrowser.java

+import java.time.Duration;
+import java.util.Map;
+
+public class SauceConfigsBrowser<T extends SauceConfigsBrowser<T>> extends SauceConfigs<T> {


What do you all think about calling this BrowserConfiguration and child classes ChromeConfiguration...?

Yeah, I named them before I put them in a separate configs package, so I wanted them all grouped together...
Rethinking from big picture, this is probably how to break it up to best fit the capabilities spreadsheet:
Either Configs or Configurations is fine for me. It probably isn't the biggest issue either way since we theoretically don't want users to even know about the names of the classes. :)

Configs
----> VDCConfigs
--------> DesktopConfigs
------------> ChromeConfigs, EdgeConfigs, SafariConfigs, IEConfigs, FirefoxConfigs
--------> EmulatorConfigs, SimulatorConfigs
----> RDCConfigs
--------> AndroidConfigs, iOSConfigs

diemol

Left a few comments, sorry for not being more helpfult but I probably miss a lot of context.

diemol · 2021-01-15T14:40:17Z

java/src/main/java/com/saucelabs/saucebindings/configs/SauceConfigsChrome.java

+     * @deprecated Not for public use
+     *      Use: {@link SauceOptions#chrome()}} or {@link SauceOptions#chrome(ChromeOptions)}}
+     */
+    @Deprecated


Why don't you leave only with package visibility? Instead of deprecating it.

@diemol I don't understand what the impact of that is. The idea of deprecating it is so that people can still use it but know they need to update their behavior before the next major release. What does changing the package visibility do?

Ok, I moved SauceOptions into a new package, which I think is *bad. I think it is ok if we release this as a 2.0?

diemol · 2021-01-15T14:56:27Z

java/src/main/java/com/saucelabs/saucebindings/SauceOptions.java

@@ -129,6 +130,22 @@
        knownCITools.put("TeamCity", "TEAMCITY_PROJECT_NAME");
    }

+    /**
+     * @return SauceConfigsChrome,
+     *   note: constructor implementation is deprecated for public use, but usage here is intentional


I think you can leave the constructor protected or package visibility to avoid the need of deprecating it.

Oh, right, I see what you are saying. We're going to deprecate it for the next release, then change it to protected/private for next major release. Wanted to give a warning about it first.

Oh, right, this is a new method. got it. (trying to remember what I was doing 2 months ago is hard)

and... it's because I have it in a separate package (com.saucelabs.saucebindings.configs vs com.saucelabs.saucebindings. Any advice on a better way to organize this? I feel like everything should probably be in an options package, but I also think it is considered bad to change the package from one version to another? :) Would it make sense to have everything in the main package?

diemol · 2021-01-15T14:59:55Z

java/src/main/java/com/saucelabs/saucebindings/SauceSession.java

@@ -20,6 +21,10 @@ public SauceSession() {
        this(new SauceOptions());
    }

+    public SauceSession(SauceConfigs configs) {
+        this(configs.build());


I believe this is not really following the builder patter, where one of the ideas is that calling the .build() returns an instanciated object.

This is kind of a hack in case people "do it wrong." We want them to build it themselves and pass it in, but we were concerned that they would get a weird error message if they didn't, and it didn't seem that hard to just add a constructor to make it just work.

diemol · 2021-01-15T15:04:40Z

java/src/main/java/com/saucelabs/saucebindings/configs/SauceConfigs.java

+        return (T) this;
+    }
+
+    public SauceOptions build() {


It feels you'd like to follow the same structure of browser options in Selenium (where EdgeOptions extends ChromiumOptions, etc...), so maybe you can get rid of this build method and only pass instanciated objects?

The goal is to only show users allowed capabilities. So SauceOptions allows everything, and the config class is restricted to only applicable values, but rather than a bunch of other constructors, we just convert the configs class into a sauce options and have that get passed into the session.

diemol · 2021-01-15T15:12:13Z

Also, the Google Docs link is only accessible to Sauce employees, which is not OSS friendly, please consider making it either read only for everyone or add that information to the repo's Wiki.

titusfortner · 2021-01-21T04:20:19Z

Well, the second commit here is bigger than the first one, because I rearranged the packages to make more sense. Except it isn't backward compatible, so maybe I'll want to put everything back, or maybe we push forward and do a 2.0 release next and hope we don't upset too many people already. :-/

I added a ScreenResolution enum (the names are correct but not super useful, but better than nothing?)
I renamed the config classes to make more sense (BaseConfigurations, VDCConfigurations & ChromeConfigurations). Superclasses are abstract so they can't be instantiated directly
I converted Integer parameters to Duration (same thing Selenium is moving to)
Methods taking boolean values changed to allow overriding the default without needing arguments

titusfortner · 2021-01-23T07:36:51Z

This is subsumed by #246

[java] implement Sauce Configs for Chrome

f95a70d

titusfortner added the java Java Bindings label Nov 30, 2020

titusfortner requested review from joshmgrant and nadvolod November 30, 2020 01:38

nadvolod reviewed Dec 1, 2020

View reviewed changes

titusfortner requested a review from diemol December 1, 2020 17:01

diemol reviewed Jan 15, 2021

View reviewed changes

SQUASH - make changes based on feedback

5cd044f

titusfortner force-pushed the configs_chrome branch from f2a070e to 5cd044f Compare January 21, 2021 04:15

This was referenced Jan 23, 2021

Java reorg #245

Closed

Java version 1.1 Proposal #246

Merged

titusfortner closed this Jan 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] implement Sauce Configs for Chrome #243

[java] implement Sauce Configs for Chrome #243

titusfortner commented Nov 30, 2020

titusfortner commented Nov 30, 2020

nadvolod Dec 1, 2020

titusfortner Jan 21, 2021

nadvolod Dec 1, 2020

titusfortner Dec 1, 2020

titusfortner Jan 21, 2021

nadvolod left a comment

nadvolod Dec 1, 2020

titusfortner Dec 1, 2020

diemol left a comment

diemol Jan 15, 2021

titusfortner Jan 21, 2021

titusfortner Jan 21, 2021

diemol Jan 15, 2021

titusfortner Jan 21, 2021

titusfortner Jan 21, 2021

titusfortner Jan 21, 2021

diemol Jan 15, 2021

titusfortner Jan 21, 2021

diemol Jan 15, 2021

titusfortner Jan 21, 2021

diemol commented Jan 15, 2021

titusfortner commented Jan 21, 2021

titusfortner commented Jan 23, 2021

[java] implement Sauce Configs for Chrome #243

[java] implement Sauce Configs for Chrome #243

Conversation

titusfortner commented Nov 30, 2020

titusfortner commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadvolod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diemol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diemol commented Jan 15, 2021

titusfortner commented Jan 21, 2021

titusfortner commented Jan 23, 2021