Initial work-in-progress commit to enable support for custom snapshot… #46

juja0 · 2020-11-12T12:57:13Z

Initial work-in-progress commit to enable support for custom snapshot comparators and reporters.

Not to be merged yet. Open for discussion.

Changes

Added a SnapshotComparator interface which can compare snapshots and actual objects and provide a CompareResult
Added a SnapshotDiffReporter interface which supports two methods
1. A method which takes a comparator as input and returns a boolean indicating if the comparator is supported by this reporter
2. A method which takes a comparison result and produces a report.
Wired up comparators and reporters into SnapshotConfig and Snapshot.
Added default 'plain text' versions of comparator and reporter.

Minor changes

Bumped up the version of lombok so that I could use the @with annotation on specific fields in Snapshot.java
Made Snapshot.java immutable so that common customizations (like adding serializers and comparators) can be extracted as variables and re-used.
Changed the month in the zonedDateTime field in a couple of tests and their corresponding snapshots because tests were failing on a fresh clone of the repository (potential daylight savings related issue ?). Tried several variations of -Duser.timezone system property but didn't work. Didn't investigate much but changing the month seemed to fixed it so I've done that for now. Hoping that this change will make the test pass regardless of where in the world the tests are run but will investigate further if that's not the case.

jackmatt2

Hey looking good so far - have left a few comments.

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/comparators/CompareResult.java

...pshot-testing-core/src/main/java/au/com/origin/snapshots/reporters/SnapshotDiffReporter.java

...esting-core/src/main/java/au/com/origin/snapshots/comparators/PlainTextEqualsComparator.java

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/Snapshot.java

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/comparators/CompareUtils.java

juja0 · 2020-11-16T15:45:32Z

Hey looking good so far - have left a few comments.

Haven't forgotten about this. It's holiday time here. Will get to this in a few days.

jackmatt2 · 2020-11-22T05:52:39Z

.github/workflows/build.yml

+    - name: Grant execute permission for gradlew
+      run: chmod +x gradlew
+    - name: Build with Gradle
+      run: ./gradlew build


Not Sure why my changes are showing up in your PR. You might need to do a git reset --soft origin/master or something.

Yeah I tried a few options but ended up messing things up more. I'll try git reset again. Worst case, I'll close this PR and create a new one.

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/SnapshotConfig.java

...pshot-testing-core/src/main/java/au/com/origin/snapshots/comparators/SnapshotComparator.java

jackmatt2 · 2020-11-22T06:07:32Z

...ting-core/src/main/java/au/com/origin/snapshots/reporters/PlainTextSnapshotDiffReporter.java

+        Patch<String> patch = DiffUtils.diff(
+                Arrays.asList(rawSnapshot.trim().split("\n")),
+                Arrays.asList(currentObject.trim().split("\n")));
+        throw new SnapshotMatchException("Error on: \n" + currentObject.trim() + "\n\n" + getDiffString(patch));


If we have multiple reporters - I don't think the extra ones will all run if this exception is thrown. We might need to rethink how and where the exception is throws to allow all reporters to do their work. Perhaps they register themselves as SUCCESS/FAILED and we throw after all have had a change to process the request.

Yeah this felt a bit iffy to me too.

I considered the aggregated reporting method as you suggested but decided to go with what I've done in favour of failing fast and also showing only a limited and concise report and to be able to allow reporters to choose to use their assertion library of choice (assertj, jsonassert for example) so that IDEs can also use that to provide some meaningful diff view (IMO typically much more advanced and interactive than any reporters that we can create)

For what it's worth I added this comment block in the getReporters method in SnapshotConfig to clarify this behaviour. What's your thought on this ? Do you think we should still do the aggregated report at the end ?

/** * Optional * Override to supply your own custom reporter functions * Reporters will run in the same sequence as provided. * Reporters can choose to throw exceptions in which case subsequent reporters will be skipped. * Typically one such reporter can be included at the end which can use common assertion libraries * like assertj or junit assertions to 'fail' the test. This is useful since IDEs like intellij can * then show a diff using their native diff tools. * <p> * In the absence of any 'throwing' reporters in the list, a default failure exception will be thrown * to make sure tests fail. * * @return custom reporter functions */

