Skip to content

✨ Add Snapshot.ignore(String... jsonPaths) method #10

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ftes
Copy link

@ftes ftes commented Dec 17, 2018

This superseeds #7.

Advantages:

  • Ignore by jsonPath (more flexible)
  • Ignore fields in any kind of object, not just maps (by first transforming to a GSON tree)

@ftes
Copy link
Author

ftes commented Dec 17, 2018

Hey @andrebonna, sorry for taking so long to get back to you. This seems like a better solution than my original PR so hopefully it was for the better ;).

@@ -69,11 +81,38 @@ private String getRawSnapshot(Collection<String> rawSnapshots) {
return null;
}

private Object ignorePaths(Object input) {
DocumentContext context = JsonPath.using(new GsonJsonProvider()).parse(gsonForIgnoringPaths.toJsonTree(input));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considered alternative: Using a GSON ExclusionStrategy. However, such a strategy has no notion of the path to a field. The methods in the interface only allow finding the type of the containing class and the name of each field.

Copy link
Member

@andrebonna andrebonna Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing JSON parser lib to Jackson. We changed default lib from GSON to Jackson due to issue #9

@andrebonna
Copy link
Member

There are some conflicting files as well!

@ftes ftes force-pushed the feature/ignore-json-path branch from 1fc6c9c to f020f04 Compare February 13, 2019 08:57
@ftes
Copy link
Author

ftes commented Feb 13, 2019

I rebased and updated the MR. Switched to Jackson.

.stream()
.map(delta -> delta.toString() + "\n")
.reduce(String::concat)
.get();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid formatting-only changes

<dependency>
<groupId>com.jayway.jsonpath</groupId>
<artifactId>json-path</artifactId>
<version>2.4.0</version>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to do this without an extra dependency?
Jackson already uses JsonPointers, maybe that could be used to specify ignored elements as well?

return Arrays.asList(current).stream()
.map(this::ignorePaths)
.collect(Collectors.toList())
.toArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream#toArray(Object[]::new)?

public void shouldMatchSnapshotAndIgnoreSeveralNestedFieldsMethod() {
FakeObject grandChild = FakeObject.builder().id("grandChild7").value(7).build();
FakeObject child = FakeObject.builder().id("child7").value(7).fakeObject(grandChild).build();
expect(FakeObject.builder().id("anyId7").value(7).name("anyName7").fakeObject(child).build())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd extract creation of test data into a variable, maybe something like:

FakeObject testObject = FakeObject.builder()
    .id("anyId7").value(7)
    .name("anyName7").fakeObject(FakeObject.builder()
            .id("child7").value(7)
            .fakeObject(FakeObject.builder()
                    .id("grandChild7").value(7)
                    .build()
            ).build()
    ).build().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants