8000 assert: add JSONEqBytes by GCrispino · Pull Request #1513 · stretchr/testify · GitHub
[go: up one dir, main page]
Skip to content

assert: add JSONEqBytes #1513

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

assert: add JSONEqBytes #1513

wants to merge 7 commits into from
+149 −7

Conversation

GCrispino
Copy link
@GCrispino GCrispino commented Dec 2, 2023 •
edited
Loading

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

suqin-haha
suqin-haha reviewed Oct 15, 2024
View reviewed changes
brackendawson
brackendawson previously approved these changes Oct 29, 2024
View reviewed changes
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 brackendawson requested a review from dolmen October 29, 2024 13:57
@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 👍🏼

brackendawson
brackendawson previously approved these changes Oct 29, 2024
View reviewed changes
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.

@dolmen dolmen changed the title Add function for supporting JSON equality checks using byte slices assert: add JSONEqBytes May 28, 2025
@fredbi
Copy link
fredbi commented Sep 27, 2025

Note to self: push merge conflict resolve

this is one nice addition in a million. The string stuff should never have existed in the first place but let’s be pragmatic and adopt one api that we test writers may use idiomatically, ie without type conversion

ccoVeille
ccoVeille previously approved these changes Sep 28, 2025
View reviewed changes
@GCrispino GCrispino dismissed stale reviews from ccoVeille and brackendawson via df6d47d September 28, 2025 15:16
@GCrispino
Copy link
Author

Hi @fredbi !

I have gone ahead and solved the merge conflicts myself. I believe it's all good, as tests are passing.
I've ran go generate ./... and go fmt ./... again but it appears that no changes happened as a result of this.

fredbi
fredbi approved these changes Sep 29, 2025
View reviewed changes
Copy link
@fredbi fredbi left a comment

Choose a reason for hiding this comment

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

Great job

fredbi added a commit to go-openapi/testify that referenced this pull request Oct 2, 2025
@fredbi
merged stretchr#1513
d425f21 
Resolved merge conflicts with the go-openapi fork, rebase and ensured
all tests pass.

Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
fredbi
fredbi reviewed Oct 2, 2025
View reviewed changes
assert/cpu.out Outdated
Copy link

Choose a reason for hiding this comment

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

We probably don't need this cpu.out file.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I hadn't realized I had added that file. Pushed a commit removing it 👍🏼

@fredbi
Copy link
fredbi commented Oct 2, 2025

@GCrispino merged in our fork go-openapi/testify

dolmen
dolmen requested changes Oct 5, 2025
View reviewed changes
Copy link
Collaborator
@dolmen dolmen left a comment

Choose a reason for hiding this comment

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

We must have special messages for the []byte(nil) cases.

@GCrispino
Copy link
Author

@ccoVeille

We must have special messages for the []byte(nil) cases.

Currently if expected == []bytes(nil), the "Expected value ('') is not valid json.\nJSON parsing error: 'unexpected end of JSON input'" message will be used, and in the case where actual == []bytes(nil), the "Input ('') needs to be valid json.\nJSON parsing error: 'unexpected end of JSON input'" one will be used.

I've added a commit that will use the messages of "Expected value is empty" and "Actual value is empty", respectively, in case that the provided byte slices 9E88 are either nil or empty, if you believe we need special messages in these cases, apart from the current ones complaining that the input is not proper JSON.
Otherwise, I can just revert this commit.

Can you please have a look at it?

@GCrispino GCrispino requested a review from dolmen October 6, 2025 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brackendawson brackendawson brackendawson left review comments

@ccoVeille ccoVeille ccoVeille left review comments

@dolmen dolmen Awaiting requested review from dolmen

+2 more reviewers

@fredbi fredbi fredbi approved these changes

@suqin-haha suqin-haha suqin-haha left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement pkg-assert Change related to package testify/assert
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@GCrispino @hendrywiranto @brackendawson @fredbi @dolmen @ccoVeille @suqin-haha
0