NotSame should fail if args are not pointers #1661 by sikehish · Pull Request #1664 · stretchr/testify

sikehish · 2024-10-17T10:27:28Z

Summary

Reduces the confusion assosciated with NotSame that previously would check nothing if any of the values is not a pointer. The changes made were tested using TestSame, TestNotSame, and Test_samePointers tests, and the changes did clear the tests.

Changes

Modified samePointers to return another bool value(ok) that would determine if the 2 values are of pointer type or not, while the returned "same" bool value would determine if the 2 pointers are same.
Modified assert.NotSame to call Fail() if the 2 values are either i)pointers pointing to the same address or ii)either/both of the values are not pointers.
Modified assert.Same to call Fail() if the 2 values are either i)pointers not pointing to the same address or ii)either/both of the values are not pointers.
Modified Test_samePointers to handle the new behavior of samePointers by checking both if the inputs are pointers (ok) and if they point to the same object (same).

Motivation

Ensure NotSame accurately verifies pointer sameness by handling non-pointer inputs explicitly, improving clarity and reducing potential misuse.

Related issues

Closes #1661

…n in assertions_test.go

assert/assertions.go

assert/assertions_test.go

Co-authored-by: Bracken <abdawson@gmail.com>

sikehish · 2024-10-18T17:20:30Z

Hi, @brackendawson.
I've made all the requested amendments. Do let me know if any other changes are to be made :)

brackendawson

Sorry for the late re-review, that's looking good, just a couple more bits.

brackendawson · 2024-10-26T18:45:15Z

assert/assertions.go

