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

assert: Remove the struct type constraint on 'TestEqualExportedValues' to support pointer comparisons #1384

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented May 19, 2023

Pointer comparisons are supported already, just as long as the are not the top-level element. Logically, if two item or sub-items have different unexported field, then they cannot have the same underlying address. With this change, the top-level will be treated no differently from the rest of the struct fields which also makes the logic simpler.

The normal use case here is when asserting any response data that has a pointer type. As the code currently stands on master, you have get the underlying values and compare those instead, otherwise it fails:

require.NotNil(t, actual)
assert.EqualExportedValues(t, *expected, *actual)

I'm adding a test case that would have failed with the old logic.

I am also getting rid of the duplicated ObjectsExportedFieldsAreEqual. Since each assert function return a boolean result, we can achieve exactly the same without the extra function. This was discussed when adding the original code (#1309 (comment)), which made sense then but now I think the time is right to remove it.

@HaraldNordgren HaraldNordgren changed the title Support pointer on top-level for 'TestEqualExportedValues' & Get rid of duplicate 'ObjectsExportedFieldsAreEqual' Remove the "struct" constraint on 'TestEqualExportedValues' to support pointer struct comparisons May 19, 2023
@HaraldNordgren HaraldNordgren changed the title Remove the "struct" constraint on 'TestEqualExportedValues' to support pointer struct comparisons Remove the struct constraint on 'TestEqualExportedValues' to support pointer struct comparisons May 19, 2023
@HaraldNordgren HaraldNordgren changed the title Remove the struct constraint on 'TestEqualExportedValues' to support pointer struct comparisons Remove the struct constraint on 'TestEqualExportedValues' to support pointer comparisons May 19, 2023
@HaraldNordgren HaraldNordgren changed the title Remove the struct constraint on 'TestEqualExportedValues' to support pointer comparisons Remove the struct type constraint on 'TestEqualExportedValues' to support pointer comparisons May 19, 2023
@HaraldNordgren
Copy link
Contributor Author

Ping @boyan-soubachov @mchlp @feidtmb

@HaraldNordgren HaraldNordgren force-pushed the ObjectsExportedFieldsAreEqual_refactor branch from 334a6f4 to 141f1f6 Compare May 19, 2023 20:40
@HaraldNordgren
Copy link
Contributor Author

Ping @boyan-soubachov

This will improve the user experience for TestEqualExportedValues.

@HaraldNordgren
Copy link
Contributor Author

Ping @boyan-soubachov

Currently the library is usable, but it gives this error when comparing Protobuf objects because you have to access the underlying pointer, otherwise the comparison is false:

call of assert.EqualExportedValues copies lock value: github.com/dietdoctor/genproto/go/my-project/v1.ListItemsResponse contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutexcopylocks

@dolmen dolmen added enhancement pkg-assert Change related to package testify/assert labels Jul 5, 2023
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.

I am also getting rid of the duplicated ObjectsExportedFieldsAreEqual

While this looks sane, this is not that easy. We have to preserve the API surface. Please keep the function and mark it as // Deprecated:.

assert/assertions_test.go Outdated Show resolved Hide resolved
@HaraldNordgren HaraldNordgren force-pushed the ObjectsExportedFieldsAreEqual_refactor branch 2 times, most recently from 382bc75 to fc8b99a Compare July 29, 2023 19:43
@HaraldNordgren
Copy link
Contributor Author

@dolmen Done, I have update the code! 🤗

@jufemaiz
Copy link

👀

@HaraldNordgren
Copy link
Contributor Author

@dolmen Hi, did you have time to take another look at this?

@HaraldNordgren
Copy link
Contributor Author

Hi @dolmen @boyan-soubachov, did you have a chance to take a look at this?

Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

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.

Please open a separate MR to deprecate ObjectsExportedFieldsAreEqual, and move that change there.

Also cleanup commits, and rebase (do not merge master into your branch!).

…of duplicate 'ObjectsExportedFieldsAreEqual'
@HaraldNordgren HaraldNordgren force-pushed the ObjectsExportedFieldsAreEqual_refactor branch from 7c6d829 to f952a8a Compare October 16, 2023 13:52
@HaraldNordgren HaraldNordgren changed the title Remove the struct type constraint on 'TestEqualExportedValues' to support pointer comparisons assert: Remove the struct type constraint on 'TestEqualExportedValues' to support pointer comparisons Oct 16, 2023
@HaraldNordgren
Copy link
Contributor Author

@dolmen Done!

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.

Ready for v1.9.0: this is a behaviour change that require a minor release.

@dolmen dolmen added this to the v1.9.0 milestone Oct 16, 2023
@HaraldNordgren
Copy link
Contributor Author

@dolmen Any update on when 1.9.0 is expected? Let's merge this in the meantime?

@HaraldNordgren
Copy link
Contributor Author

Ping @dolmen

@brackendawson
Copy link
Collaborator

I think #1517 fully implemented this.

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

Successfully merging this pull request may close these issues.

6 participants