Java visual #276

titusfortner · 2022-02-16T23:12:22Z

This is a PR against #275, and it'll stay in Draft until that one is merged.
It subsumes #259

I finally really like this API!

Easiest to see what this does from the examples:

Using default:

sauce_bindings/java/main/src/test/java/com/saucelabs/saucebindings/examples/VisualTest.java

Lines 12 to 26 in 11d7745

    
               // 1. Create Visual Session object with minimum required 
        
               VisualSession session = new VisualSession("Example test name"); 
        
               // 2. Start Session to get the driver 
        
               RemoteWebDriver driver = session.start(); 
        
               // 3. Use the driver in your tests just like normal 
        
               driver.get("https://www.saucedemo.com/"); 
        
               // 4. Take snapshot 
        
               session.takeSnapshot("Name of Snapshot"); 
        
               // 5. Stop the Session 
        
               session.stop(); 
        
           }

Updating Options:

sauce_bindings/java/main/src/test/java/com/saucelabs/saucebindings/examples/VisualOptionsTest.java

Lines 14 to 39 in 11d7745

    
           // 1. Create Sauce Options object with desired test settings 
        
           // Note: setting the test name is required 
        
           SauceOptions sauceOptions = SauceOptions.chrome() 
        
                   .setName("Example Visual Options Test") 
        
                   .build(); 
        
           // 2. Create Visual Options instance with Sauce Options instance 
        
           VisualOptions visualOptions = new VisualOptions(sauceOptions); 
        
           // 3. Set desired visual options 
        
           visualOptions.setFailOnNewStates(false); 
        
           // 4. Create Visual Session instance with Visual Options instance 
        
           VisualSession session = new VisualSession(visualOptions); 
        
           // 5. Start Session to get the driver 
        
           RemoteWebDriver driver = session.start(); 
        
           // 6. Use the driver in your tests just like normal 
        
           driver.get("https://www.saucedemo.com/"); 
        
           // 7. Take snapshot 
        
           session.takeSnapshot("Name of Snapshot"); 
        
           // 8. Stop the Session 
        
           session.stop();

Cool things this does:

Automatically populates the default branch name with the actual branch name if it can determine it
Automatically populates the Sauce Labs job with pass/fail based on whether Visual tests pass/fail
Useful error when attempting to send a visual command after getting results (i.e., calling "end")

I still need to add javadocs because that's important and stuff

mykola-mokhnach · 2022-02-17T06:38:15Z

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

+import java.io.InputStreamReader;
+
+public class GitManager {
+    private static String currentBranch = "_default_";


does such branch (default) really exist?

If you don't specify anything, Screener uses (default), presumably to make sure it isn't a real branch. Unfortunately, this prevents me from clicking a link with that URL because my IDE doesn't like that character. So I changed the default to _default_, also unlikely to be the actual branch name, but I can click on resulting links.

mykola-mokhnach · 2022-02-17T06:40:21Z

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

+        } catch (IOException | InterruptedException e) {
+            // Ignore exception and use default
+        }
+        return currentBranch;


is it ok that a previously remembered value is returned if an exception happens?

Trying to think of a scenario where that could happen and be bad. It's going to just use the default value set at the top. I made these static methods/fields because no one should be switching git branches in the middle of their test execution.

my scenario is:

get current branch

the branch is changed

get current branch and get an exception

the previous (incorrect) branch name is returned

If we want to always return the cached value then this method should probably have an explicit condition for that

I don't think you can programmatically change branches in the middle of a test run. If it was found before, use that, if it wasn't found before, use default. This is a weird nice-to-have-but-not-very-important feature in the scheme of this project.

this totally makes sense to me. What I want to say is that this method is going to still spend time for the command line execution even though the branch name has been already retrieved. the condition I was talking about:

if (!Objects.equal(currentBranch, DEFAULT_BRANCH)) { return currentBranch; } ... do existing stuff to get the branch name from git

mykola-mokhnach · 2022-02-17T06:40:38Z

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