-			expected, expected), msgAndArgs...)
+	same, ok := samePointers(expected, actual)
+
+	if ok {


Almost there, if you flip this to if !ok { then you can avoid the nested if statements and fully preserve line of sight.

Also we don't really want the blank line first as this is "error handling" for the line above.

I've made the changes

brackendawson · 2024-10-26T18:47:55Z

assert/assertions_test.go

+		name string
+		args args
+		same BoolAssertionFunc
+		ok   BoolAssertionFunc


That's perfect. 👌

Can you add a couple more test cases for when the left and right arguments aren't pointers but the other one is, like:

args{first: 1, second: p}

args{first: p, second: 1}

Yup, I've added 2 additional test cases now

…nters in assert/assertions_test.go

… in assert/assertions.go

sikehish · 2024-10-26T19:21:30Z

Hi @brackendawson. I've made the requested changes. Let me know if any other changes need to be made.

brackendawson

Code is spot on 👍, just found one doc-string inaccuracy.

assert/assertions.go

Co-authored-by: Bracken <abdawson@gmail.com>

brackendawson

LGTM, thank you for your contribution.

sikehish · 2024-10-28T11:18:42Z

LGTM, thank you for your contribution.

Thanks for the opportunity :)

…nercloud/fleeting-plugin-hetzner!175) This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/stretchr/testify](https://github.com/stretchr/testify) | require | minor | `v1.9.0` -> `v1.10.0` | --- ### Release Notes <details> <summary>stretchr/testify (github.com/stretchr/testify)</summary> ### [`v1.10.0`](https://github.com/stretchr/testify/releases/tag/v1.10.0) [Compare Source](stretchr/testify@v1.9.0...v1.10.0) #### What's Changed ##### Functional Changes - Add PanicAssertionFunc by [@fahimbagar](https://github.com/fahimbagar) in stretchr/testify#1337 - assert: deprecate CompareType by [@dolmen](https://github.com/dolmen) in stretchr/testify#1566 - assert: make YAML dependency pluggable via build tags by [@dolmen](https://github.com/dolmen) in stretchr/testify#1579 - assert: new assertion NotElementsMatch by [@hendrywiranto](https://github.com/hendrywiranto) in stretchr/testify#1600 - mock: in order mock calls by [@ReyOrtiz](https://github.com/ReyOrtiz) in stretchr/testify#1637 - Add assertion for NotErrorAs by [@palsivertsen](https://github.com/palsivertsen) in stretchr/testify#1129 - Record Return Arguments of a Call by [@jayd3e](https://github.com/jayd3e) in stretchr/testify#1636 - assert.EqualExportedValues: accepts everything by [@redachl](https://github.com/redachl) in stretchr/testify#1586 ##### Fixes - assert: make tHelper a type alias by [@dolmen](https://github.com/dolmen) in stretchr/testify#1562 - Do not get argument again unnecessarily in Arguments.Error() by [@TomWright](https://github.com/TomWright) in stretchr/testify#820 - Fix time.Time compare by [@myxo](https://github.com/myxo) in stretchr/testify#1582 - assert.Regexp: handle \[]byte array properly by [@kevinburkesegment](https://github.com/kevinburkesegment) in stretchr/testify#1587 - assert: collect.FailNow() should not panic by [@marshall-lee](https://github.com/marshall-lee) in stretchr/testify#1481 - mock: simplify implementation of FunctionalOptions by [@dolmen](https://github.com/dolmen) in stretchr/testify#1571 - mock: caller information for unexpected method call by [@spirin](https://github.com/spirin) in stretchr/testify#1644 - suite: fix test failures by [@stevenh](https://github.com/stevenh) in stretchr/testify#1421 - Fix issue [#1662](stretchr/testify#1662) (comparing infs should fail) by [@ybrustin](https://github.com/ybrustin) in stretchr/testify#1663 - NotSame should fail if args are not pointers [#1661](stretchr/testify#1661) by [@sikehish](https://github.com/sikehish) in stretchr/testify#1664 - Increase timeouts in Test_Mock_Called_blocks to reduce flakiness in CI by [@sikehish](https://github.com/sikehish) in stretchr/testify#1667 - fix: compare functional option names for indirect calls by [@arjun-1](https://github.com/arjun-1) in stretchr/testify#1626 ##### Documantation, Build & CI - .gitignore: ignore "go test -c" binaries by [@dolmen](https://github.com/dolmen) in stretchr/testify#1565 - mock: improve doc by [@dolmen](https://github.com/dolmen) in stretchr/testify#1570 - mock: fix FunctionalOptions docs by [@snirye](https://github.com/snirye) in stretchr/testify#1433 - README: link out to the excellent testifylint by [@brackendawson](https://github.com/brackendawson) in stretchr/testify#1568 - assert: fix typo in comment by [@JohnEndson](https://github.com/JohnEndson) in stretchr/testify#1580 - Correct the EventuallyWithT and EventuallyWithTf example by [@JonCrowther](https://github.com/JonCrowther) in stretchr/testify#1588 - CI: bump softprops/action-gh-release from 1 to 2 by [@dependabot](https://github.com/dependabot) in stretchr/testify#1575 - mock: document more alternatives to deprecated AnythingOfTypeArgument by [@dolmen](https://github.com/dolmen) in stretchr/testify#1569 - assert: Correctly document EqualValues behavior by [@brackendawson](https://github.com/brackendawson) in stretchr/testify#1593 - fix: grammar in godoc by [@miparnisari](https://github.com/miparnisari) in stretchr/testify#1607 - .github/workflows: Run tests for Go 1.22 by [@HaraldNordgren](https://github.com/HaraldNordgren) in stretchr/testify#1629 - Document suite's lack of support for t.Parallel by [@brackendawson](https://github.com/brackendawson) in stretchr/testify#1645 - assert: fix typos in comments by [@alexandear](https://github.com/alexandear) in stretchr/testify#1650 - mock: fix doc comment for NotBefore by [@alexandear](https://github.com/alexandear) in stretchr/testify#1651 - Generate better comments for require package by [@Neokil](https://github.com/Neokil) in stretchr/testify#1610 - README: replace Testify V2 notice with [@dolmen](https://github.com/dolmen)'s V2 manifesto by [@hendrywiranto](https://github.com/hendrywiranto) in stretchr/testify#1518 #### New Contributors - [@fahimbagar](https://github.com/fahimbagar) made their first contribution in stretchr/testify#1337 - [@TomWright](https://github.com/TomWright) made their first contribution in stretchr/testify#820 - [@snirye](https://github.com/snirye) made their first contribution in stretchr/testify#1433 - [@myxo](https://github.com/myxo) made their first contribution in stretchr/testify#1582 - [@JohnEndson](https://github.com/JohnEndson) made their first contribution in stretchr/testify#1580 - [@JonCrowther](https://github.com/JonCrowther) made their first contribution in stretchr/testify#1588 - [@miparnisari](https://github.com/miparnisari) made their first contribution in stretchr/testify#1607 - [@marshall-lee](https://github.com/marshall-lee) made their first contribution in stretchr/testify#1481 - [@spirin](https://github.com/spirin) made their first contribution in stretchr/testify#1644 - [@ReyOrtiz](https://github.com/ReyOrtiz) made their first contribution in stretchr/testify#1637 - [@stevenh](https://github.com/stevenh) made their first contribution in stretchr/testify#1421 - [@jayd3e](https://github.com/jayd3e) made their first contribution in stretchr/testify#1636 - [@Neokil](https://github.com/Neokil) made their first contribution in stretchr/testify#1610 - [@redachl](https://github.com/redachl) made their first contribution in stretchr/testify#1586 - [@ybrustin](https://github.com/ybrustin) made their first contribution in stretchr/testify#1663 - [@sikehish](https://github.com/sikehish) made their first contribution in stretchr/testify#1664 - [@arjun-1](https://github.com/arjun-1) made their first contribution in stretchr/testify#1626 **Full Changelog**: stretchr/testify@v1.9.0...v1.10.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

@sikehish

#### Description Fix the TestPerfCounter_Reset that is failing consistently. ```terminal === FAIL: . TestPerfCounter_Reset (0.00s) watcher_test.go:131: Error Trace: D:/a/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/winperfcounters/watcher_test.go:131 Error: Both arguments must be pointers Test: TestPerfCounter_Reset ``` Root case: after upgrading `testify` circa of 2 weeks ago, since it included: > NotSame should fail if args are not pointers stretchr/testify#1661 by @sikehish in stretchr/testify#1664 See https://github.com/stretchr/testify/releases #### Testing Local test run Co-authored-by: Antoine Toulme <atoulme@splunk.com>

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/stretchr/testify](https://github.com/stretchr/testify) | require | minor | `v1.9.0` -> `v1.10.0` | --- ### Release Notes <details> <summary>stretchr/testify (github.com/stretchr/testify)</summary> ### [`v1.10.0`](https://github.com/stretchr/testify/releases/tag/v1.10.0) [Compare Source](stretchr/testify@v1.9.0...v1.10.0) #### What's Changed ##### Functional Changes - Add PanicAssertionFunc by [@fahimbagar](https://github.com/fahimbagar) in stretchr/testify#1337 - assert: deprecate CompareType by [@dolmen](https://github.com/dolmen) in stretchr/testify#1566 - assert: make YAML dependency pluggable via build tags by [@dolmen](https://github.com/dolmen) in stretchr/testify#1579 - assert: new assertion NotElementsMatch by [@hendrywiranto](https://github.com/hendrywiranto) in stretchr/testify#1600 - mock: in order mock calls by [@ReyOrtiz](https://github.com/ReyOrtiz) in stretchr/testify#1637 - Add assertion for NotErrorAs by [@palsivertsen](https://github.com/palsivertsen) in stretchr/testify#1129 - Record Return Arguments of a Call by [@jayd3e](https://github.com/jayd3e) in stretchr/testify#1636 - assert.EqualExportedValues: accepts everything by [@redachl](https://github.com/redachl) in stretchr/testify#1586 ##### Fixes - assert: make tHelper a type alias by [@dolmen](https://github.com/dolmen) in stretchr/testify#1562 - Do not get argument again unnecessarily in Arguments.Error() by [@TomWright](https://github.com/TomWright) in stretchr/testify#820 - Fix time.Time compare by [@myxo](https://github.com/myxo) in stretchr/testify#1582 - assert.Regexp: handle \[]byte array properly by [@kevinburkesegment](https://github.com/kevinburkesegment) in stretchr/testify#1587 - assert: collect.FailNow() should not panic by [@marshall-lee](https://github.com/marshall-lee) in stretchr/testify#1481 - mock: simplify implementation of FunctionalOptions by [@dolmen](https://github.com/dolmen) in stretchr/testify#1571 - mock: caller information for unexpected method call by [@spirin](https://github.com/spirin) in stretchr/testify#1644 - suite: fix test failures by [@stevenh](https://github.com/stevenh) in stretchr/testify#1421 - Fix issue [#1662](stretchr/testify#1662) (comparing infs should fail) by [@ybrustin](https://github.com/ybrustin) in stretchr/testify#1663 - NotSame should fail if args are not pointers [#1661](stretchr/testify#1661) by [@sikehish](https://github.com/sikehish) in stretchr/testify#1664 - Increase timeouts in Test_Mock_Called_blocks to reduce flakiness in CI by [@sikehish](https://github.com/sikehish) in stretchr/testify#1667 - fix: compare functional option names for indirect calls by [@arjun-1](https://github.com/arjun-1) in stretchr/testify#1626 ##### Documantation, Build & CI - .gitignore: ignore "go test -c" binaries by [@dolmen](https://github.com/dolmen) in stretchr/testify#1565 - mock: improve doc by [@dolmen](https://github.com/dolmen) in stretchr/testify#1570 - mock: fix FunctionalOptions docs by [@snirye](https://github.com/snirye) in stretchr/testify#1433 - README: link out to the excellent testifylint by [@brackendawson](https://github.com/brackendawson) in stretchr/testify#1568 - assert: fix typo in comment by [@JohnEndson](https://github.com/JohnEndson) in stretchr/testify#1580 - Correct the EventuallyWithT and EventuallyWithTf example by [@JonCrowther](https://github.com/JonCrowther) in stretchr/testify#1588 - CI: bump softprops/action-gh-release from 1 to 2 by [@dependabot](https://github.com/dependabot) in stretchr/testify#1575 - mock: document more alternatives to deprecated AnythingOfTypeArgument by [@dolmen](https://github.com/dolmen) in stretchr/testify#1569 - assert: Correctly document EqualValues behavior by [@brackendawson](https://github.com/brackendawson) in stretchr/testify#1593 - fix: grammar in godoc by [@miparnisari](https://github.com/miparnisari) in stretchr/testify#1607 - .github/workflows: Run tests for Go 1.22 by [@HaraldNordgren](https://github.com/HaraldNordgren) in stretchr/testify#1629 - Document suite's lack of support for t.Parallel by [@brackendawson](https://github.com/brackendawson) in stretchr/testify#1645 - assert: fix typos in comments by [@alexandear](https://github.com/alexandear) in stretchr/testify#1650 - mock: fix doc comment for NotBefore by [@alexandear](https://github.com/alexandear) in stretchr/testify#1651 - Generate better comments for require package by [@Neokil](https://github.com/Neokil) in stretchr/testify#1610 - README: replace Testify V2 notice with [@dolmen](https://github.com/dolmen)'s V2 manifesto by [@hendrywiranto](https://github.com/hendrywiranto) in stretchr/testify#1518 #### New Contributors - [@fahimbagar](https://github.com/fahimbagar) made their first contribution in stretchr/testify#1337 - [@TomWright](https://github.com/TomWright) made their first contribution in stretchr/testify#820 - [@snirye](https://github.com/snirye) made their first contribution in stretchr/testify#1433 - [@myxo](https://github.com/myxo) made their first contribution in stretchr/testify#1582 - [@JohnEndson](https://github.com/JohnEndson) made their first contribution in stretchr/testify#1580 - [@JonCrowther](https://github.com/JonCrowther) made their first contribution in stretchr/testify#1588 - [@miparnisari](https://github.com/miparnisari) made their first contribution in stretchr/testify#1607 - [@marshall-lee](https://github.com/marshall-lee) made their first contribution in stretchr/testify#1481 - [@spirin](https://github.com/spirin) made their first contribution in stretchr/testify#1644 - [@ReyOrtiz](https://github.com/ReyOrtiz) made their first contribution in stretchr/testify#1637 - [@stevenh](https://github.com/stevenh) made their first contribution in stretchr/testify#1421 - [@jayd3e](https://github.com/jayd3e) made their first contribution in stretchr/testify#1636 - [@Neokil](https://github.com/Neokil) made their first contribution in stretchr/testify#1610 - [@redachl](https://github.com/redachl) made their first contribution in stretchr/testify#1586 - [@ybrustin](https://github.com/ybrustin) made their first contribution in stretchr/testify#1663 - [@sikehish](https://github.com/sikehish) made their first contribution in stretchr/testify#1664 - [@arjun-1](https://github.com/arjun-1) made their first contribution in stretchr/testify#1626 **Full Changelog**: stretchr/testify@v1.9.0...v1.10.0 </details> --- - [ ] If you want to rebase/retry this MR, check this box ---    See merge request alpine/infra/build-server-status!18

@sikehish

#### Description Fix the TestPerfCounter_Reset that is failing consistently. ```terminal === FAIL: . TestPerfCounter_Reset (0.00s) watcher_test.go:131: Error Trace: D:/a/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/winperfcounters/watcher_test.go:131 Error: Both arguments must be pointers Test: TestPerfCounter_Reset ``` Root case: after upgrading `testify` circa of 2 weeks ago, since it included: > NotSame should fail if args are not pointers stretchr/testify#1661 by @sikehish in stretchr/testify#1664 See https://github.com/stretchr/testify/releases #### Testing Local test run Co-authored-by: Antoine Toulme <atoulme@splunk.com>

@sikehish

#### Description Fix the TestPerfCounter_Reset that is failing consistently. ```terminal === FAIL: . TestPerfCounter_Reset (0.00s) watcher_test.go:131: Error Trace: D:/a/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/winperfcounters/watcher_test.go:131 Error: Both arguments must be pointers Test: TestPerfCounter_Reset ``` Root case: after upgrading `testify` circa of 2 weeks ago, since it included: > NotSame should fail if args are not pointers stretchr/testify#1661 by @sikehish in stretchr/testify#1664 See https://github.com/stretchr/testify/releases #### Testing Local test run Co-authored-by: Antoine Toulme <atoulme@splunk.com>

…1664) ## Summary Reduces the confusion assosciated with NotSame that previously would check nothing if any of the values is not a pointer. The changes made were tested using TestSame, TestNotSame, and Test_samePointers tests, and the changes did clear the tests. ## Changes 1. Modified samePointers to return another bool value(ok) that would determine if the 2 values are of pointer type or not, while the returned "same" bool value would determine if the 2 pointers are same. 2. Modified assert.NotSame to call Fail() if the 2 values are either i)pointers pointing to the same address or ii)either/both of the values are not pointers. 3. Modified assert.Same to call Fail() if the 2 values are either i)pointers not pointing to the same address or ii)either/both of the values are not pointers. 4. Modified Test_samePointers to handle the new behavior of samePointers by checking both if the inputs are pointers (ok) and if they point to the same object (same). ## Motivation Ensure NotSame accurately verifies pointer sameness by handling non-pointer inputs explicitly, improving clarity and reducing potential misuse. ## Related issues Closes stretchr#1661 --------- Co-authored-by: Bracken <abdawson@gmail.com>

