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 function for supporting JSON equality checks using byte slices #1513

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GCrispino
Copy link

@GCrispino GCrispino commented Dec 2, 2023

Summary

This PR adds a new function into the assert package, JSONEqBytes, that does the same as JSONEq, but by receiving []byte values as parameter instead of string ones.

Changes

assert/assertions.go:
The implementation of JSONEqBytes is pretty much what was in JSONEq, but having it receive []byte values and then not needing to convert these values to []byte when calling json.Unmarshal inside it.

Then, the implementation of JSONEq was changed such that it just calls JSONEqBytes by converting its original arguments to []byte values (which it already did when calling json.Unmarshal)

assert/assertions_test.go:
Tests for JSONEqBytes function were replicated from the tests from JSONEq.

Motivation

It is common in Go to store JSON text in []byte variables instead of string values, so having a function that allows to receive these values when doing JSON-equality checks without having to cast them to string can be useful.

Maybe not as important, it would also avoid unnecessary []byte-to-string conversions, which could add up when dealing with big JSON payloads.

Example of usage:

package main

import (
    "fmt"
    "testing"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)


func TestJSONFiles(t *testing.T) {
    actualFile := "/path/to/file.json"
    byteValue, err := ioutil.ReadAll(jsonFile)
    require.NoError(t, err)

    expectedContentsFile := "/path/to/expected.json"
    byteValue, err = ioutil.ReadAll(jsonFile)
    require.NoError(t, err)

    assert.JSONEqBytes(t, expected, byteValue)
}

@dolmen dolmen added enhancement pkg-assert Change related to package testify/assert labels Dec 7, 2023
@hendrywiranto
Copy link
Contributor

Hi !
this one is great but I think currently maintainers are more focused in fixing bugs rather than expanding the API interface.
Let's wait their response

brackendawson
brackendawson previously approved these changes Oct 29, 2024
Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

This is a good implementation.

I have questions about whether JSONEq should have ever been added. My preference would always be your API should yield stably formatted output. If JSONEq should have been added then it should have always accepted a []byte rather than a string. I'm not going to merge this without input from other maintainers, any thoughts @dolmen?

It would be useful to see if there is a real world example where the string is prohibitively expensive.

@brackendawson
Copy link
Collaborator

@GCrispino can you re-base to run the latest version of the tests.

@brackendawson
Copy link
Collaborator

I note that YAMLEq also accepts a string, that's an even more problematic assertion.

@GCrispino
Copy link
Author

@brackendawson rebased my branch against master and re ran go generate and go fmt 👍🏼

Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

Still approve, as per my previous comments.

@GCrispino
Copy link
Author

@brackendawson thanks! Is there something else I should do, or are you only expecting review from another maintainer?

@brackendawson
Copy link
Collaborator

@brackendawson thanks! Is there something else I should do, or are you only expecting review from another maintainer?

Nothing else needed from you at this time, just wait for another maintainer to opine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pkg-assert Change related to package testify/assert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants