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

Feature/fix change set applier #726

Merged
merged 3 commits into from
May 11, 2020
Merged

Conversation

diba1013
Copy link
Contributor

Before submission, please check that ...

  • the code follows the Clean Code guidelines.
  • the necessary tests are either created or updated.
  • all status checks (Travis CI, SonarCloud, etc.) pass.
  • your commit history is cleaned up.
  • you updated the changelog.
  • you updated necessary documentation within retest/docs.

Description

TLDR; Fix that "equal" changes are not accepted together. This brings back the 1-click maintenance.

Bug

This especially noticeable, when testing the same website multiple times (different browsers, screen resolutions, etc.). Because the same site is tested multiple times, the same differences will occur within the different tests. The example below shows a test performed with two different screen resolutions, executed twice to account for the Golden Master creation.

Suite 'TemplateTest' has 6 difference(s) in 4 test(s):
	Test 'large' has 1 difference(s) in 1 state(s):
	page resulted in:
		No Golden Master found. First time test was run? Created new Golden Master, so don't forget to commit...
	Test 'small' has 1 difference(s) in 1 state(s):
	page resulted in:
		No Golden Master found. First time test was run? Created new Golden Master, so don't forget to commit...
	Test 'large' has 3 difference(s) in 1 state(s):
	page resulted in:
		div (div) at 'html[1]/body[1]/div[1]':
			outline:
			  expected="java.awt.Rectangle[x=0,y=0,width=300,height=200]",
			    actual="java.awt.Rectangle[x=0,y=0,width=297,height=198]"
			align-self:
			  expected="",
			    actual="flex-start"
		span (span) at 'html[1]/body[1]/span[1]':
			was deleted
	Test 'small' has 3 difference(s) in 1 state(s):
	page resulted in:
		div (div) at 'html[1]/body[1]/div[1]':
			outline:
			  expected="java.awt.Rectangle[x=0,y=0,width=200,height=100]",
			    actual="java.awt.Rectangle[x=0,y=0,width=187,height=97]"
			align-self:
			  expected="",
			    actual="flex-start"
		span (span) at 'html[1]/body[1]/span[1]':
			was deleted

These changes are present twice and thus are expected to be accepted with a single method call:

div (div) at 'html[1]/body[1]/div[1]':
    align-self:
        expected="",
        actual="flex-start"
span (span) at 'html[1]/body[1]/span[1]':
    was deleted

However, when accepting this within review, each change has to be accepted individually to be counted for the model and accepted into the Golden Master.

Note that review (develop) already uses a similar technique to identify similar changes, which are not propagated properly to the GlobalChangeSetApplier because of this bug.

Workaround

Instead of using the hashCode, use the reduced identifier methods of both IdentifyingAttributes and AttributeDifferences to only take into account important attributes. The former therefore will only take the path, type and suffix attribute, while the latter will only take actual and expected attributes. We might want to change this in the future, but it seems to work for now.

I would consider this more of a workaround, since per definition all IdentifyingAttributes should only hold stable attributes. Therefore I would consider implementing retest/recheck-web#200, but that might be a major breaking change.

Test

To have a test as close to the actual behavior, I introduced an integration test. Since manually putting together a report is cumbersome and we do not have a migration strategy for file based reports to load from, the integration test performs a full recheck lifecycle using the example above to create a report.

Since we are collecting the report within a separate class SuiteAggregator it has to be reset before the test begins to prevent other tests to contribute differences to the report. I am aware that this is not a clean solution as it relies on side effects due to the usage of static singletons. However, alternative solutions would be either introducing a new constructor that takes such a SuiteAggregator or exposing the current test report from RecheckImpl which both then would be public API. If the current solution does not suit you, I would be in favor of exposing the current report as that would be the lowest impact.

Due to the broken down identifier methods, the contents of the various lookup methods cannot be compared anymore. Therefore I decided to only use simple hasSize-checks to test for the validity of the generated lookup tables, as all other possible checks (e.g. accepting changes) seemed to be very cumbersome to properly check.

This has been verified manually that it works with review

diba1013 added 3 commits May 11, 2020 12:17
since the TestReport is shared between all tests within the VM
which emulates the following use case:
1. Test the same website with two different resolutions.
2. Create initial Golden Master.
3. Compare with slight resolution differences, css differences and removed element.

Since the same elements are compared (only different resolution and thus different outline), the GlobalChangeSetApplier should still merge the same elements.
since the outline is not a stable attribute and thus falsifies the hashCode of IdentifyingAttributes.

The same goes for the AttributeDifference, which may or may not contain warnings.
@diba1013 diba1013 added the bug Something isn't working label May 11, 2020
@diba1013 diba1013 requested a review from martin-v May 11, 2020 11:41
@diba1013 diba1013 self-assigned this May 11, 2020
@cla-bot cla-bot bot added the cla-signed label May 11, 2020
@mergify mergify bot merged commit b15d05f into master May 11, 2020
@mergify mergify bot deleted the feature/fix-change-set-applier branch May 11, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed
Development

Successfully merging this pull request may close these issues.

2 participants