search

approvals / go-approval-tests

Apache License 2.0
86 stars 22 forks source link

Discussion on API change #10

Open dertseha opened 4 years ago

dertseha commented 4 years ago

Hello there!

Based on the Twitter exchange, I now came across this repo. Starting to take an interest in it (See #9), I want to discuss a few further changes, which probably require an API change.

Avoidance of error return of Verify* functions

Starting with the error return, I'd remove this and have the Verify* function simply FailNow() for any internal error. This then also removes the linter warnings for the user, who probably wouldn't do anything else. (as a side effect, I'd then go ahead and enable the linter for error returns and handle any remaining issues)

Use of idiomatic testdata directory for file storage

Then I'd have all the files be read relative to the idiomatic testdata directory. This could be hardcoded.

Use of test name (t.Name()) instead of reflection and name-guessing

Finally, using t.Name() would also allow for table-driven tests (typically used in combination with t.Run()), receiving different names per case. (this one is on the TODO anyway)

All of this requires an extension of the Failable interface, as well as moving the reference files.

Since there hasn't been a v1.x.x release yet, this would be OK from a Go-versioning perspective. v0.x.x is allowed to have a brittle API until settled.

I understand you accept rather smaller pull requests - would these three steps be small enough for you? What's the rule of thumb? :)

emilybache commented 3 years ago

I would also like to see support for table-driven tests and the suggestion of using t.Name() seems much more sensible than parsing the stack trace.

emilybache commented 3 years ago

I just made some changes that work for table-driven parameterized tests. I also addressed the linter problem with ignored errors, I think. I'm not sure what you mean by idiomatic use of 'testdata' directory so I didn't attempt that.

I'd love your feedback on these changes

dertseha commented 3 years ago

Wow, thank you for picking this up. I pretty much forgot about having opened this.

Skimming over the changes, all looks good!

As for the detail on testdata, someone else also picked this up and #16 explains it more.

With that, this issue would be resolved and can be closed.