Newfeature/layout comparator with treshold #293

plutasnyy · 2018-07-25T11:28:45Z

I created error treshold

Description

I added error treshold in pixels and percentages for screen comparator, also I had to create new status for frontend because we would like to know which test passed without changes and which test passed with changes. We changed css and html files for support new status with @m-suchorski . Finally I modified and added a little bit unit tests.

Motivation and Context

For example, the time on the screenshot may be different, and the application may pass this test.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

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

tkaik · 2018-07-25T14:12:51Z

...ion-api/src/main/java/com/cognifide/aet/communication/api/metadata/ComparatorStepResult.java

@@ -51,6 +51,7 @@ public boolean isRebaseable() {
    FAILED,
    WARNING,
    REBASED,
-    PROCESSING_ERROR
+    PROCESSING_ERROR,
+    CONDITIONAL,


I suggest changing it to CONDITIONAL_PASS

tkaik

As discussed, the ImageComparisonResult should only keep a result of image comparison, so it should not keep the threshold values or the methods for checking if the thresholds are not reached. This logic can be moved to e.g. LayoutComparator

EDIT: Please also fix typos: treshold -> threshold

tkaik · 2018-07-27T09:34:51Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

+        && mask.getPixelDifferenceCount() == 0;
+  }
+
+  private void addPixelDifferenceDataToRestult(ComparatorStepResult result,


Please fix the typo Restult -> Result

malaskowski

Let's discuss having additional conditionally passed status and marking it as a separate statistic.

malaskowski · 2018-07-30T10:04:31Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

-    ComparatorStepResult result = new ComparatorStepResult(null, ComparatorStepResult.Status.PASSED,
-        false);
+  boolean hasMaskThresholdWithAcceptableDifference(ImageComparisonResult mask) {
+    if (this.pixelThreshold.isPresent() && this.percentageThreshold.isPresent()) {


this reference is unnecessary here, please remove it.

malaskowski · 2018-07-30T10:07:29Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

      } catch (Exception e) {
        throw new ProcessingException(e.getMessage(), e);
      } finally {
+


Remove this empty line.

malaskowski · 2018-07-30T10:13:11Z

report/src/main/webapp/assets/sass/_variables.scss

@@ -60,6 +60,7 @@ $warning: #f0ad4e;
 $rebased: #0097fe;
 $failed: #bb5a5a;
 $passed: #6f9f00;
+$conditionallyPassed: #aacc2d;


I'm not sure about marking conditionally passed as a separate statistic and color.
There should be proper filter to show conditionally passed, but let's not introduce additional color for now.

tkaik · 2018-07-30T11:49:55Z

As discussed with @Skejven, please add following changes on report level:

number of PASS results should include conditional passed, e.g. change "0 (1)" to "1 (1)"
use same color for conditional and regular pass (green)
add different icon for Conditional Passed on both URL and Test level (e.g. https://fontawesome.com/icons/dot-circle?style=solid )
add filter to "Show only conditionally passed" (existing "show passed urls" filter will also show conditionals)
CC @m-suchorski

tkaik · 2018-07-31T13:16:24Z

...jobs/src/test/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparatorTest.java

+  }
+
+  @Test
+  public void Comparator_hasMaskThresholdWithAcceptableDifference_withoutThreshold_expectFalse() {


Comparator_ prefix is not needed in test names - please use Tests naming convention

tkaik · 2018-07-31T13:21:19Z

report/src/main/webapp/assets/sass/_buttons.scss

-
-  svg {
+  
+    svg {


Please fix the formatting here (indentations and keep a space after : )

tkaik · 2018-07-31T14:00:09Z

documentation/src/main/wiki/LayoutComparator.md

+| Parameter | Value | Description | Mandatory |
+| --------- | ----- | ----------- | --------- |
+| `pixelThreshold` | int (bigger than 0) | The value to set the error threshold in pixels | no |
+| `percentageThreshold` | double (0 to 100) | The value to set the error threshold in percentages | no |


This documentation is missing some details - how the threshold mechanism works and what will happen on report if the difference will be below/equal/above threshold?

tkaik · 2018-07-31T14:03:15Z

Please update integration-tests with some example usages of new threshold parameters (e.g. in integration-tests/test-suite/partials/layout.xml file).

malaskowski · 2018-08-01T09:48:28Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

  private final ComparatorProperties properties;

  private final ArtifactsDAO artifactsDAO;

+  private Optional<Integer> pixelThreshold = Optional.empty();


Using Optional as a field type is not a good practice. Please use Integer instead.

malaskowski · 2018-08-01T09:49:39Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

+  }
+
+  private boolean isAcceptablePixelChange(ImageComparisonResult mask) {
+    return mask.getPixelDifferenceCount() <= this.pixelThreshold.get();


You will get NPE if pixelThreshold is absent.

malaskowski · 2018-08-01T10:17:51Z

...c/main/java/com/cognifide/aet/job/common/comparators/layout/utils/ImageComparisonResult.java

 import java.awt.image.BufferedImage;
+import java.util.Optional;


Looks like unused import.

malaskowski · 2018-08-01T10:20:27Z

documentation/src/main/wiki/LayoutComparator.md

+| --------- | ----- | ----------- | --------- |
+| `pixelThreshold` | int (bigger than 0) | The value to set the error threshold in pixels e.g if difference between photos is smaller or equal to `pixelThreshold`, the test will pass. In case of difference is bigger than `pixelThreshold`, the test will fail. | no |
+| `percentageThreshold` | double (0 to 100) | It works as `pixelThreshold` but values are in percentages | no |
+When you provide `pixelThreshold` and `percentageThreshold` test will pass only if pixel difference is smaller or equal than `pixelThreshold` and percentage difference is smaller or equal than `percentageThreshold`.


There is something wrong with the formatting, maybe put extra blank line between the table and sentence in line 17.

malaskowski · 2018-08-01T10:22:21Z

report/src/main/webapp/app/services/metadata.service.js

@@ -287,6 +291,14 @@ define(['angularAMD', 'metadataCacheService', 'metadataEndpointService'],
          }
        }

+        function conditionallyPassed() {
+          decoratedObject.total++;
+          decoratedObject.conditionallyPassed++;


We probably should also increase passed value here?

malaskowski

Please update CHANGELOG according to AET Contributing rules

malaskowski · 2018-08-01T12:19:38Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

  private final ComparatorProperties properties;

  private final ArtifactsDAO artifactsDAO;

+  private Integer pixelThreshold = null;


You don't have to assign null. This is default value of java Object.

malaskowski · 2018-08-01T12:26:07Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

+    if (params.containsKey(PIXEL_THRESHOLD_PARAM)) {
+      setPixelThreshold(Integer.valueOf(params.get(PIXEL_THRESHOLD_PARAM)));
+      ParametersValidator.checkParameter(pixelThreshold >= 0,
+          "Pixel threshold should be greater than 0");


Condition above says greater or equal than 0 :)

malaskowski · 2018-08-01T12:29:14Z

core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/layout/LayoutComparator.java

+      setPercentageThreshold(Double.valueOf(params.get(PERCENTAGE_THRESHOLD_PARAM)));
+      ParametersValidator
+          .checkRange(percentageThreshold.intValue(), 0, 100,
+              "Wrong percentage threshold value");


Maybe that message would be more verbose:

"Percentage threshold should be a decimal value between 0 and 100."

…t comparator, created new status conditionally passed

tkaik · 2018-08-02T08:54:43Z

Please also update documentation of Report webapp, e.g.:

https://github.com/Cognifide/aet/wiki/SuiteReportFeatures#filtering-test-results - add "Conditionally passed" state
https://github.com/Cognifide/aet/wiki/SuiteReportLayoutCase - describe what you can see when there is a conditional pass - e.g. images difference/mask is shown, but you can't accept is (test is passed)

…github.com/Cognifide/aet into newfeature/layout_comparator_with_treshold

tkaik · 2018-08-03T07:28:00Z

integration-tests/test-suite/partials/layout.xml

@@ -148,5 +148,117 @@
 			<url href="comparators/layout/success.jsp" />
 		</urls>
 	</test>
+
+	<test name="F-comparator-Layout-pixel-threshold">


Please update expected number of failed/passed tests in bobcat tests (integration-tests/sanity-functional)

malaskowski · 2018-08-03T12:44:54Z

report/src/main/webapp/app/services/metadata.service.js

@@ -287,6 +291,14 @@ define(['angularAMD', 'metadataCacheService', 'metadataEndpointService'],
          }
        }

+        function conditionallyPassed() {
+          decoratedObject.total++;
+          decoratedObject.conditionallyPassed++;


Do we need to increase passed value here or not?

We can increase passed. Now, it is evaluating there: https://github.com/Cognifide/aet/blob/f2dbc057f4278e8335583c8cfad43a43b0deb0ee/report/src/main/webapp/app/layout/sidepanel/sidepanel.view.html#L119
What do you think, which way will be better?
I am not 100% sure, why it works in toolbar:
https://github.com/Cognifide/aet/blob/f2dbc057f4278e8335583c8cfad43a43b0deb0ee/report/src/main/webapp/app/layout/toolbar/toolbarTop.view.html#L56
but as you can see on the screen shot, it works correctly:
https://github.com/Cognifide/aet/blob/f2dbc057f4278e8335583c8cfad43a43b0deb0ee/documentation/src/main/wiki/assets/suiteReport/layout-conditionally-passed.PNG
I think @m-suchorski know it better ;)

plutasnyy requested a review from tkaik July 25, 2018 11:28

tkaik reviewed Jul 25, 2018

View reviewed changes

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch 3 times, most recently from 7b2bc1a to ee98394 Compare July 27, 2018 05:28

tkaik reviewed Jul 27, 2018

View reviewed changes

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch from 78e40b0 to 4983ee8 Compare July 27, 2018 10:53

malaskowski suggested changes Jul 30, 2018

View reviewed changes

tkaik reviewed Jul 31, 2018

View reviewed changes

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch from a0a97af to bbe1b50 Compare August 1, 2018 07:41

malaskowski suggested changes Aug 1, 2018

View reviewed changes

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch from 121aca4 to 2312405 Compare August 1, 2018 13:01

plutasnyy added 4 commits August 1, 2018 15:15

created threshold, moved logic from image comparision result to layou…

2be75f9

…t comparator, created new status conditionally passed

created tests for threshold

0843464

created status conditionally passed in frontend

6c78eb6

created documentation for threshold parameters

6831e14

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch from 329bb24 to 6831e14 Compare August 1, 2018 13:16

plutasnyy and others added 2 commits August 1, 2018 15:24

updated changelog

b5129aa

Merge branch 'master' into newfeature/layout_comparator_with_treshold

87a4f61

malaskowski added the enhancement label Aug 1, 2018

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch from 644af33 to 87a4f61 Compare August 2, 2018 10:04

plutasnyy and others added 2 commits August 2, 2018 12:04

updated documentation

4124737

Update SuiteReportLayoutCase.md

115cac8

plutasnyy added 4 commits August 2, 2018 14:28

added sum of passed and conditionally passed in side panel

cd2da98

Merge branch 'newfeature/layout_comparator_with_treshold' of https://…

5679cd5

…github.com/Cognifide/aet into newfeature/layout_comparator_with_treshold

updated photo

47acb27

removed unused incrementation

8bd8915

tkaik approved these changes Aug 2, 2018

View reviewed changes

tkaik reviewed Aug 3, 2018

View reviewed changes

plutasnyy added 3 commits August 3, 2018 12:41

updated tests

0294312

merge

e211d4b

resolved conflicts

f2dbc05

malaskowski reviewed Aug 3, 2018

View reviewed changes

malaskowski approved these changes Aug 7, 2018

View reviewed changes

malaskowski mentioned this pull request Aug 7, 2018

Feature/exclude #303

Merged

6 tasks

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch 4 times, most recently from 343f24d to a47c9aa Compare August 9, 2018 13:48

plutasnyy added 2 commits August 10, 2018 08:49

merge

7c8977d

updated gulp

b89582b

plutasnyy force-pushed the newfeature/layout_comparator_with_treshold branch from 6847792 to f2dbc05 Compare August 10, 2018 07:15

plutasnyy and others added 5 commits August 10, 2018 10:20

updated gulp and bobcat

0a52b2a

updated doc

11fbb76

removed wrong margin

9d88d41

add information about pixel difference in raport

e2b6148

Merge branch 'master' into newfeature/layout_comparator_with_treshold

e8666d4

tkaik merged commit fecce3b into master Aug 14, 2018

tMaxx deleted the newfeature/layout_comparator_with_treshold branch August 6, 2020 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Newfeature/layout comparator with treshold #293

Newfeature/layout comparator with treshold #293

plutasnyy commented Jul 25, 2018 •

edited

Loading

tkaik Jul 25, 2018

tkaik left a comment •

edited

Loading

tkaik Jul 27, 2018

malaskowski left a comment

malaskowski Jul 30, 2018

malaskowski Jul 30, 2018

malaskowski Jul 30, 2018

tkaik commented Jul 30, 2018

tkaik Jul 31, 2018

tkaik Jul 31, 2018 •

edited

Loading

tkaik Jul 31, 2018

tkaik commented Jul 31, 2018

malaskowski Aug 1, 2018

malaskowski Aug 1, 2018

malaskowski Aug 1, 2018

malaskowski Aug 1, 2018

malaskowski Aug 1, 2018

malaskowski left a comment

malaskowski Aug 1, 2018

malaskowski Aug 1, 2018

malaskowski Aug 1, 2018

tkaik commented Aug 2, 2018

tkaik Aug 3, 2018

malaskowski Aug 3, 2018

plutasnyy Aug 3, 2018

		import java.awt.image.BufferedImage;
		import java.util.Optional;

Newfeature/layout comparator with treshold #293

Newfeature/layout comparator with treshold #293

Conversation

plutasnyy commented Jul 25, 2018 • edited Loading

Description

Motivation and Context

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

tkaik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malaskowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkaik commented Jul 30, 2018

Choose a reason for hiding this comment

tkaik Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkaik commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malaskowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkaik commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plutasnyy commented Jul 25, 2018 •

edited

Loading

tkaik left a comment •

edited

Loading

tkaik Jul 31, 2018 •

edited

Loading