[Java] visual implemented with subclass

[titusfortner]

This is an alternative to #256
This is more in line with the API @nadvolod created in #182

I think I prefer this one?

You still have to setName() in Options, and VisualSession only has a constructor that takes a SauceOptions instance, but since we are specifying VisualSession, we no longer need to use setVisual() in Options to indicate that it is a visual test. Also, no need to toggle on Data Center; the URL can be hard coded in VisualSession.

• Minimum required:

SauceOptions sauceOptions = SauceOptions.chrome().setName("Test Name").build();
VisualSession session = new VisualSession(sauceOptions);
RemoteWebDriver driver = session.start();

• setVisualOptions(VisualOptions) remains the same as in [Java] Add support for visual tests #256
• Remaining functionality matches [Java] Add support for visual tests #256, except methods don't need visual() method in session:

VisualSession session = new VisualSession(sauceOptions);
session.init("New Test Name");
session.takeSnapshot("Snapshot Name");
Map results = session.getResults();

[nadvolod]

I also prefer this over 256. It's more intuitive and not as annoying to have to do .visual() for all visual commands.

[nadvolod]

@titusfortner I also prefer this approach over PR 256

[nadvolod]

Have we considered enforcing the 3 mandatory parameters through the constructor instead of runtime exceptions?

VisualSession session = new VisualSession(sauceOptions, "My Test Name", "My Project Name");

[titusfortner]

I don't care about project name. "Build Name" is probably what they want, and if they want something different, setProjectName() should be sufficient.

Test Name / init value is a little harder because we can't provide a reasonable default.
I did try some implementations that put it in the constructor, but we probably need to be encouraging people to set this regardless: sauceOptions.setName(testName.getMethodName())

Maybe we update examples for TestRunner instead of just language so this always gets shown?

[nadvolod]

@titusfortner I think projectName is mandatory. Hence, I'm suggesting to make it as part of a constructor.
How does .setName("My Test Name") propogate to visual? In visual there's no concept of test name, rather a snapshotName.
Agreed about init()
Can you elaborate on why you want to encourage people to sauceOptions.setName(testName.getMethodName())?

[titusfortner]

We only need to avoid the default projectName in a Code Parameterized paradigm, not in a Build Differentiated paradigm, and I generally don't think that parameterization is the best solution for configuration variations. Regardless, that can be up to the end user to manage if the default doesn't work for them, but that also needs to be set in VisualOptions class, not the VisualSession class.

setName() is for init() not for takeSnapshot().

I could be convinced to require SauceOptions#setName(Name) in all Sauce tests, not just visual ones. I wish JUnit made it easier to get the method name by default (I'm spoiled in Ruby because RSpec makes it globally accessible from a class variable). Anyway, we should have that discussion prior to Sauce Bindings v2.0.

[titusfortner]

We only need to avoid the default projectName in a Code Parameterized paradigm, not in a Build Differentiated paradigm

Actually looking at your code below this isn't true either.

Instead of "Project Name" it would be "Sauce Bindings" (pulled from the property that is getting set the line before; which is only necessary when running locally, since running it on a CI will use whatever the Build name of the CI is). Actually, we probably should discuss which takes precedence separately.

[nadvolod]

What about something like this @titusfortner ?

@RunWith(Parameterized.class)
public class RealWorldVisualTest {
    private VisualSession session;

    @Before
    public void setup() {
        System.setProperty("BUILD_NAME", "Sauce Bindings");

//Project name is mandatory
        VisualOptions visualOptions = new VisualOptions("Project Name");
        //Visual doesn't support everything that Sauce does
        // so it's disadvantageous to configure browser, version, os through SauceOptions
        visualOptions.setBrowserName(browserName);
        visualOptions.setBrowserVersion(browserVersion);
        visualOptions.setPlatform(platform);

        SauceOptions sauceOptions = new SauceOptions(visualOptions);
        VisualSession session = new VisualSession(sauceOptions);
        session.start();
    }

    @After
    public void cleanUp() {
        if (session != null) {
            //TODO visual.end needs to be called here
            session.stop(true);
        }
    }
    @Parameterized.Parameter
    public String browserName;
    @Parameterized.Parameter(2)
    public String browserVersion;
    @Parameterized.Parameter(1)
    public String platform;
    @Parameterized.Parameter(3)
    public String viewportSize;
    @Parameterized.Parameter(4)
    public String deviceName;