I think ideally we should throw once all reporters get a chance to process the request. Maybe each reporter run gets wrapped in a try/catch, we add the exception to a list - then we re-throw the lot at the end if the errorList is not empty. This may have unintended consequences on the IDE integration you spoke about though.

If you have already taken a look at it and it is problematic - we can look at it later in a separate ticket.

I was experimenting earlier today by enhancing SnapshotMatchException to accept a list of Throwables and it wasn't working properly. I was appending all messages separated by System.lineSeparator() but intellij couldn't detect that these were separate errors.

I was trying to see how opentest4j handles this and found something interesting. It seems to have a MultipleFailuresError which takes a list of Throwables and when thrown, intellij is able to identify individual errors and shows a separate diff view for each error 😃 The reason it seems to be working is because it extends AssertionError but SnapshotMatchException extends RuntimeException.

I think it's reasonable to have SnapshotMatchException extend AssertionError and do something similar to what MultipleFailuresError does so that we can get aggregated reporting. I'll try this out and let you know how it goes.

If this works out, I'll also rename SnapshotMatchException to SnapshotMatchError.

It only seems to work when I explicitly extend MultipleFailuresError and not AssertionError.

Related discussion : lukas-krecan/JsonUnit#146 (this library BTW looks promising as a potential reporter)

Extending MultipleFailures from opentest4j doesn't seem ideal (would have preferred if AssertionError worked somehow) but I still think it adds value. Shall I go ahead and make SnapshotMatchException extend MultipleFailuresError ?

If that's the case we can add it. Perhaps we should shadow it so it doesn't conflict with other versions like we do for jackson?

IDE might not work if we shadow it actually - if it's looking for specific packages etc.

Ah that makes sense. Will do.

Just to be clear, I'm guessing you meant relocate and not shadow. Shadowing just makes things even worse 😛 . Will relocate the package.

Oh yeah I didn't think about that. Will test it out anyway.

Yeah I meant relocate using shadowJar like here: https://github.com/origin-energy/java-snapshot-testing/blob/master/build.gradle#L22-L27

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/Snapshot.java

juja0 · 2020-11-22T13:00:03Z

@jackmatt2

Soft reset did not work for some reason. Tried a hard reset followed by a force push and the PR closed automatically so I've raised a new one.

#48

Also intellij couldn't show proper diff after relocation so I've skipped it.

juja0 mentioned this pull request Nov 12, 2020

General thoughts for discussion on the framework #44

Closed

jackmatt2 requested changes Nov 12, 2020

View reviewed changes

jackmatt2 reviewed Nov 22, 2020

View reviewed changes

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/Snapshot.java Outdated Show resolved Hide resolved

java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/Snapshot.java Outdated Show resolved Hide resolved

juja0 closed this Nov 22, 2020

juja0 force-pushed the master branch from 8d3d4d7 to 2597196 Compare November 22, 2020 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work-in-progress commit to enable support for custom snapshot… #46

Initial work-in-progress commit to enable support for custom snapshot… #46

juja0 commented Nov 12, 2020

jackmatt2 left a comment

juja0 commented Nov 16, 2020

jackmatt2 Nov 22, 2020

juja0 Nov 22, 2020

jackmatt2 Nov 22, 2020 •

edited

Loading

juja0 Nov 22, 2020

jackmatt2 Nov 22, 2020

juja0 Nov 22, 2020

juja0 Nov 22, 2020

jackmatt2 Nov 22, 2020 •

edited

Loading

jackmatt2 Nov 22, 2020

juja0 Nov 22, 2020

juja0 Nov 22, 2020

jackmatt2 Nov 22, 2020 •

edited

Loading

juja0 commented Nov 22, 2020

Initial work-in-progress commit to enable support for custom snapshot… #46

Initial work-in-progress commit to enable support for custom snapshot… #46

Conversation

juja0 commented Nov 12, 2020

jackmatt2 left a comment

Choose a reason for hiding this comment

juja0 commented Nov 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackmatt2 Nov 22, 2020 • edited Loading

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

jackmatt2 Nov 22, 2020 • edited Loading

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

jackmatt2 Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

juja0 commented Nov 22, 2020

jackmatt2 Nov 22, 2020 •

edited

Loading

jackmatt2 Nov 22, 2020 •

edited

Loading

jackmatt2 Nov 22, 2020 •

edited

Loading