@sikehish

#### Description Fix the TestPerfCounter_Reset that is failing consistently. ```terminal === FAIL: . TestPerfCounter_Reset (0.00s) watcher_test.go:131: Error Trace: D:/a/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/winperfcounters/watcher_test.go:131 Error: Both arguments must be pointers Test: TestPerfCounter_Reset ``` Root case: after upgrading `testify` circa of 2 weeks ago, since it included: > NotSame should fail if args are not pointers stretchr/testify#1661 by @sikehish in stretchr/testify#1664 See https://github.com/stretchr/testify/releases #### Testing Local test run Co-authored-by: Antoine Toulme <atoulme@splunk.com>

sikehish added 3 commits October 17, 2024 13:56

Modified samePointers signature and additional boolean returned.

e0d1ff1

Modified return statement in assert.NotSame

8bff88f

Modified Test_samePointers based on the modified samePointers functio…

65657a1

…n in assertions_test.go

sikehish mentioned this pull request Oct 17, 2024

NotSame should fail if args are not pointers #1661

Closed

brackendawson requested changes Oct 18, 2024

View reviewed changes

assert/assertions.go Show resolved Hide resolved

assert/assertions.go Outdated Show resolved Hide resolved

assert/assertions.go Outdated Show resolved Hide resolved

assert/assertions_test.go Outdated Show resolved Hide resolved