    @Parameterized.Parameters(name = "{4}")
    public static Collection<Object[]> crossBrowserData() {
        return Arrays.asList(new Object[][]{
                {"Chrome", "Windows 10", "latest", "412x732", "Pixel XL"},
                {"Chrome", "Windows 10", "latest", "412x869", "Galaxy Note 10+"},
                {"Safari", "macOS 10.15", "latest", "375x812", "iPhone X"}
        });
    }

    @Test
    public void checksOnePage() {
        // open the page
        session.init(deviceName);
        session.takeSnapshot("Page Name");
session.stop(); //I don't know if this need to be called first to get back an object of results
        assertNull(session.hasVisualDifferences());
    }
}

[titusfortner]

Edit: see code example below, it might explain things better.

Visual doesn't support everything that Sauce does

What makes you say this? Visual tests are run on Sauce, and everything you can do on Sauce you can do with screener.io as a go between.

I guess how we think of it depends on whether we're running Visual Tests as an add-on to existing functional tests or in a separate suite just for visual tests. I kind of like the idea of advocating for them to be separate, but we don't need to force that. I'm actually wondering what it might look like to show off 2 test suites:

1. performance + accessibility + visual
2. functional

I don't think we should have a String in a constructor for something that can have a reasonable default (Sytem.getEnv("BUILD_NAME")). It shouldn't be too big of an issue to do it in 2 lines if you don't want that default:

VisualOptions visualOptions = new VisualOptions();
visualOptions.setProjectName("Project Name")

Hmm, we could store the results in the session rather than requiring a user to get it, but I'm not sure that makes sense since you lose access to visual after stop() is called, and you don't want to have assertions in the Watcher.
This would be my thought, with stop in afterhook / test watcher

VisualResults results = session.getResults();
assertTrue(results.isPassed()); 

I guess we could put it in the test watcher like this if we are mixing functional & visual:

