Hotfix/screen max height #387
Conversation
Jakub-Izbicki
force-pushed
the
hotfix/screen-max-height
branch
from
6baf272
to
30cb4ce
Compare
CHANGELOG.md
Outdated
| @@ -15,6 +15,7 @@ All notable changes to AET will be documented in this file. | |||
|
|
|||
| - [PR-380](https://github.com/Cognifide/aet/pull/380) Exclude elements position calculated with partial screenshot offset ([#379](https://github.com/Cognifide/aet/issues/379)) | |||
| - [PR-378](https://github.com/Cognifide/aet/pull/378) OSGI-configurable Chrome options. | |||
| - [PR-enter pr no. here](https://github.com/Cognifide/aet/pull/prNumber) Set max allowed page screenshot height to 35k pixels. | |||
There was a problem hiding this comment.
Could you please enter this number here please :) ?
| @@ -41,7 +41,7 @@ | |||
|
|
|||
| private static final String JAVASCRIPT_GET_BODY_HEIGHT = "return document.body.scrollHeight"; | |||
|
|
|||
| private static final int MAX_SIZE = 15000; | |||
| private static final int MAX_SIZE = 35000; | |||
There was a problem hiding this comment.
Please update Resolution Modifier wiki.
| <test name="S-comparator-Layout-long-page-without-dynamic-at-bottom"> | ||
| <collect> | ||
| <open/> | ||
| <resolution width="767"/> |
There was a problem hiding this comment.
Could you please leave here ToDo comment with info that resolution-sleep-resolution workaround should be removed after #357 is fixed?
| @@ -474,5 +474,38 @@ | |||
| <url href="comparators/layout/failed.jsp"/> | |||
| </urls> | |||
| </test> | |||
|
|
|||
| <test name="F-comparator-Layout-dynamic-element-at-bottom-of-long-page"> | |||
There was a problem hiding this comment.
Could you please explain what is the idea for this failing test?
There was a problem hiding this comment.
I think it's about checking if AET are capable of taking partial screenshot of element located at the bottom (.dynamic2) of a very long page.
@Jakub-Izbicki I think you could also add a test (F- type) which takes the screenshot of the whole page without excluding the dynamic content at the bottom - this way we'll check if AET is capable of taking such big screenshots.
There was a problem hiding this comment.
@tkaik You are right about what the test does. Concerning your suggestion about additional test: I don't think it is necessary as, as far I noticed, AET is always doing a full page screenshot, regardless whether partial is present or not? Take a look:
private byte[] takeScreenshot() throws ProcessingException {
try {
if (isSelectorPresent()) {
SeleniumWaitHelper
.waitForElementToBePresent(webDriver, getLocator(), getTimeoutInSeconds());
return getImagePart(getFullPageScreenshot(), webDriver.findElement(getLocator()));
} else {
return getFullPageScreenshot();
}
. . .
There was a problem hiding this comment.
@Jakub-Izbicki You're right, I never noticed that :) However the full screenshot is taken but it's immediately changed to a partial-image - the full screenshot is not saved in the database or displayed on the report - I think it's good idea to test such case as well. What do you think?
There was a problem hiding this comment.
Good idea, fixed!
Jakub-Izbicki
force-pushed
the
hotfix/screen-max-height
branch
from
48f7640
to
677244d
Compare
Change max allowed screenshot pixels height to 35k.
Description
After lowering of max allowed screenshot height from 100k to 15k, some projects were unable to test some of their long pages. After testing, this pr changes the value to 35k pixels.
Types of changes
Checklist:
I hereby agree to the terms of the AET Contributor License Agreement.