Skip to content
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

Screenshots of individual elements of the page #44

Merged
merged 30 commits into from
Jan 20, 2017
Merged

Conversation

subiron
Copy link
Contributor

@subiron subiron commented Jan 13, 2017

added ability to take screenshots of parts of the page by using css or xpath element selectors

Please do not merge yet!
(Base branch is set to "modifiers_enhancements" for clear diff until modifiers_enhancements be merged into master branch)


I hereby agree to the terms of the AET Contributor License Agreement.

import org.openqa.selenium.TakesScreenshot;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use * imports.

@@ -22,76 +22,109 @@
import com.cognifide.aet.job.api.collector.CollectorProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check your formatter settings and make sure you use Google Formatter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

wiiitek and others added 8 commits January 16, 2017 23:49
# Conflicts:
#	core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/w3chtml5/W3cHtml5ComparatorJob.java
…ements' into screen_parts

# Conflicts:
#	core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java
…ements' into screen_parts

# Conflicts:
#	core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java
@subiron subiron changed the base branch from master to feature/modifiers_enhancements January 19, 2017 12:06
@subiron subiron changed the base branch from feature/modifiers_enhancements to master January 19, 2017 12:11
@subiron subiron changed the base branch from master to feature/modifiers_enhancements January 19, 2017 12:11
@subiron subiron self-assigned this Jan 19, 2017
@subiron subiron changed the title Screen parts Screenshots of indyvidual elements of the page Jan 19, 2017
@subiron subiron changed the title Screenshots of indyvidual elements of the page Screenshots of individual elements of the page Jan 19, 2017
Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

Nice work and very valuable feature!

@@ -83,15 +97,62 @@ private boolean isPatternAndResultMD5Identical(byte[] screenshot) {

@Override
public void setParameters(Map<String, String> params) throws ParametersException {
//No parameters here
if (StringUtils.isNotBlank(params.get(XPATH_PARAM)) || StringUtils
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could move setting parameters from WebElementsLocatorParams to some method in WebElementsLocatorParams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is, but this is extra check for screen collector as both of those params are optional.

}
}

private byte[] bufferedImageToByteArray(BufferedImage bufferedImage) throws ProcessingException {
ByteArrayOutputStream temporaryStream = new ByteArrayOutputStream();
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may get benefit from Closable interface that ByteArrayOutputStream implements:

    try (ByteArrayOutputStream temporaryStream = new ByteArrayOutputStream()){
      ImageIO.write(bufferedImage, PNG_FORMAT, temporaryStream);
      temporaryStream.flush();
      return temporaryStream.toByteArray();
    } catch (IOException e) {
      throw new ProcessingException("Unable to convert screenshot part to byte Array", e);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always forget about that

| --------- | ----- | ----------- | --------- |
| `xpath` | xpath_to_element | Xpath to element(s) | optional (either xpath or css) |
| `css` | css_selector_to_element | css selector to element(s)| optional (either xpath or css) |
| `timeout` | 1000ms | The timeout for the element to appear, in milliseconds. The max value of this parameter is 15000 milliseconds (15 seconds). | no (default will be used) |
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks that timeout parameter is used only when one of selectors is used. It should be mentioned here unless its possible to update the code (which would be much more awesome!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean some kind if warning message when timeout param is provided but neither xpath or css ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather thought about mentioning at documentation, that timeout parameter is applicable only when taking a screenshot of a page element. It does not work in fullScreen mode.
But I would be much more happier if timeout would also work in fullScreen mode and there would be no need to change docs then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I would be much more happier if timeout would also work in fullScreen mode and there would be no need to change docs then.

I have no idea how to (for what purpose) use timeout param when taking fullscreen-shot
(we have sleep modifier for that)
imo just update doc, what do you think ?

@@ -118,5 +118,19 @@
<url href="comparators/layout/failed.jsp" />
</urls>
</test>

<test name="comparator-Layout-Partial-SUCCESS">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide one more test for xpath selector.

Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

I've pushed small improvements for this CR.

@subiron subiron changed the base branch from feature/modifiers_enhancements to master January 20, 2017 21:01
@subiron subiron merged commit e7dfa3a into master Jan 20, 2017
@malaskowski malaskowski deleted the screen_parts branch January 20, 2017 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants