testify icon indicating copy to clipboard operation
testify copied to clipboard

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

Open HaraldNordgren opened this issue 2 years ago • 10 comments

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 (https://github.com/stretchr/testify/pull/1309#discussion_r1063031396), which made sense then but now I think the time is right to remove it.

HaraldNordgren avatar May 19 '23 19:05 HaraldNordgren

Ping @boyan-soubachov @mchlp @feidtmb

HaraldNordgren avatar May 19 '23 20:05 HaraldNordgren

Ping @boyan-soubachov

This will improve the user experience for TestEqualExportedValues.

HaraldNordgren avatar Jun 18 '23 16:06 HaraldNordgren

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

HaraldNordgren avatar Jun 28 '23 09:06 HaraldNordgren

@dolmen Done, I have update the code! 🤗

HaraldNordgren avatar Jul 29 '23 19:07 HaraldNordgren

👀

jufemaiz avatar Aug 10 '23 03:08 jufemaiz

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

HaraldNordgren avatar Aug 11 '23 08:08 HaraldNordgren

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

HaraldNordgren avatar Sep 07 '23 08:09 HaraldNordgren

@dolmen Done!

HaraldNordgren avatar Oct 16 '23 13:10 HaraldNordgren

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

HaraldNordgren avatar Jan 11 '24 19:01 HaraldNordgren

Ping @dolmen

HaraldNordgren avatar Feb 10 '24 10:02 HaraldNordgren

I think #1517 fully implemented this.

brackendawson avatar Feb 23 '24 13:02 brackendawson