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.

Sign up for GitHub

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

Jump to bottom

Hotfix/screen max height #387

Merged
merged 6 commits into from
Oct 17, 2018
Merged

Hotfix/screen max height #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0520ec7
Increase screenshot max height.
Jakub-Izbicki Oct 9, 2018
f916829
Add tests for long page screenshots
Jakub-Izbicki Oct 11, 2018
30cb4ce
Update changelog
Jakub-Izbicki Oct 11, 2018
677244d
Fix review issues
Jakub-Izbicki Oct 12, 2018
f07c4a0
Remove dynamic content partial to take screenshot of full long page
Jakub-Izbicki Oct 15, 2018
9e9e2b9
Move changelog notes under Unreleased section
Jakub-Izbicki Oct 15, 2018
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ All notable changes to AET will be documented in this file.

## Unreleased
**List of changes that are finished but not yet released in any final version.**
- [PR-387](https://github.com/Cognifide/aet/pull/387) Set max allowed page screenshot height to 35k pixels.

## Version 3.0.1

Expand Down
2 changes: 1 addition & 1 deletion ...s/src/main/java/com/cognifide/aet/job/common/modifiers/resolution/ResolutionModifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class ResolutionModifier implements CollectorJob {

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update Resolution Modifier wiki.

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


private static final int INITIAL_HEIGHT = 300;

Expand Down
2 changes: 1 addition & 1 deletion ...c/test/java/com/cognifide/aet/job/common/modifiers/resolution/ResolutionModifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public class ResolutionModifierTest {

private static final int CUSTOM_HEIGHT = 600;

private static final int BROWSER_HEIGHT_LIMIT = 15000;
private static final int BROWSER_HEIGHT_LIMIT = 35000;

@Mock
private RemoteWebDriver webDriver;
Expand Down
4 changes: 2 additions & 2 deletions documentation/src/main/wiki/ResolutionModifier.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ Module name: **resolution**

| Parameter | Value | Description | Mandatory |
| --------- | ----- | ----------- | --------- |
| `width` | int (1 to 15000) | Window width | yes |
| `height` | int (1 to 15000) | Window height | no |
| `width` | int (1 to 35000) | Window width | yes |
| `height` | int (1 to 35000) | Window height | no |

| Note |
|:------ |
Expand Down
27 changes: 27 additions & 0 deletions integration-tests/sample-site/src/main/webapp/sanity/comparators/layout/long_page.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<%--

AET

Copyright (C) 2013 Cognifide Limited

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

--%>
<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %>
<%@ include file="/includes/header.jsp" %>
<c:forEach begin="1" end="30" varStatus="loop">
<div class="sponsor" style="height: 1000px">
<img src="/sample-site/assets/demo_files/logo.png" alt="Bootswatch" />
</div>
</c:forEach>
<%@ include file="dynamic_content.jsp" %>
8 changes: 4 additions & 4 deletions ...anity-functional/src/test/java/com/cognifide/aet/sanity/functional/HomePageTilesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@
@Modules(GuiceModule.class)
public class HomePageTilesTest {

private static final int TESTS = 136;
private static final int TESTS = 138;

private static final int EXPECTED_TESTS_SUCCESS = 77;
private static final int EXPECTED_TESTS_SUCCESS = 78;

private static final int EXPECTED_TESTS_CONDITIONALLY_PASSED = 9;
private static final int EXPECTED_TESTS_CONDITIONALLY_PASSED = 10;

private static final int EXPECTED_TESTS_WARN = 5;

private static final int EXPECTED_TESTS_FAIL = 54;
private static final int EXPECTED_TESTS_FAIL = 55;

@Inject
private ReportHomePage page;
Expand Down
4 changes: 2 additions & 2 deletions integration-tests/sanity-functional/src/test/resources/features/filtering.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ Feature: Tests Results Filtering
Scenario: Filtering Tests Results: layout
Given I have opened sample tests report page
When I search for tests containing "layout"
Then There are 35 tiles visible
And Statistics text contains "35 ( 15 / 0 / 20 (9) / 0 )"
Then There are 37 tiles visible
And Statistics text contains "37 ( 16 / 0 / 21 (10) / 0 )"

Scenario: Filtering Tests Results: jserrors
Given I have opened sample tests report page
Expand Down
37 changes: 37 additions & 0 deletions integration-tests/test-suite/partials/layout.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,5 +474,42 @@
<url href="comparators/layout/failed.jsp"/>
</urls>
</test>

<test name="F-comparator-Layout-dynamic-element-at-bottom-of-long-page">
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain what is the idea for this failing test?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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();
      }
      . . .

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, fixed!

<collect>
<open/>
<resolution width="767"/>
<!--ToDo: Remove resolution-sleep-resolution workaround after issue #357 is fixed.
(https://github.com/Cognifide/aet/issues/357)-->
<sleep duration="2000"/>
<resolution width="767"/>
<screen/>
</collect>
<compare>
<screen comparator="layout"/>
</compare>
<urls>
<url href="comparators/layout/long_page.jsp"/>
</urls>
</test>

<test name="S-comparator-Layout-long-page-without-dynamic-at-bottom">
<collect>
<open/>
<resolution width="767"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please leave here ToDo comment with info that resolution-sleep-resolution workaround should be removed after #357 is fixed?

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

<!--ToDo: Remove resolution-sleep-resolution workaround after issue #357 is fixed.
(https://github.com/Cognifide/aet/issues/357)-->
<sleep duration="2000"/>
<resolution width="767"/>
<screen exclude-elements=".dynamic1,.dynamic2"/>
</collect>
<compare>
<screen comparator="layout"/>
</compare>
<urls>
<url href="comparators/layout/long_page.jsp"/>
</urls>
</test>

<!-- Layout-Comparator END -->
</suite>