fix: NotSame was returning true for non-pointers #1738

ccoVeille · 2025-05-09T01:08:06Z

Summary

The code was different than the comment, and the code behavior was different from other asserters

Changes

fix: NotSame was returning true for non-pointers
chore: improve Same and NotSame tests

Motivation

NotSame had a different behavior than the other asserters:

NotImplements fails and returns false for nil
NotElementsMatch fails and returns false for non-lists
NotEqual fails and returns false for invalid arguments
NotContains fails and returns false if a length cannot be computed
NotSubset fails and returns false if the objects are not comparable- fix: NotSame was returning true for non-pointers

Nothing returns true, and Fail.

Related to #1660, #1661, #1664

The code was different that the comment. Also, it's a different behavior than the other asserters: - NotImplements fails and returns false for nil - NotElementsMatch fails and returns false for non-lists - NotEqual fails and returns false for invalid arguments - NotContains fails and returns false if a length cannot be computed - NotSubset fails and returns false if the objects are not comparable

Previous tests where very basic, and checked only the returned values. Checking the failure messages is way more important, as it helps to avoid regressions in the future.

dolmen

Do not remove the existing testsuites.

I want to be able to see that with your changes the existing testsuite still pass, and your new tests also.

dolmen · 2025-05-13T07:43:56Z

assert/assertions_test.go

+			result:   false,
+			resultErrMsg: "Not same: \n" +
+				fmt.Sprintf("expected: %v %#v\n", p1, p1) +
+				fmt.Sprintf("actual  : %v %#v\n", p1bis, p1bis),


Use a single fmt.Sprintf expression for the whole string.

Use indexed formats like %#[1]v to remove parameter duplication (we should do that also in Same/NotSame code, but in a separate PR).

Use %p like in the tested code

dolmen · 2025-05-13T07:44:35Z

assert/assertions_test.go

+			result:   false,
+			resultErrMsg: "Not same: \n" +
+				fmt.Sprintf("expected: %v %#v\n", p1, p1) +
+				fmt.Sprintf("actual  : %v %#v\n", p2, p2),


Use a single fmt.Sprintf expression for the whole string.

Use indexed formats like %#[1]v to remove parameter duplication (we should do that also in Same/NotSame code, but in a separate PR).

Use %p like in the tested code

dolmen · 2025-05-13T07:44:45Z

assert/assertions_test.go

+			expected: p1,
+			result:   false,
+			resultErrMsg: "Expected and actual point to the same object: " +
+				fmt.Sprintf("%v %#v\n", p1, p1),


Use a single fmt.Sprintf expression for the whole string.

Use indexed formats like %#[1]v to remove parameter duplication (we should do that also in Same/NotSame code, but in a separate PR).

Use %p like in the tested code

we should do that also in Same/NotSame code, but in a separate PR

Done with #1742

assert.Same/NotSame: improve usage of Sprintf #1742

ccoVeille

Good feedbacks, I will address them all.

I have already opened #1742 about using indexes when its possible

#1742

assert/assertions.go

ccoVeille · 2025-05-13T13:17:37Z

assert/assertions_test.go

+			expected: p1,
+			result:   false,
+			resultErrMsg: "Expected and actual point to the same object: " +
+				fmt.Sprintf("%v %#v\n", p1, p1),


we should do that also in Same/NotSame code, but in a separate PR

Done with #1742

assert.Same/NotSame: improve usage of Sprintf #1742

ccoVeille · 2025-05-14T19:09:54Z

I'm closing this PR and I will split it in two PRs.

This will also allow me to switch to my fork.

ccoVeille · 2025-05-17T12:43:54Z

Some follow-ups:

I started working locally on the changes that could land in a new PR.

The more I work on the changes, the more doubtful I am if I was going in the right direction with this PR.

I mean, I'm no longer sure NotSame was not OK, and the others (NotImplements,NotElementsMatch,NotEqual, NotContains, NotSubset…) weren't the ones to fix in fact

ccoVeille · 2025-06-03T11:54:15Z

