Graphene Extension - RushEye for visual validation#170
Conversation
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
Hi,
thank you very much for the PR. The implementation and usage look great!
I've attached a few comments there. Apart from that, I have several other requests:
- create another module called arquillian-graphene-rusheye-api that will contain annotations and interfaces - you know - to separate API
- Add javadoc at least to the API classes and methods
- please, for all if statements use curly braces
- perform auto-formating for all files - there are sometimes different indentation
- would be good to have the code covered by unit-tests or integration tests...
| } | ||
| ---- | ||
|
|
||
| * Using ```@Snap```, Page objects / Page abstractions are mapped to the baseline(snapshot) images for the visual validation. |
There was a problem hiding this comment.
I guess that it is possible to use it also for test cases without page object/fragments?
https://github.com/MatousJobanek/examples/blob/master/drone-graphene/drone-grahene-simple/src/test/java/arquillian/drone/graphene/simple/GoogleUITest.java
Maybe you could start with this simple example and then jump to additional abstractions
| } | ||
| } catch (Exception e) { | ||
| throw new NoSuchMaskException(maskFiles.toString(), e); | ||
| }*/ |
There was a problem hiding this comment.
for every TODO create an issue - to not forget
| import java.lang.annotation.Target; | ||
|
|
||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ElementType.TYPE}) |
There was a problem hiding this comment.
How about using it also for test methods...?
| return this; | ||
| } | ||
|
|
||
| public Ocular sleep(long time, TimeUnit timeUnit) { |
|
|
||
| import org.arquillian.rusheye.suite.ComparisonResult; | ||
|
|
||
| public class OcularResult extends ComparisonResult{ |
There was a problem hiding this comment.
would be good to have an interface for this
| <dependency> | ||
| <groupId>org.jboss.arquillian.graphene</groupId> | ||
| <artifactId>graphene-webdriver</artifactId> | ||
| <version>${version.org.jboss.arquillian.graphene}</version> |
There was a problem hiding this comment.
the property version.org.jboss.arquillian.graphene is redundant - use here the ${project.version}
| <artifactId>testng</artifactId> | ||
| <version>6.11</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
all modules are using JUnit for testing - would be good to have it consistent
| <version>${version.org.jboss.arquillian.drone}</version> | ||
| <type>pom</type> | ||
| <scope>import</scope> | ||
| </dependency> |
There was a problem hiding this comment.
these two are already defined in the parent pom
| <dependency> | ||
| <groupId>org.arquillian.rusheye</groupId> | ||
| <artifactId>rusheye-impl</artifactId> | ||
| <version>1.0.0</version> |
| *.iml | ||
| .idea | ||
| gh-pages | ||
| gh-pages |
There was a problem hiding this comment.
.gitignore is not necessary here, is it?
|
@vinsguru Hello, how is going this PR? Just to plan a new release with this new feature on Graphene :) |
|
@lordofthejars @MatousJobanek , Thanks for the review comments. Got stuck with some project work. Will work on this. |
|
Hi @rmpestano, |
|
@rmpestano sorry for my long inactivity. As @vinsguru is not responding, I would say that you can (if you want) start moving the stuff forward on your forked branch. |
|
+1 let's try to move this forward and let's try to record a demo showing this feature. |
|
Hi guys, I din't forgot this, as soon a I got some time I'll dig into this. (Its conference season here at my city) |
5 participants
Create an arquillian graphene extension for validating the visual aspects of the application under test using Rusheye.