sikehish and others added 4 commits October 18, 2024 22:15

Update assert/assertions.go

c64e595

Co-authored-by: Bracken <abdawson@gmail.com>

Updated doc-string of samePointers in assert/assertions.go

e2f3668

Refactored NotSame in assert/assertions.go to preserve line of sight

31b28ee

Refactored Test_samePointers

c7b4b09

brackendawson requested changes Oct 26, 2024

View reviewed changes

sikehish added 2 commits October 27, 2024 00:40

Add test cases for pointer vs non-pointer comparisons in Test_samePoi…

17dcafd

…nters in assert/assertions_test.go

Refactor NotSame to improve readability and streamline error handling…

e7392c6

… in assert/assertions.go

brackendawson requested changes Oct 27, 2024

View reviewed changes

assert/assertions.go Outdated Show resolved Hide resolved

Update assert/assertions.go

db25ad0

Co-authored-by: Bracken <abdawson@gmail.com>

brackendawson approved these changes Oct 28, 2024

View reviewed changes

Merge branch 'master' into master

5392ef3

brackendawson merged commit 118fb83 into stretchr:master Oct 28, 2024

turbolent mentioned this pull request Dec 4, 2024

Update to Cadence v1.3.0 onflow/flow-go#6779

Merged

pjanotti mentioned this pull request Dec 9, 2024

[chore] Fix TestPerfCounter_Reset test open-telemetry/opentelemetry-collector-contrib#36726

Merged

zmoog mentioned this pull request Feb 19, 2025

build(deps): bump github.com/Azure/azure-sdk-for-go/sdk/azidentity from 1.7.0 to 1.8.2 elastic/beats#42690

Merged

ccoVeille mentioned this pull request May 9, 2025

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

Closed

renovate bot mentioned this pull request Jul 31, 2025

fix(deps): update all moul/u#74

Open

1 task

Conversation

sikehish commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Motivation

Related issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sikehish commented Oct 18, 2024

Uh oh!

brackendawson left a comment

Choose a reason for hiding this comment

Uh oh!

brackendawson Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

brackendawson Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

sikehish Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

brackendawson Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

sikehish Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

sikehish commented Oct 26, 2024

Uh oh!

brackendawson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brackendawson left a comment

Choose a reason for hiding this comment

Uh oh!

sikehish commented Oct 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sikehish commented Oct 17, 2024 •

edited

Loading

brackendawson left a comment •

edited

Loading