General thoughts for discussion on the framework

Thank you for this excellent library.

I was using an in-house utility for snapshot testing which was pretty much identical to this but since this one is open source, I'm moving to this one. There are a few things that I do miss from my old framework though so I wanted to have a general discussion about whether or not they can be incorporated in this library. These are not 'issues' per se so I'm listing them here. I can create separate issues for these if required or once we make a decision on them.

Json comparison is currently being done using text equals and the report generated using diffutils, which sometimes highlights differences in newlines and whitespaces. There may be cases where this might be preferred but I was wondering if it's possible to have a plug-in comparator and report generator. This gives users the option to use something like https://github.com/skyscreamer/JSONassert for json comparison or regular junit/assertj for assertions (this is nice because IDEs can then show a diff natively)
The default behaviour when a snapshot doesn't exist should probably not be to create one automatically. This may be a problem in CI/CD environments because if a test class is refactored and moved to a different package and if the snapshot is not moved, the test will always keep passing on CI/CD even if there are breaking changes.
Would also be nice to create the snapshot file lazily if it doesn't exist. We use junit jupiter's meta-annotation support to create a single test annotation which configures sane defaults for all tests and that includes SnapshotExtension. This single annotation is then used for all tests instead of each test having a huge list of annotations (especially for spring based tests). This works now but it ends up creating a bunch of empty snapshot files since all tests now use the snapshot extension but don't actually use the 'expect' call.

Let me know what you think about these. I'll be happy to submit a PR for the changes we agree upon.

All these seem feasible to me.

I too was thinking of making the report pluggable so you can supply your ownSnapshotReporter be that CLI, HTML etc. Probably an array so it would produce all of them.

Potentially the *.snap.debug feature could be a type of report too.

Regarding JSONassert - we would need to refactor so that the Verifier is aware of the serializer that produced it. Currently it isn't. In a given file you could have snapshots that are created from .toString(), Jackson JSON serialization or any other custom serializer such as an XMLSerializer.

Kind of what I'm getting at is to support this that snapshot perhaps needs to be aware of the comparitor. This may need some through and might need to be also serialized into the snapshot itself. expect(blah).comparitor(MyComparitor.class).toMatchSnapshot() might well do also.

I think we can include a few very basic reporters in core and others can be their own separate projects. java-snapshot-testing-reporter-jsonassert for example.

Agree, it shouldn't create snapshots on CI.

So there is a conflict of concerns here

We want it to be restrictive on CI
We want it to be permissive locally or else the user will need to constantly pass some sort of property.

I wonder if there is a way we can achieve this without inhibiting the end user too much while they work locally. Perhaps a SNAPSHOT_DEV_MACHINE environment variable or something. At least that is a once of operation for the user.

Agree, sounds doable.

And yeah - Happy to review PR's.

Thanks

Pluggable reporters sounds great. A chain of reporters sounds great too 👍

we would need to refactor so that the Verifier is aware of the serializer that produced it

Do we need the verifier to be aware of the entire serializer or would it be enough if it's aware of the 'type' of serializer. Would it make sense to have an enum of serializer types and pass just the enum value to the verifier ? That way we don't have to store the className or any reference in the snapshot and just rely on the existing expect().json() or expect().text() call to make the distinction of what verifier to use. What do you think ?

Agree with your view that we need to make it permissive/restrictive depending on the environment. I think instead of checking if we're on a dev machine and making it permissive, we can do the reverse of checking if we're on a CI and make it restrictive. I think all major CI environments support the environment variable 'CI' which will be true when executing on a CI environment. I normally lean towards making things restrictive by default and permissive as a fallback but in this case I feel it's safe to rely on the 'CI' variable. We can additionally consider supporting a CiDetectionStrategy whose default implementation can just look for the 'CI' variable (and edge cases if we find any). We can then expose a method called getCiDetectionStrategy on the SnapshotConfig object which users can override if they want to. What do you think ?
Great.

I'll work on the PR this weekend and have something ready for you to look at sometime next week.

We can't use an enum because we want it to be pluggable so people can supply their own custom serializers. I think I said the wrong thing too. It needs to be aware of the comparitor not the serializer necessarily. The SnapshotComparitor can be defined on the expect() call so I'm hoping we can retrieve it from there without serializing anything to the snapshot.

expect(blath).json().comparitor(JSONAssertSnapshotComparitor.class).toMatchSnapshot()

.json() can also set the best comparitor for JSON by default.

Otherwise - we can consider header meta information in the snapshot itself:

au.com.example.company.UserEndpointTest.shouldReturnCustomerData={
  "serializer": "au.com.origin.snapshots.serializers.JacksonSerializer",
  "comparitor": "au.com.origin.snapshots.comparitors.JSONAssertSnapshotComparitor"
},[
  {
    "id": "1",
    "firstName": "John",
    "lastName": "Smith",
    "age": 34
  }
]

Yeah I like this suggestion. Let's add isCI() method to SnapshotConfig so people can supply their own. The default can be the CI environment variable as you suggested. Also can we log a message for each scenario

Something like

log.warn("We detected you are running on a developer machine - if this is incorrect please override the "isCI()" method in SnapshotConfig")

log.info("We detected you are running on a CI Server - if this is incorrect please override the "isCI()" method in SnapshotConfig")

BTW I'm now realising there is a difference between the Comparitor an the Reporter let's not get the two confused.

Comparitor will potentially break the snapshot (1 only per snapshot)
Reporter will output reports such as diffs (potentially many reports for a single snapshot)

Ah I see what you're getting at. I agree and I think we're on the same page now. Will let you know once I have some updates on the PR.

#46 Submitted a draft pull request for discussion. Let me know if I'm off on anything. I'll address them on subsequent commits.

Have merged item 3 (lazy snapshots).

@juja0

Thanks for your work on this @juja0. Will do a release shortly.

Released 2.1.0

Great. Thanks mate.

Navigation Menu

Repository navigation

General thoughts for discussion on the framework #44

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

General thoughts for discussion on the framework #44

Description

Activity

jackmatt2 commented on Nov 9, 2020

juja0 commented on Nov 9, 2020

jackmatt2 commented on Nov 9, 2020

jackmatt2 commented on Nov 9, 2020

juja0 commented on Nov 12, 2020

juja0 commented on Nov 12, 2020

jackmatt2 commented on Nov 20, 2020

jackmatt2 commented on Nov 23, 2020

jackmatt2 commented on Nov 23, 2020

juja0 commented on Nov 24, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions