testify icon indicating copy to clipboard operation
testify copied to clipboard

Move object equality functions to another package for clarity

Open mminklet opened this issue 3 years ago • 4 comments

Summary

Move none assertion functions into a seperate package to make it clear that these do no do any assertions. There is a a lot of existing code out there with useless tests because people are assuming that assert.Foobar functions will do assertions.

Happy to take guidance on comments/naming etc.

Changes

Moved ObjectsAreEqual and ObjectsAreEqualValues and tests into a new sub package equals. Label existing functions as deprecated and call new functions instead. Call new methods anywhere in assert where the previous functions are called.

Motivation

Currently very confusing that these are in the assert package when they don't do any assertions at all.

change assert.ObjectsAreEqual to equal.ObjectsAreEqual etc

Related issues

Closes 1180

mminklet avatar Oct 10 '22 02:10 mminklet

If the aim of this is to prevent people from thinking that ObjectsAreEqual is an assertion that they should use, then I think this new equal package should be internal:

testify/
|-- assert
|   `-- assertions.go
|-- internal
|   `-- equal
|       `-- equal.go
|-- mock
|   `-- mock.go

brackendawson avatar Oct 10 '22 09:10 brackendawson

If the aim of this is to prevent people from thinking that ObjectsAreEqual is an assertion that they should use, then I think this new equal package should be internal:

testify/
|-- assert
|   `-- assertions.go
|-- internal
|   `-- equal
|       `-- equal.go
|-- mock
|   `-- mock.go

I was actually thinking that people would still continue to use these methods, as they are useful, they're just not immediately obvious that the bool needs to be checked to have a working test. But perhaps another name/location outside of assert makes more sense?

mminklet avatar Oct 10 '22 10:10 mminklet

If the aim of this is to prevent people from thinking that ObjectsAreEqual is an assertion that they should use, then I think this new equal package should be internal:

testify/
|-- assert
|   `-- assertions.go
|-- internal
|   `-- equal
|       `-- equal.go
|-- mock
|   `-- mock.go

I was actually thinking that people would still continue to use these methods, as they are useful, they're just not immediately obvious that the bool needs to be checked to have a working test. But perhaps another name/location outside of assert makes more sense?

While it's true that they might be useful to someone, I think people will still misuse them in exactly the same way. Testify is not a utility library, I think if people are looking for a comparison library then they should be looking at go-cmp instead.

brackendawson avatar Oct 10 '22 11:10 brackendawson

Yeah that's a good point. Tbh then I'm a bit reticent about introducing this change then, ~~as I don't want to remove useful functionality even if it is currently being misused~~

Actually scratch that, I realise that people just need to be directed to use the Equal and EqualValues assertions instead. I will alter my PR now

Edit: Updated with suggested changes.

mminklet avatar Oct 10 '22 22:10 mminklet

Is there any progress on this? If there are suggestions that are required, please let me know and I'll gladly make them. People are still being caught by this

mminklet avatar Mar 27 '23 23:03 mminklet

Please rebase this branch on top of master, to fix gofmt issues.

dolmen avatar Jul 10 '23 06:07 dolmen

Replaced by #1431.

dolmen avatar Jul 25 '23 04:07 dolmen

Closing in favor of #1431.

dolmen avatar Aug 08 '23 12:08 dolmen

Nice! Sorry I didn’t have time to rebase, it had been so long that there were more changes that needed more work. Glad to see the change got in anyway!

mminklet avatar Aug 08 '23 22:08 mminklet