Feature/inconsistent screenshot height

CHANGELOG.md

@@ -13,6 +13,7 @@ All notable changes to AET will be documented in this file.
 - [PR-396](https://github.com/Cognifide/aet/pull/396) Added horizontal scrollbar for wide pages ([#393](https://github.com/Cognifide/aet/issues/393)) 
 - [PR-403](https://github.com/Cognifide/aet/pull/403) Conditionally passed tests can be accepted ([#400](https://github.com/Cognifide/aet/issues/400))
 - [PR-387](https://github.com/Cognifide/aet/pull/387) Set max allowed page screenshot height to 35k pixels.
+- [PR-397](https://github.com/Cognifide/aet/pull/397) Add algorithm to enable taking long screenshots without resolution-sleep-resolution workaround 
 
 ## Version 3.0.1
 

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/resolution/ResolutionModifier.java

@@ -19,8 +19,9 @@
 import com.cognifide.aet.job.api.ParametersValidator;
 import com.cognifide.aet.job.api.collector.CollectorJob;
 import com.cognifide.aet.job.api.exceptions.ParametersException;
-import com.cognifide.aet.job.api.exceptions.ProcessingException;
+import com.cognifide.aet.job.common.utils.Sampler;
 import java.util.Map;
+import java.util.function.Supplier;
 import org.apache.commons.lang3.math.NumberUtils;
 import org.openqa.selenium.Dimension;
 import org.openqa.selenium.JavascriptExecutor;
@@ -39,27 +40,39 @@ public class ResolutionModifier implements CollectorJob {
 
   private static final String HEIGHT_PARAM = "height";
 
+  private static final String SAMPLING_PERIOD_PARAM = "samplingPeriod";
+
   private static final String JAVASCRIPT_GET_BODY_HEIGHT = "return document.body.scrollHeight";
 
-  private static final int MAX_SIZE = 35000;
+  private static final int HEIGHT_MAX_SIZE = 35000;
 
   private static final int INITIAL_HEIGHT = 300;
 
   private static final int HEIGHT_NOT_DEFINED = 0;
 
+  private static final int DEFAULT_SAMPLING_WAIT_PERIOD = 100;
+
+  private static final int MAX_SAMPLES_THRESHOLD = 15;
+
+  private static final int SAMPLE_QUEUE_SIZE = 3;
+
+  private static final int MAX_SAMPLING_PERIOD = 10000;
+
   private final WebDriver webDriver;
 
   private int width;
 
   private int height;
 
+  private int samplingPeriod;
+
   public ResolutionModifier(WebDriver webDriver) {
     this.webDriver = webDriver;
   }
 
 
   @Override
-  public CollectorStepResult collect() throws ProcessingException {
+  public CollectorStepResult collect() {
     setResolution(this.webDriver);
     return CollectorStepResult.newModifierResult();
   }
@@ -68,30 +81,52 @@ public CollectorStepResult collect() throws ProcessingException {
   public void setParameters(Map<String, String> params) throws ParametersException {
     if (params.containsKey(WIDTH_PARAM)) {
       width = NumberUtils.toInt(params.get(WIDTH_PARAM));
-      ParametersValidator.checkRange(width, 1, MAX_SIZE, "Width should be greater than 0");
+      ParametersValidator.checkRange(width, 1, HEIGHT_MAX_SIZE, "Width should be greater than 0");
       if (params.containsKey(HEIGHT_PARAM)) {
-        height = NumberUtils.toInt(params.get(HEIGHT_PARAM));
-        ParametersValidator
-            .checkRange(height, 1, MAX_SIZE, "Height should be greater than 0 and smaller than " + MAX_SIZE);
+        setHeight(params);
+      } else {
+        setHeightSamplingPeriod(params);
       }
     } else {
       throw new ParametersException("You have to specify width, height parameter is optional");
     }
   }
 
+  private void setHeight(Map<String, String> params) throws ParametersException {
+    height = NumberUtils.toInt(params.get(HEIGHT_PARAM));
+    ParametersValidator
+        .checkRange(height, 1, HEIGHT_MAX_SIZE,
+            "Height should be greater than 0 and smaller than " + HEIGHT_MAX_SIZE);
+  }
+
+  private void setHeightSamplingPeriod(Map<String, String> params) throws ParametersException {
+    samplingPeriod = NumberUtils
+        .toInt(params.get(SAMPLING_PERIOD_PARAM), DEFAULT_SAMPLING_WAIT_PERIOD);
+    ParametersValidator
+        .checkRange(samplingPeriod, 0, MAX_SAMPLING_PERIOD,
+            "samplingPeriod should be greater than or equal 0 and smaller or equal "
+                + MAX_SAMPLING_PERIOD);
+  }
+
   private void setResolution(WebDriver webDriver) {
-    Window window = webDriver.manage().window();
     if (height == HEIGHT_NOT_DEFINED) {
-      window.setSize(new Dimension(width, INITIAL_HEIGHT));
-      JavascriptExecutor js = (JavascriptExecutor) webDriver;
-      height = Integer
-          .parseInt(js.executeScript(JAVASCRIPT_GET_BODY_HEIGHT).toString());
-      if (height > MAX_SIZE) {
-        LOG.warn("Height is over browser limit, changing height to {}", MAX_SIZE);
-        height = MAX_SIZE;
+      height = calculateWindowHeight(webDriver);
+      if (height > HEIGHT_MAX_SIZE) {
+        LOG.warn("Height is over browser limit, changing height to {}", HEIGHT_MAX_SIZE);
+        height = HEIGHT_MAX_SIZE;
       }
     }
     LOG.info("Setting resolution to  {}x{}  ", width, height);
-    window.setSize(new Dimension(width, height));
+    webDriver.manage().window().setSize(new Dimension(width, height));
+  }
+
+  private int calculateWindowHeight(WebDriver webDriver) {
+    Window window = webDriver.manage().window();
+    window.setSize(new Dimension(width, INITIAL_HEIGHT));
+
+    Supplier<Integer> heightSupplier = () -> Integer.parseInt(
+        ((JavascriptExecutor) webDriver).executeScript(JAVASCRIPT_GET_BODY_HEIGHT).toString());
+    return Sampler
+        .waitForValue(heightSupplier, samplingPeriod, SAMPLE_QUEUE_SIZE, MAX_SAMPLES_THRESHOLD);
   }
 }

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/sleep/SleepModifier.java

@@ -20,6 +20,7 @@
 import com.cognifide.aet.job.api.collector.CollectorJob;
 import com.cognifide.aet.job.api.exceptions.ParametersException;
 import com.cognifide.aet.job.api.exceptions.ProcessingException;
+import com.cognifide.aet.job.common.utils.CurrentThread;
 import java.util.Map;
 import org.apache.commons.lang3.math.NumberUtils;
 import org.slf4j.Logger;
@@ -53,11 +54,7 @@ public void setParameters(Map<String, String> params) throws ParametersException
   }
 
   private void sleepInMillis(int ms) {
-    try {
-      Thread.sleep(ms);
-    } catch (InterruptedException e) {
-      Thread.currentThread().interrupt();
-    }
+    CurrentThread.sleep(ms);
   }
 
 }

core/jobs/src/main/java/com/cognifide/aet/job/common/utils/CurrentThread.java

@@ -0,0 +1,28 @@
+/**
+ * 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.
+ */
+
+package com.cognifide.aet.job.common.utils;
+
+public final class CurrentThread {
+
+  public static void sleep(int ms) {
+    try {
+      Thread.sleep(ms);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+    }
+  }
+}

core/jobs/src/main/java/com/cognifide/aet/job/common/utils/Sampler.java

@@ -0,0 +1,68 @@
+/**
+ * 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.
+ */
+package com.cognifide.aet.job.common.utils;
+
+import java.util.function.Supplier;
+import org.apache.commons.collections.buffer.CircularFifoBuffer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class Sampler {
+
+  private static final Logger LOG = LoggerFactory.getLogger(Sampler.class);
+
+  /**
+   * Collects values from supplier in specified periods of time, and compares last n samples every
+   * iteration. If all n samples are equal(), returns the value. If last n samples don't match
+   * before max iterations threshold is reached, returns last collected sample.
+   *
+   * @param samplesSupplier supplier of value to wait for,
+   * @param samplingPeriod milliseconds period between taking each sample,
+   * @param sampleQueueSize defines the last n elements that are to be compared,
+   * @param maxSamplesThreshold max number of samples before return
+   * @return last collected sample
+   */
+  public static <T> T waitForValue(Supplier<T> samplesSupplier, int samplingPeriod,
+      int sampleQueueSize, int maxSamplesThreshold) {
+    CircularFifoBuffer samplesQueue = new CircularFifoBuffer(sampleQueueSize);
+
+    int samplesTaken = 0;
+    while (!isThresholdReached(samplesTaken, maxSamplesThreshold) &&
+        !areAllSamplesEqual(samplesQueue)) {
+
+      CurrentThread.sleep(samplingPeriod);
+
+      T nextSample = samplesSupplier.get();
+      samplesQueue.add(nextSample);
+      ++samplesTaken;
+    }
+    return (T) samplesQueue.get();
+  }
+
+  private static boolean isThresholdReached(int samplesTaken, int maxSamplesThreshold) {
+    if (samplesTaken >= maxSamplesThreshold) {
+      LOG.warn("Sampling reached threshold");
+      return true;
+    }
+    return false;
+  }
+
+  private static boolean areAllSamplesEqual(CircularFifoBuffer samplesQueue) {
+    return samplesQueue.isFull() &&
+        samplesQueue.stream().allMatch(sample -> samplesQueue.get() != null &&
+            samplesQueue.get().equals(sample));
+  }
+}

core/jobs/src/test/java/com/cognifide/aet/job/common/utils/SamplerTest.java

@@ -0,0 +1,64 @@
+/**
+ * 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.
+ */
+package com.cognifide.aet.job.common.utils;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Random;
+import java.util.function.Supplier;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SamplerTest {
+
+  private static final int MAX_SAMPLES_THRESHOLD = 5;
+  private static final int SAMPLE_QUEUE_SIZE = 3;
+  private static final int SAMPLING_PERIOD = 1;
+
+  @Mock
+  private Supplier<Integer> supplier;
+
+  @Test
+  public void sampleChangingValueTest_AllSamplesMatch() {
+    when(supplier.get()).thenReturn(0);
+
+    Integer finalSample = Sampler
+        .waitForValue(supplier, SAMPLING_PERIOD, SAMPLE_QUEUE_SIZE, MAX_SAMPLES_THRESHOLD);
+
+    assertEquals((int) finalSample, 0);
+  }
+
+  @Test
+  public void sampleChangingValueTest_AllSamplesDiffer_ThresholdReached() {
+    Random random = new Random();
+    when(supplier.get())
+        .thenReturn(random.nextInt())
+        .thenReturn(random.nextInt())
+        .thenReturn(random.nextInt())
+        .thenReturn(random.nextInt())
+        .thenReturn(random.nextInt());
+
+    Sampler.waitForValue(supplier, 1, 3, 5);
+
+    verify(supplier, times(5)).get();
+  }
+}

documentation/src/main/wiki/ResolutionModifier.md

@@ -14,10 +14,14 @@ Module name: **resolution**
 | --------- | ----- | ----------- | --------- |
 | `width` | int (1 to 35000) | Window width | yes |
 | `height` | int (1 to 35000) | Window height | no |
+| `samplingPeriod` | int (milliseconds) | Used when `height` is not defined. Defaults to 100ms (see notes below) | no |
+
+TODO: Change 15000 to 35000 when (https://github.com/Cognifide/aet/pull/387) is merged.
 
 | Note |
 |:------ |
-| When height is not specified then it's computed by JavaScript (using `document.body.scrollHeight` property). | 
+| When `height` is not specified then it's computed by JavaScript (using `document.body.scrollHeight` property). |
+| For very long pages, it may take some time to render the page in order to get its full height, so AET is using an algorithm that samples the page's height over some specified period of time. `samplingPeriod` specifies the amount of time between taking each sample. If defined number of samples would match (3 last samples) or when the max number of samples is reached (15), the acquired valued is used as `height` resolution for screenshot.| 
 | **If the resolution is specified without height parameter it should be specified after [`open`](https://github.com/Cognifide/aet/wiki/Open)** and after all modifiers which may affect the page height (e.g. [`hide`](https://github.com/Cognifide/aet/wiki/HideModifier))  |
 
 ##### Example Usage
@@ -47,35 +51,6 @@ Module name: **resolution**
 </suite>
 ```
 
-##### Known issues
-
-[#357](https://github.com/Cognifide/aet/issues/357) - If you're using the auto-height calculation feature of [[Resolution Modifier|ResolutionModifier]], it may happen that the
-height of collected screenshot is different every time you run the suite, which results in failures on the report.
-Currently you can use one of following workarounds to fix this issues:
-* specify the `height` parameter manually with a value which is equal or greater than the height of page you want to test, e.g.:
-  ```$xml
-    <open/>
-    <resolution width="1366" height="5000"/>
-    <scren/>
-  ```
-* use an additional `resolution` modifier with any `height` (value doesn't matter) before the `open` phase - to ensure that 
-  the page will be opened with desired `width` and the 2nd `resolution` will only compute and change the height.
-  ```$xml
-  <resolution width="1366" height="100"/>
-  <open/>
-  <resolution width="1366"/>
-  <scren/>
-  ```
-* use two `resolution` modifiers with the same `width` attribute (the first one may also have `height` attribute with any value)
- and a `sleep` modifier between them, e.g:
-  ```$xml
-  <open/>
-  <resolution width="1366"/>
-  <sleep duration="1000"/>
-  <resolution width="1366"/>
-  <scren/>
-  ```
-
 #### Tips and tricks
 
 In order to make sure that your screenshots have the resolution you expect them to have you need to test it first.

documentation/src/main/wiki/UpgradeNotes.md

@@ -27,11 +27,6 @@ the width of collected screenshot could be different that the width set in the `
 (see "Notes" section in [[Resolution Modifier|ResolutionModifier]] wiki). This issue doesn't occur when using AET 3.0 with Chrome browser - make
 sure to adjust the resolution `width` value when updating your suite from previous AET versions.
 
-##### Known issues
-
-* [#357](https://github.com/Cognifide/aet/issues/357) - see Known issues section in [[Resolution Modifier|ResolutionModifier]] wiki
-for possible workarounds.
-
 #### `aet-maven-plugin` marked as deprecated
 That means it will be no longer supported after release of this version and expect it will be removed soon.
 Please use [[client script|ClientScripts]] instead or simply communicate with AET Web API to schedule your suite.