I feel like I faced the same issue that @olivergondza had with

assert.PanicsWithError: report error message #1400 (comment)

Unless I'm wrong Oliver came to the same conclusion than mine: captureTestingT cannot be used with negative asserters like NotSame and co.

Could you confirm Oliver and share your thoughts about this struct?

Also I'm curious about what you said about other label than errors.

Please note, I'm aware of the code you talked about and the loop on label to look for the one holding Error.

I'm just unsure what are the other labels you talked about.

I feel like I should conduct a refactoring of captureTestingT, that could help to handle negative asserter like NotSame, and the other I listed in my previous post above this one.

Thanks

olivergondza · 2025-06-03T12:51:37Z

I feel like I faced the same issue that @olivergondza had with

assert.PanicsWithError: report error message #1400 (comment)

Unless I'm wrong Oliver came to the same conclusion than mine: captureTestingT cannot be used with negative asserters like NotSame and co.

Could you confirm Oliver and share your thoughts about this struct?

Also I'm curious about what you said about other label than errors.

Please note, I'm aware of the code you talked about and the loop on label to look for the one holding Error.

I'm just unsure what are the other labels you talked about.

I feel like I should conduct a refactoring of captureTestingT, that could help to handle negative asserter like NotSame, and the other I listed in my previous post above this one.

Glad I am not alone who was fighting with this :)

Long story short, I ended up using explicit assertions on captureTestingT.msg over captureTestingT.checkResultAndErrMsg.

The reason captureTestingT.checkResultAndErrMsg not worked for me all that well is twofold:

It parses the "mismatch" message into assertions.labeledContent, and only check the content of the line labeled with "Error".
It only assert exact string match of the given portion.

My PR introduced the whole new label for panic, that it does not examine, obviously. And I also needed contains / not contains on the entire unparsed "mismatch" message.

I have spent some time looking into refactoring checkResultAndErrMsg to account for all that, but that would force me to sacrifice defect localization - as the messages would become too generic to hardly be helpful. Also, the API would likely bloat somewhat, and for a method that have 4 arguments already, it did not seem like a good idea.

But as long as I can get the entire message, I am fine using captureTestingT.msg as demonstrated by the other PR.

ccoVeille · 2025-06-03T14:13:13Z

Thanks for your detailed reply.

I do feel like there are room for improvement yes. cc @brackendawson @dolmen

You are right, the panic has to be handled differently.

I'm still interested if you could open a PR for a refactoring suggestion of checkResultAndErrMsg

ccoVeille added 2 commits May 9, 2025 03:01

chore: improve Same and NotSame tests

a18e52d

Previous tests where very basic, and checked only the returned values. Checking the failure messages is way more important, as it helps to avoid regressions in the future.

ccoVeille mentioned this pull request May 12, 2025

Improve ErrorAs failure message when error is nil #1734

Merged

dolmen requested changes May 13, 2025

View reviewed changes

ccoVeille mentioned this pull request May 13, 2025

assert.Same/NotSame: improve usage of Sprintf #1742

Merged

ccoVeille commented May 13, 2025

View reviewed changes

ccoVeille closed this May 14, 2025

ccoVeille deleted the not-same branch May 14, 2025 19:10

fix: NotSame was returning true for non-pointers #1738

fix: NotSame was returning true for non-pointers #1738

Uh oh!

Conversation

ccoVeille commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Motivation

Uh oh!

dolmen left a comment

Choose a reason for hiding this comment

Uh oh!

dolmen May 13, 2025

Choose a reason for hiding this comment

Uh oh!

dolmen May 13, 2025

Choose a reason for hiding this comment

Uh oh!

dolmen May 13, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille May 13, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ccoVeille May 13, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccoVeille commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccoVeille commented Jun 3, 2025

Uh oh!

olivergondza commented Jun 3, 2025

Uh oh!

ccoVeille commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ccoVeille commented May 9, 2025 •

edited

Loading

ccoVeille commented May 14, 2025 •

edited

Loading

ccoVeille commented May 17, 2025 •

edited

Loading