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

Add EMIT_TEST_OPERATIONS diff flag (supports #91) #92

Merged
merged 7 commits into from
Feb 10, 2019

Conversation

holograph
Copy link
Collaborator

No description provided.

JsonNode testNode = mapper.readTree("{\"op\":\"test\",\"path\":\"/key\",\"value\":\"original\"}");
assertEquals(2, diff.size());
assertEquals(testNode, diff.iterator().next());
}
Copy link

Choose a reason for hiding this comment

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

It looks very good! Thank you!

But I could also suggest extends tests not only check test node present on patch, but it also correctly handled on apply operation by complimentary add apply test for each current test like:

    @Test
    public void testNodeEmittedBeforeReplaceOperation() throws IOException {
        JsonNode source = mapper.readTree("{\"key\":\"original\"}");
        JsonNode target = mapper.readTree("{\"key\":\"replaced\"}");

        JsonNode diff = JsonDiff.asJson(source, target, flags);

        assertEquals(2, diff.size());
        Iterator<JsonNode> iterator = diff.iterator();
        assertEquals(mapper.readTree("{\"op\":\"test\",\"path\":\"/key\",\"value\":\"original\"}"), iterator.next());
        assertEquals(mapper.readTree("{\"op\":\"replace\",\"path\":\"/key\",\"value\":\"replaced\"}"), iterator.next());
    }

    @Rule
    public ExpectedException exceptionRule = ExpectedException.none();

    @Test
    public void testApplyWithTestOnReplaceOperation() throws IOException {
        exceptionRule.expect(JsonPatchApplicationException.class);
        exceptionRule.expectMessage("[TEST Operation] value mismatch");

        JsonNode source = mapper.readTree("{\"key\":\"original\"}");
        JsonNode target = mapper.readTree("{\"key\":\"replaced\"}");

        JsonNode diff = JsonDiff.asJson(source, target, flags);

        source = mapper.readTree("{\"key\":\"changed\"}"); // Change source
        JsonPatch.apply(diff, source);
    }

It will be also very good to see context at what it failed, as I suggest in issue. So instead of just message [TEST Operation] value mismatch provide path and values, both expected and present. What are you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO that's covered by separate tests (the ones dealing with TEST ops). As for the error message, I'd treat that as a separate change request, though I certainly agree that more information is better...

@vishwakarma
Copy link
Member

The changes looks good, i will merge and release the updated version.

@vishwakarma vishwakarma merged commit fe7cc58 into flipkart-incubator:master Feb 10, 2019
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