protected void succeeded(Description description) {
    if (isVisualSession) {
        VisualResults result = session.getResults();
        if (!result.isPassed()) {
            session.context("Visual Comparison Failed" + result.getFailure())
            session.stop("failed);
            return;
        }
    }
    session.stop("passed")
}

[titusfortner]

Added VisualResults

What gets printed at the end of a passing test:

Visual Test Results (1 tests): 
	Accepted: 1
	New: 0
	Changed: 0
	Rejected: 0

Failing test:

Visual Test Results (1 tests): 
	Accepted: 0
	New: 0
	Changed: 1
Snapshot Name: https://screener.io/v2/states/60b672e26ede9aca2fa4926d/java_visual_subclass/1024x768/Chrome%2091.0%20Windows%2010/Z1icJFg.Snapshot%2520necessary%2520to%2520for%2520Visual%2520test%2520to%2520pass-0
	Rejected: 0

[titusfortner]

Your example would look like this:

@RunWith(Parameterized.class)
public class RealWorldVisualTest {
    private VisualSession session;
    private RemoteWebDriver driver;

    @Before
    public void setup() {
        // This is used to set default projectName
        System.setProperty("BUILD_NAME", "Sauce Bindings");

        SauceOptions sauceOptions = null;
        VisualOptions visualOptions = new VisualOptions();
        visualOptions.setViewportSize(viewportSize);

        if ("Chrome".equals(browserName)) {
          sauceOptions = SauceOptions().chrome()
                                       .setName(deviceName)
                                       .setBrowserVersion(browserVersion)
                                       .setPlatformName(platform)
                                       .setVisualOptions(visualOptions)
                                       .build();
        } else if ("Safari".equals(browserName)) {
          sauceOptions = SauceOptions().safari()
                                       .setName(deviceName)
                                       .setBrowserVersion(browserVersion)
                                       .setPlatformName(platform)
                                       .setVisualOptions(visualOptions)
                                       .build();
        }
            
        session = new VisualSession(sauceOptions);
        driver = session.start();
    }

    @After
    public void cleanUp() {
        if (session != null) {
            session.stop(true);
        }
    }
    @Parameterized.Parameter
    public String browserName;
    @Parameterized.Parameter(2)
    public String browserVersion;
    @Parameterized.Parameter(1)
    public String platform;
    @Parameterized.Parameter(3)
    public String viewportSize;
    @Parameterized.Parameter(4)
    public String deviceName;

    @Parameterized.Parameters(name = "{4}")
    public static Collection<Object[]> crossBrowserData() {
        return Arrays.asList(new Object[][]{
                {"Chrome", "Windows 10", "latest", "412x732", "Pixel XL"},
                {"Chrome", "Windows 10", "latest", "412x869", "Galaxy Note 10+"},
                {"Safari", "macOS 10.15", "latest", "375x812", "iPhone X"}
        });
    }

    @Test
    public void sauceDemoPage() {
        driver.get("https://www.saucedemo.com");
        session.takeSnapshot("Page Name");
        assertTrue(session.getResults().passed());
    }
}

[titusfortner]

Ok, just added resolution / viewport size enums...

Parameterized values don't have to be Strings, right?
Instead of this:

        return Arrays.asList(new Object[][]{
                {"Chrome", "Windows 10", "latest", "412x732", "Pixel XL"},
                {"Chrome", "Windows 10", "latest", "412x869", "Galaxy Note 10+"},
                {"Safari", "macOS 10.15", "latest", "375x812", "iPhone X"}
        });

We could do this?

    @Parameterized.Parameter
    public BaseConfigurations configClass;
    @Parameterized.Parameter(1)
    public SaucePlatform platform;
    @Parameterized.Parameter(2)
    public String browserVersion;
    @Parameterized.Parameter(3)
    public DeviceDimensions deviceName;

        return Arrays.asList(new Object[][]{
                {SauceOptions.chrome(), SaucePlatform.WINDOWS_10, "latest", Google.PIXEL_X},
                {"SauceOptions.chrome()", SaucePlatform.WINDOWS_10, "latest", Samsung.GALAXY_NOTE_10_PLUS},
                {"SauceOptions.safari()", SaucePlatform.MAC_CATALINA, "latest", Apple.IPHONE_X}
        });

Which would let us do this:

Visual Options visualOptions = new VisualOptions();      
visualOptions.setViewportSize(deviceName.getValue());

sauceOptions = configClass
    .setName(deviceName.getEnumName)
    .setBrowserVersion(browserVersion)
    .setPlatformName(platform)
    .setVisualOptions(visualOptions)
    .build();

Edit: Just realized this can't work because enums can't be subclasses....
Wait, except they can implement an interface... hmmm....
Edit: Ok, implemented with an interface... it's hacky. :(

[nadvolod]

@titusfortner this is certainly a cleaner approach. I'm finding a few small things that are throwing me off:

• SauceOptions are constructed with a .build() while VisualOptions are not. I think that we should follow the same pattern.
• com.saucelabs.saucebindings.examples.VisualTest needs an assertion at the end so the test passes/fails. Currently it's doing session.stop(true);. However, that's not realistic and also doesn't actually show what an assertion in a visual test looks like.

[titusfortner]

Technically it's the static methods (e.g., SauceOptions.chrome()) that are implemented with build().
You can still do new SauceOptions().setFoo(x).setBar(y) without using build().
The difference is that the first one the IDE only shows you the options that are compatible with chrome.

I'd love to be able to have VisualOptions subclass SauceOptions and have VisualOptions.chrome().setVisualFeature(x).setChromeFeature(y).build()
But each static method like chrome() would need to create a class like ChromeVisualConfigurations, which could subclass ChromeConfigurations, but then we'd have to implement the same visual methods in each of the <Browser>VisualConfigurations classes. I just don't see a way to set both visual and browser specific options without a lot of code duplication somewhere.

We could do this:

SauceOptions.chrome().setPlatformName(x).setVisualOptions().setVisualSomething(y).build().setBrowserVersion(z).build()

But having 2 build() methods in there seems more complicated than just passing in a VisualOptions instance.
Plus, it's set up so that VisualOptions parameters get added to sauce:visual and SauceLabsOptions parameters get added to sauce:options. They get to share code like that, just like Appium code will all get prepended with appium: based on the separate class implementation.

[titusfortner]

subsumed by #276