Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion on API change #10

Open
dertseha opened this issue Oct 8, 2019 · 3 comments
Open

Discussion on API change #10

dertseha opened this issue Oct 8, 2019 · 3 comments

Comments

@dertseha
Copy link
Contributor

dertseha commented Oct 8, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants