[java] support for Browser on EmuSim #170

titusfortner · 2020-05-30T23:21:28Z

This replaces #169

Note that this is Draft, because it relies on a new PR to Appium getting merged and released. I've built the jar with that commit locally and used it to ensure that this code works as intended.

The main updates from 169 is that I've added all the tests, and iOS is completely supported

So now it looks like this:

AndroidOptions androidOptions = new AndroidOptions();
SauceOptions sauceOptions = SauceOptions.android(androidOptions);
SauceAndroidSession session = new SauceAndroidSession(sauceOptions);
AndroidDriver driver = session.start()

When reviewing, please check out the things I labeled "TODO" for potential considerations.

mykola-mokhnach · 2020-05-31T05:12:20Z

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

+    }
+
+    public static String fromString(String value) {
+        return DeviceOrientationLookup.lookup.get(value);


return this.valueOf(value.toUpperCase())
or
return Stream.of(this.values()).findFirst((x) -> x.value.equalsIgnoreCase(value)).orElse(null)

mykola-mokhnach · 2020-05-31T05:15:14Z

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

+    }
+
+    public static Set keys() {
+        return DeviceOrientationLookup.lookup.keySet();


Could also be return Stream.of(this.values()).map((x) -> x.name()).collect(toList())

Yeah, it took me a while to find an approach that worked for what I needed. This code matches what I'm doing in all the other enums. Is the an advantage to replacing the code in all of them?

I would say it makes sense since we get rid of the redundant data structure. All operations could be done using native methods of the Enum class itself

mykola-mokhnach · 2020-05-31T05:24:25Z

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

+    // These technically belong in Appium's IOSOptions & AndroidOptions
+    // but Sauce has always had them in its documentation, so it should handle them
+    private String app; // TODO - consider handling URL and auto-add "sauce-storage:"
+    private String appActivity; // Android only


The fact this class includes platform-specific options makes me think it needs to be refactored and split

to be more specific

sauceOptions.setAppActivity

must not be compilable if sauceOptions does not target the Android platform

Yeah, from a CS perspective, this is a "bad class" because not all fields apply to all the Sessions. At the same time, the reason it is bad is because it can theoretically be more difficult to maintain when it isn't abstracted out. The stated goal of the project is to make it easy for our users to get what they want, and when balancing out the added maintenance costs here I believe we should err on the side of usability.

Essentially I don't want users to have to spend time figuring out whether options need to be in AndroidOptions or SauceOptions when our docs have always included them. If someone needs things that are only specified in Appium docs or tutorials, that's an easier ask to put in a separate class, especially since I expect that to be a small minority of users.

I guess I should say that we should be comparing this code to MutableCapabilities which has zero protections, and from that standpoint this is better. We could do a better job of evaluating what combinations of capabilities don't work before we send it to Sauce, but I don't think that is MVP for this.

I agree with @mykola-mokhnach here. This causes me a lot of confusion to see a mixture of mobile options and browser based options in a single class. I think that we do want the user to figure out "whether options need to be in AndroidOptions or SauceOptions". AndroidOption for Android, IosOptions for iOS, SauceOptions for VDC. Regardless of our docs, these 3 products have very distinctive responsibility boundaries. I think that should be reflected when a user is trying to use our bindings. Otherwise they'll have this massive class with a massive API. What applies only for android, what applies for iOS? Will also make it difficult for us to name methods. To me it's thee same difference as determining whether to use AppiumOptions or MutableCapabilities. Yes, as a user we need to know if we're testing web or mobile.

mykola-mokhnach · 2020-05-31T05:27:09Z

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

+        SauceOptions sauceOptions = new SauceOptions(options);
+        sauceOptions.setPlatformName(SaucePlatform.ANDROID);
+        sauceOptions.setDeviceName("Android GoogleAPI Emulator");
+        sauceOptions.setPlatformVersion("8.1");


should it be hardcoded?

This is a way to set a different default for each driver instead of defining one in the field directly. Honestly, I'd rather Sauce provide a default when it isn't specified, but until then this software should do it. Obviously it can be overridden as is already necessary with current code.

Also, we probably need a way to track the things that are hard-coded that are likely to change. Ideally we can pull from a config file that can be auto-generated by the API at some point, but that is a lot of work for where we are right now.

nadvolod · 2020-06-02T13:39:05Z

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

+    // These technically belong in Appium's IOSOptions & AndroidOptions
+    // but Sauce has always had them in its documentation, so it should handle them
+    private String app; // TODO - consider handling URL and auto-add "sauce-storage:"
+    private String appActivity; // Android only


I agree with @mykola-mokhnach here. This causes me a lot of confusion to see a mixture of mobile options and browser based options in a single class. I think that we do want the user to figure out "whether options need to be in AndroidOptions or SauceOptions". AndroidOption for Android, IosOptions for iOS, SauceOptions for VDC. Regardless of our docs, these 3 products have very distinctive responsibility boundaries. I think that should be reflected when a user is trying to use our bindings. Otherwise they'll have this massive class with a massive API. What applies only for android, what applies for iOS? Will also make it difficult for us to name methods. To me it's thee same difference as determining whether to use AppiumOptions or MutableCapabilities. Yes, as a user we need to know if we're testing web or mobile.

nadvolod · 2020-06-02T15:05:58Z

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

                }
                break;
