-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: main
Are you sure you want to change the base?
Java visual #276
Changes from all commits
377a8d3
12da378
f33479a
3e55f34
857dd22
e7c8554
6aca22b
1a32387
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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_"; | ||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the line be trimmed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean whether it could be that |
||
} | ||
} catch (IOException | InterruptedException e) { | ||
// Ignore exception and use default | ||
} | ||
return currentBranch; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my scenario is:
If we want to always return the cached value then this method should probably have an explicit condition for that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
} | ||
} |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it make sense to have an enum instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could also be rewritten in more functional style:
|
||
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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume |
||
newVisualTest(getSauceOptions().sauce().getName()); | ||
return driver; | ||
} | ||
|
||
@Override | ||
public URL getSauceUrl() { | ||
try { | ||
return new URL("https://hub.screener.io/wd/hub"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the URL be moved into a constant/config? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
} catch (MalformedURLException e) { | ||
throw new InvalidArgumentException("Invalid URL", e); | ||
} | ||
} | ||
|
||
public void newVisualTest(String testName) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like when methods return There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been treating |
||
if (visualResults == null) { | ||
this.visualResults = VisualResults.generate(driver); | ||
} | ||
return visualResults; | ||
} | ||
|
||
@Override | ||
public void stop(Boolean passed) { | ||
String result = passed ? "passed" : "failed"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. magic strings There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
stop(result); | ||
} | ||
|
||
@Override | ||
public void stop(String result) { | ||
if (driver != null) { | ||
System.out.println(getVisualResults().getSummary()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can sauceOptions be null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, there are 2 constructors. One that accepts a |
||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.