Repository navigation
Conversation
jackmatt2
left a comment
jackmatt2
left a comment
There was a problem hiding this comment.
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. |
| - name: Grant execute permission for gradlew | ||
| run: chmod +x gradlew | ||
| - name: Build with Gradle | ||
| run: ./gradlew build |
There was a problem hiding this comment.
Not Sure why my changes are showing up in your PR. You might need to do a git reset --soft origin/master or something.
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
*/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
IDE might not work if we shadow it actually - if it's looking for specific packages etc.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh yeah I didn't think about that. Will test it out anyway.
There was a problem hiding this comment.
Yeah I meant relocate using shadowJar like here: https://github.com/origin-energy/java-snapshot-testing/blob/master/build.gradle#L22-L27
|
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. Also intellij couldn't show proper diff after relocation so I've skipped it. |
This branch was previously deployed
2 participants
Initial work-in-progress commit to enable support for custom snapshot comparators and reporters.
Not to be merged yet. Open for discussion.
Changes
SnapshotComparatorinterface which can compare snapshots and actual objects and provide aCompareResultSnapshotDiffReporterinterface which supports two methods1. 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.
SnapshotConfigandSnapshot.Minor changes
Snapshot.javaSnapshot.javaimmutable so that common customizations (like adding serializers and comparators) can be extracted as variables and re-used.zonedDateTimefield 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.