NotSame should fail if args are not pointers #1661 #1664

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 <[email protected]>

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 <[email protected]>

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 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 Outdated 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 <[email protected]>

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 <[email protected]>

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
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NotSame should fail if args are not pointers #1661 #1664

NotSame should fail if args are not pointers #1661 #1664

sikehish commented Oct 17, 2024 •

edited

Loading

sikehish commented Oct 18, 2024

brackendawson left a comment

brackendawson Oct 26, 2024

brackendawson Oct 26, 2024

sikehish Oct 26, 2024

brackendawson Oct 26, 2024

sikehish Oct 26, 2024

sikehish commented Oct 26, 2024

brackendawson left a comment •

edited

Loading

brackendawson left a comment

sikehish commented Oct 28, 2024

NotSame should fail if args are not pointers #1661 #1664

NotSame should fail if args are not pointers #1661 #1664

Conversation

sikehish commented Oct 17, 2024 • edited Loading

Summary

Changes

Motivation

Related issues

sikehish commented Oct 18, 2024

brackendawson left a comment

Choose a reason for hiding this comment

brackendawson Oct 26, 2024

Choose a reason for hiding this comment

brackendawson Oct 26, 2024

Choose a reason for hiding this comment

sikehish Oct 26, 2024

Choose a reason for hiding this comment

brackendawson Oct 26, 2024

Choose a reason for hiding this comment

sikehish Oct 26, 2024

Choose a reason for hiding this comment

sikehish commented Oct 26, 2024

brackendawson left a comment • edited Loading

Choose a reason for hiding this comment

brackendawson left a comment

Choose a reason for hiding this comment

sikehish commented Oct 28, 2024

sikehish commented Oct 17, 2024 •

edited

Loading

brackendawson left a comment •

edited

Loading