+            BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
+            String line = reader.readLine();
+            if (!line.contains("fatal:")) {
+                currentBranch = line;


should the line be trimmed?

What do you mean? (and I don't remember what all I did to get while trying to get this to work when I wrote it over a year ago).

I mean whether it could be that line ends with a line break. Also, I would assume it would be safer to check the process exit code than the actual stdout

mykola-mokhnach · 2022-02-17T06:43:24Z

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

+
+        this.passed = (Boolean) results.get("passed");
+        this.message = (String) results.get("message");
+        this.status = (String) results.get("status"); // success, failure, error, timeout, cancelled


does it make sense to have an enum instead?

I usually only do enums for Constructor/Method parameters so users don't have to remember the "magic string" to get the code to work correctly. We could do it for the return value here, but the use case for 99% of people is seeing the result displayed in stdout at the end of their test. I can't see anyone asserting on this or using it in logic anywhere.

mykola-mokhnach · 2022-02-17T06:46:15Z

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

+        this.message = (String) results.get("message");
+        this.status = (String) results.get("status"); // success, failure, error, timeout, cancelled
+
+        for (Map state : (List<Map>) results.get("states")) {


this could also be rewritten in more functional style:

((List<Map>) results.get("states")).map(VisualSnapshot::new).forEach(snapshots::add)

mykola-mokhnach · 2022-02-17T06:47:14Z

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

+
+@Setter @Getter
+public class VisualResults {
+    Map<String, Object> results;


should the field be private?

mykola-mokhnach · 2022-02-17T06:48:27Z

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

+    @Override
+    public URL getSauceUrl() {
+        try {
+            return new URL("https://hub.screener.io/wd/hub");


should the URL be moved into a constant/config?

Good point. the getSauceUrl() in the superclass is based on sauceUrl field, which is a Data Center Enum, which doesn't apply for visual. How we use it here is probably going to depend on if we're planning on having a new visual data center or somehow merge the screener.io into saucelabs.com. I'll get more info.

Sauce does have plans to add data centers (internal link- https://saucedev.atlassian.net/browse/VSLT-2178) I'll figure out what the plan is for formatting, etc.

This is what I just got about future data center URL, so I should definitely create a VisualDataCenter enum for these and update when necessary.

[11:19 AM] the future endpoints are going to be visual.eu-central-1.saucelabs.com and visual.hub.eu-central-1.saucelabs.com

mykola-mokhnach · 2022-02-17T06:51:42Z

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

+    @Override
+    public void stop(String result) {
+        if (driver != null) {
+            System.out.println(getVisualResults().getSummary());


do we really want to use println? this function does not allow any external configuration/customization

We want users to be able to know how to see Sauce info by looking at their Jenkins console logs, etc.
For instance, the Jenkins plugin does this by default.
Making it configurable is possible, but out of scope for current purposes.

Really, the link to the build is the most important thing here, imo, which is why it is frustrating that Screener doesn't expose it if the test passes. Trying to get that changed.

mykola-mokhnach · 2022-02-17T06:52:34Z

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

+
+    @Override
+    public void stop(Boolean passed) {
+        String result = passed ? "passed" : "failed";


magic strings

You know, this is a good point. It comes from our using — https://docs.saucelabs.com/basics/test-config-annotation/test-annotation/#methods which is literally *all magic strings.

When we update the implementation to use the API instead of JS to record pass/fail, then the magic strings are completely useless... Yeah, now I want to deprecate the method in SauceSession instead of supporting it in VisualSession.

mykola-mokhnach · 2022-02-17T06:53:41Z

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

+        }
+    }
+
+    public void newVisualTest(String testName) {


I like when methods return this instead of void, so calls could be chained

Heh, I was just getting chastised for using a fluent pattern in code I wrote for Selenium, and that was for a builder construction where it makes more sense to me. In this case it doesn't matter, there is nothing else for the Session to do after this method is called, the next line will be using the driver.

mykola-mokhnach · 2022-02-17T06:54:40Z

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

+        if (driver != null) {
+            VisualResults results = getVisualResults();
+            System.out.println(results.getSummary());
+            String result = results.getPassed() ? "passed" : "failed";


I can observe some code duplication here

mykola-mokhnach · 2022-02-17T06:57:17Z

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

+
+    @Override
+    public RemoteWebDriver start() {
+        this.driver = createRemoteWebDriver(getSauceUrl(), visualOptions.toCapabilities());


I assume this. is redundant

mykola-mokhnach · 2022-02-17T06:58:56Z

java/main/src/main/java/com/saucelabs/saucebindings/options/VisualOptions.java

+    private Boolean iframes = null;
+    private Map<String, Object> iframesOptions = null;
+
+    public final List<String> validOptions = Arrays.asList(


I usually wrap constants into Collections.immutableList

mykola-mokhnach · 2022-02-17T07:01:57Z

java/main/src/main/java/com/saucelabs/saucebindings/options/VisualOptions.java

+    public MutableCapabilities toCapabilities() {
+        String msg = "Environment Variable or System Property for 'SCREENER_API_KEY' must be provided";
+        String sauceVisualApiKey = SystemManager.get("SCREENER_API_KEY", msg);
+        capabilities.setCapability("apiKey", sauceVisualApiKey);


IMHO it is not a very secure practice to expose API keys in capabilities as their values are visible in logs. Why not, for example, use request headers for such purpose?

Screener doesn't accept it in the header (because that's for Sauce credentials). It has to be in the capabilities.
As for body vs header, it might be pedantic, but it is security by convention rather than an actual difference in security. Nothing prevents a logger from recording header information even if it is less common. Either way, Sauce removes references to these keys in our logs.

mykola-mokhnach · 2022-02-17T07:04:39Z

java/main/src/main/java/com/saucelabs/saucebindings/options/VisualOptions.java

+            if (key.equals("visual")) {
+                ((Map<String, Object>) entry.getValue()).forEach(this::setCapability);
+            } else {
+                sauceOptions.setCapability(key, entry.getValue());


can sauceOptions be null?

No, there are 2 constructors. One that accepts a SauceOptions instance, the other creates one.

mykola-mokhnach · 2022-02-17T07:06:39Z

java/main/src/test/java/com/saucelabs/saucebindings/examples/VisualOptionsTest.java

+        session.takeSnapshot("Name of Snapshot");
+
+        // 8. Stop the Session
+        session.stop();


I assume this call should be in a finally block or AfterEach section

The examples directory is for displaying on our website, and are designed to show how easy it is to get the functionality. Best practices would be to use it with the test runner library and not have to think about it at all. :)

mykola-mokhnach · 2022-02-17T07:08:23Z

java/main/src/test/java/com/saucelabs/saucebindings/integration/VisualTest.java

+
+        assertNull(results.getMessage());
+
+        assertEquals(Long.valueOf(1), results.getTotal());


Long.valueOf(1) could be simplified to 1L

mykola-mokhnach · 2022-02-17T07:11:21Z

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

+
+    public void newVisualTest(String testName) {
+        validateVisualStatus();
+        driver.executeScript("/*@visual.init*/", testName);


shall we first verify is the session is active/driver is initialised? I assume customers might be confused by NPE if they call this method without invoking start() first

same comment to the below methods. They only make sense while the session is active

mykola-mokhnach · 2022-02-17T07:13:49Z

java/main/src/test/java/com/saucelabs/saucebindings/integration/VisualTest.java

+        assertEquals("VisualTest getVisualResults", snapshot.getGroupName());
+        assertEquals("accepted", snapshot.getStatus());
+        String url = snapshot.getUrl();
+        assertTrue(url.contains(GitManager.getCurrentBranch()));


I prefer to use something like Assert.assertThat(x, CoreMatchers.containsString("foo"));. This is giving much better error messages with actual/expected values instead of just true/false

mykola-mokhnach · 2022-02-17T07:15:19Z

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

+        driver.executeScript("/*@visual.snapshot*/", name);
+    }
+
+    public VisualResults getVisualResults() {


shall we make it possible to call this method after the session is stopped?

It needs the driver to call executeScript, and we don't have the driver after the session is stopped

yes I know. Currently NPE is going to be thrown in such case (which is anyway not very useful for a client). My though was that we could automatically cache that result if stop is called, so we don't need to query driver instance anymore.

I've been treating stop() as the final everything is done method, but yeah, there's no reason we couldn't cache this.

mykola-mokhnach · 2022-02-17T07:17:36Z

java/main/src/test/java/com/saucelabs/saucebindings/options/VisualOptionsTest.java

+                .build();
+
+        Map<String, Object> diffOptions = new HashMap<>();
+        diffOptions.put("structure", true);


I find this map initialisation syntax a bit too talkative. We might use some available shortcuts instead: https://www.baeldung.com/java-initialize-hashmap

BTW, guava is a part of selenium, so it is safe to use ImmutableMap.of()

lol, yeah, I just started using it. This code is a copy/paste from other stuff I've done. I'll take a look at those examples.

mykola-mokhnach · 2022-02-17T07:20:32Z

java/main/src/test/java/com/saucelabs/saucebindings/options/VisualOptionsTest.java

+
+    @SneakyThrows
+    public Map<String, Object> serialize(String key) {
+        InputStream input = new FileInputStream(new File("src/test/java/com/saucelabs/saucebindings/options.yml"));


I usually use something like https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html with closeable streams to make sure they are properly closed

mykola-mokhnach · 2022-02-17T14:53:11Z

It is a good practice to document public methods in user-facing libs, not sure how much this principle applies to this framework

titusfortner · 2022-02-17T15:21:52Z

Thanks @mykola-mokhnach ! Yes, this definitely needs the javadocs updated before it can be released. That's just never the fun part. :)

titusfortner · 2022-02-17T15:57:28Z

And looks like there are Snapshot options that I'm not supporting yet — https://docs.saucelabs.com/visual/e2e-testing/commands-options/#arguments-1

titusfortner · 2022-03-31T20:18:01Z

Inadvertently closed because I deleted the merged branch that this was PR'd against. Trying to fix now.

titusfortner requested review from nadvolod and c3ccl3ston February 16, 2022 23:12

titusfortner mentioned this pull request Feb 16, 2022

[Java] visual implemented with subclass #259

Closed

titusfortner marked this pull request as draft February 16, 2022 23:22

mykola-mokhnach reviewed Feb 17, 2022

View reviewed changes

titusfortner force-pushed the java_fixes branch from d964028 to da0d6f4 Compare March 1, 2022 19:17

titusfortner force-pushed the java_fixes branch 7 times, most recently from 9aecf9e to fcb089c Compare March 1, 2022 23:27

titusfortner deleted the branch main March 14, 2022 20:19

titusfortner closed this Mar 14, 2022

titusfortner reopened this Mar 31, 2022

titusfortner changed the base branch from java_fixes to main March 31, 2022 20:18

titusfortner added 5 commits March 31, 2022 16:36

[java] basic visual support

377a8d3

[java] visual options support

12da378

[java] dynamically obtain current git branch for visual by default

f33479a

[java] implement visual results

3e55f34

[java] implement visual options from Map

857dd22

nadvolod force-pushed the java_visual branch from 86c1b50 to 857dd22 Compare March 31, 2022 20:39

nadvolod added 3 commits March 31, 2022 17:07

add env variables to jdk 9

e7c8554

add env variables for jdk8

6aca22b

add setFailOnNewState

1a32387

	// 1. Create Visual Session object with minimum required
	VisualSession session = new VisualSession("Example test name");

	// 2. Start Session to get the driver
	RemoteWebDriver driver = session.start();

	// 3. Use the driver in your tests just like normal
	driver.get("https://www.saucedemo.com/");

	// 4. Take snapshot
	session.takeSnapshot("Name of Snapshot");

	// 5. Stop the Session
	session.stop();
	}

	// 1. Create Sauce Options object with desired test settings
	// Note: setting the test name is required
	SauceOptions sauceOptions = SauceOptions.chrome()
	.setName("Example Visual Options Test")
	.build();

	// 2. Create Visual Options instance with Sauce Options instance
	VisualOptions visualOptions = new VisualOptions(sauceOptions);

	// 3. Set desired visual options
	visualOptions.setFailOnNewStates(false);

	// 4. Create Visual Session instance with Visual Options instance
	VisualSession session = new VisualSession(visualOptions);

	// 5. Start Session to get the driver
	RemoteWebDriver driver = session.start();

	// 6. Use the driver in your tests just like normal
	driver.get("https://www.saucedemo.com/");

	// 7. Take snapshot
	session.takeSnapshot("Name of Snapshot");

	// 8. Stop the Session
	session.stop();


		assertNull(results.getMessage());

		assertEquals(Long.valueOf(1), results.getTotal());

Java visual #276

Are you sure you want to change the base?

Java visual #276

Conversation

titusfortner commented Feb 16, 2022

mykola-mokhnach Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

titusfortner Feb 17, 2022 • edited Loading

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

mykola-mokhnach Feb 17, 2022 • edited Loading

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

mykola-mokhnach Feb 17, 2022 • edited Loading

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

mykola-mokhnach Feb 17, 2022 • edited Loading

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

mykola-mokhnach Feb 17, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach commented Feb 17, 2022

titusfortner commented Feb 17, 2022

titusfortner commented Feb 17, 2022

titusfortner commented Mar 31, 2022

mykola-mokhnach Feb 17, 2022 •

edited

Loading

titusfortner Feb 17, 2022 •

edited

Loading

mykola-mokhnach Feb 17, 2022 •

edited

Loading

mykola-mokhnach Feb 17, 2022 •

edited

Loading

mykola-mokhnach Feb 17, 2022 •

edited

Loading

mykola-mokhnach Feb 17, 2022 •

edited

Loading