+            case "deviceOrientation":
+                if (!DeviceOrientation.keys().contains(value)) {


Wow, this method is getting scary! I created a PR to clean it up #171

nadvolod · 2020-06-02T15:45:07Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

Complexity increasing per file
==============================
- java/src/test/java/com/saucelabs/saucebindings/integration/SauceTestWatcher.java  1
- java/src/test/java/com/saucelabs/saucebindings/SauceIOSSessionTest.java  1
- java/src/test/java/com/saucelabs/saucebindings/SauceAndroidSessionTest.java  1
- java/src/main/java/com/saucelabs/saucebindings/DeviceOrientation.java  1
- java/src/main/java/com/saucelabs/saucebindings/SauceOptions.java  9
- java/src/test/java/com/saucelabs/saucebindings/integration/SauceEmuSimBrowserTest.java  1
- java/src/main/java/com/saucelabs/saucebindings/DeviceType.java  1
- java/src/main/java/com/saucelabs/saucebindings/SauceAndroidSession.java  1
- java/src/test/java/com/saucelabs/saucebindings/SauceMobileOptionsTest.java  1
- java/src/main/java/com/saucelabs/saucebindings/SauceIOSSession.java  1
- java/src/main/java/com/saucelabs/saucebindings/SauceAutomationName.java  1
         

Clones added
============
- java/src/test/java/com/saucelabs/saucebindings/SauceMobileOptionsTest.java  2

See the complete overview on Codacy

nadvolod · 2020-06-02T15:45:07Z

java/src/test/java/com/saucelabs/saucebindings/SauceAndroidSessionTest.java

+import java.net.URL;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.*;


Issue found: Avoid unused imports such as 'org.mockito.Mockito'

nadvolod · 2020-06-02T15:45:08Z

java/src/test/java/com/saucelabs/saucebindings/SauceIOSSessionTest.java

+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.*;


Issue found: Avoid unused imports such as 'org.mockito.Mockito'

[java] support for Browser on EmuSim

fb2e453

titusfortner requested review from joshmgrant and nadvolod May 30, 2020 23:21

titusfortner mentioned this pull request May 30, 2020

Android browser emusim #169

Closed

mykola-mokhnach reviewed May 31, 2020

View reviewed changes

nadvolod reviewed Jun 2, 2020

View reviewed changes

Base automatically changed from master to main March 15, 2021 20:20

[java] support for Browser on EmuSim #170

Are you sure you want to change the base?

[java] support for Browser on EmuSim #170

Uh oh!

Conversation

titusfortner commented May 30, 2020

Uh oh!

mykola-mokhnach May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadvolod commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mykola-mokhnach May 31, 2020 •

edited

Loading

mykola-mokhnach May 31, 2020 •

edited

Loading