Skip to content

Java visual #276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions .github/workflows/jdk8.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# This workflow will build a Java project with Maven
# For more information see: https://help.github.com/actions/language-and-framework-guides/building-and-testing-java-with-maven

name: JDK 8
env:
SCREENER_API_KEY: ${{ secrets.SCREENER_API_KEY }}
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}

on:
push:
Expand All @@ -19,9 +21,6 @@ jobs:
with:
java-version: 1.8
- name: Test with Maven
env:
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
run: make java_tests
junit4:
runs-on: ubuntu-latest
Expand All @@ -32,9 +31,6 @@ jobs:
with:
java-version: 1.8
- name: Test with Maven
env:
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
run: make junit4_tests
junit5:
runs-on: ubuntu-latest
Expand All @@ -45,7 +41,4 @@ jobs:
with:
java-version: 1.8
- name: Test with Maven
env:
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
run: make junit5_tests
13 changes: 4 additions & 9 deletions .github/workflows/jdk9.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
# For more information see: https://help.github.com/actions/language-and-framework-guides/building-and-testing-java-with-maven

name: JDK 9+
env:
SCREENER_API_KEY: ${{ secrets.SCREENER_API_KEY }}
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}

on:
push:
Expand All @@ -19,9 +23,6 @@ jobs:
with:
java-version: 9
- name: Test with Maven
env:
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
run: cd java/main && mvn clean test -Dmaven.javadoc.skip=true;
junit4:
runs-on: ubuntu-latest
Expand All @@ -32,9 +33,6 @@ jobs:
with:
java-version: 9
- name: Test with Maven
env:
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
run: cd java/junit4 && mvn clean test -Dmaven.javadoc.skip=true;
junit5:
runs-on: ubuntu-latest
Expand All @@ -45,7 +43,4 @@ jobs:
with:
java-version: 9
- name: Test with Maven
env:
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
run: cd java/junit5 && mvn clean test -Dmaven.javadoc.skip=true;
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package com.saucelabs.saucebindings;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class GitManager {
private static String currentBranch = "_default_";
Copy link

@mykola-mokhnach mykola-mokhnach Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does such branch (default) really exist?

Copy link
Collaborator Author

@titusfortner titusfortner Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public static String getCurrentBranch() {
try {
Process process = Runtime.getRuntime().exec("git rev-parse --abbrev-ref HEAD");
process.waitFor();
BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
String line = reader.readLine();
if (!line.contains("fatal:")) {
currentBranch = line;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the line be trimmed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@mykola-mokhnach mykola-mokhnach Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.saucelabs.saucebindings;

public class SauceVisualException extends RuntimeException {
public SauceVisualException(String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package com.saucelabs.saucebindings;

import lombok.Getter;
import lombok.Setter;
import org.openqa.selenium.remote.RemoteWebDriver;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the field be private?

private Map<String, Object> totals;

private final Boolean passed;
private final String message;
private final String status;
private final List<VisualSnapshot> snapshots = new ArrayList<>();
private final Long totalNew;
private final Long totalChanged;
private final Long totalRejected;
private final Long totalAccepted;
private final Long total;

public static VisualResults generate(RemoteWebDriver driver) {
return new VisualResults(driver);
}

private VisualResults(RemoteWebDriver driver) {
results = (Map<String, Object>) driver.executeScript("/*@visual.end*/");

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to have an enum instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (Map state : (List<Map>) results.get("states")) {
Copy link

@mykola-mokhnach mykola-mokhnach Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also be rewritten in more functional style:

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

snapshots.add(new VisualSnapshot(state));
}

this.totals = (Map) results.get("totals");
this.totalNew = (Long) totals.get("new");
this.totalChanged = (Long) totals.get("changed");
this.totalRejected = (Long) totals.get("rejected");
this.totalAccepted = (Long) totals.get("accepted");
this.total = (Long) totals.get("all");
}

public String getSummary() {
String disposition = getPassed() ? getStatus() : getMessage();
return "Totals: " + getTotals() + "; Disposition: " + disposition + "\n";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package com.saucelabs.saucebindings;

import com.saucelabs.saucebindings.options.SauceOptions;
import com.saucelabs.saucebindings.options.VisualOptions;
import org.openqa.selenium.InvalidArgumentException;
import org.openqa.selenium.remote.RemoteWebDriver;

import java.net.MalformedURLException;
import java.net.URL;

public class VisualSession extends SauceSession {
private VisualResults visualResults;
private final VisualOptions visualOptions;

public VisualSession(String testName) {
this(new VisualOptions(SauceOptions.chrome().setName(testName).build()));
}

public VisualSession(VisualOptions options) {
super(options.getSauceOptions());
this.visualOptions = options;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this. is redundant

newVisualTest(getSauceOptions().sauce().getName());
return driver;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the URL be moved into a constant/config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} catch (MalformedURLException e) {
throw new InvalidArgumentException("Invalid URL", e);
}
}

public void newVisualTest(String testName) {
Copy link

@mykola-mokhnach mykola-mokhnach Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

validateVisualStatus();
driver.executeScript("/*@visual.init*/", testName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

public void takeSnapshot(String name) {
validateVisualStatus();
driver.executeScript("/*@visual.snapshot*/", name);
}

public VisualResults getVisualResults() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (visualResults == null) {
this.visualResults = VisualResults.generate(driver);
}
return visualResults;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic strings

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stop(result);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

super.stop(result);
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can observe some code duplication here

super.stop(result);
}
}

private void validateVisualStatus() {
if (this.visualResults != null) {
throw new SauceVisualException("Can not execute Visual Commands after calling getVisualResults()");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.saucelabs.saucebindings;

import lombok.Getter;
import lombok.Setter;

import java.util.Map;

@Setter @Getter
public class VisualSnapshot {
private final String name;
private final String groupName;
private final String status;
private final String url;

public VisualSnapshot(Map<String, String> results) {
this.name = results.get("name");
this.groupName = results.get("groupName");
this.status = results.get("status");
this.url = results.get("url");
}

@Override
public String toString() {
return name + ": " + url;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package com.saucelabs.saucebindings.options;

import com.saucelabs.saucebindings.CITools;
import com.saucelabs.saucebindings.GitManager;
import com.saucelabs.saucebindings.SauceSession;
import com.saucelabs.saucebindings.SystemManager;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.Setter;
import lombok.experimental.Accessors;
import org.openqa.selenium.MutableCapabilities;

import java.util.Arrays;
import java.util.List;
import java.util.Map;

@Accessors(chain = true) @Setter @Getter
public class VisualOptions extends BaseOptions {
@Setter(AccessLevel.NONE) private SauceOptions sauceOptions;
private String projectName;
private String viewportSize;
private String branch;
private String baseBranch;
private Map<String, Object> diffOptions = null;
private String ignore;
private Boolean failOnNewStates = null;
private Boolean alwaysAcceptBaseBranch = null;
private Boolean disableBranchBaseline = null;
private Boolean scrollAndStitchScreenshots = null;
private Boolean disableCORS = null;
private Boolean iframes = null;
private Map<String, Object> iframesOptions = null;

public final List<String> validOptions = Arrays.asList(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually wrap constants into Collections.immutableList

"projectName",
"viewportSize",
"branch",
"baseBranch",
"diffOptions",
"ignore",
"failOnNewStates",
"alwaysAcceptBaseBranch",
"disableBranchBaseline",
"scrollAndStitchScreenshots",
"disableCORS",
"iframes",
"iframesOptions");

public VisualOptions(String testName) {
this(SauceOptions.chrome().setName(testName).build());
}

public VisualOptions(SauceOptions options) {
if (options.sauce().getName() == null) {
String msg = "Visual Tests Require setting a name in SauceOptions";
throw new InvalidSauceOptionsArgumentException(msg);
}

this.sauceOptions = options;
capabilityManager = new CapabilityManager(this);
}

/**
* @return instance of MutableCapabilities representing all key value pairs set in SauceOptions
* @see SauceSession#start()
*/
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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (projectName == null) {
projectName = CITools.getBuildName();
}

if (branch == null) {
branch = GitManager.getCurrentBranch();
}

capabilityManager.addCapabilities();

MutableCapabilities toReturn = getSauceOptions().toCapabilities();
toReturn.setCapability("sauce:visual", capabilities);

return toReturn;
}

/**
* Use Case is pulling serialized information from JSON/YAML, converting it to a HashMap and passing it in
* This is a preferred pattern as it avoids conditionals in code
* Warning: For VisualOptions this adds a hard coded Test Name that needs to be updated
*
* @param capabilitiesToMerge a Map object representing key value pairs to convert to capabilities
*/
public void mergeCapabilities(Map<String, Object> capabilitiesToMerge) {
for (Map.Entry<String, Object> entry : capabilitiesToMerge.entrySet()) {
String key = entry.getKey();
if (key.equals("visual")) {
((Map<String, Object>) entry.getValue()).forEach(this::setCapability);
} else {
sauceOptions.setCapability(key, entry.getValue());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can sauceOptions be null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
}
Loading