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

proposal: testing: better support test helper functions with TB.Helper #18440

Closed
cespare opened this issue Dec 28, 2016 · 8 comments
Closed

proposal: testing: better support test helper functions with TB.Helper #18440

cespare opened this issue Dec 28, 2016 · 8 comments

Comments

@cespare
Copy link
Contributor

cespare commented Dec 28, 2016

(Placeholder for design doc.)

Can someone with sufficient access please attach the Proposal label?

@minux minux added the Proposal label Dec 28, 2016
@dsnet dsnet added this to the Proposal milestone Dec 28, 2016
@cespare
Copy link
Contributor Author

cespare commented Dec 28, 2016

I mailed the proposal CL: https://golang.org/cl/34717.

Once the proposal has been submitted, I will update this post with a link to the rendered markdown.

@cespare cespare changed the title proposal: testing: better support test helper functions with (TB).Helper proposal: testing: better support test helper functions with TB.Helper Dec 28, 2016
@gopherbot
Copy link

CL https://golang.org/cl/34717 mentions this issue.

@cespare
Copy link
Contributor Author

cespare commented Dec 28, 2016

One detail I should mention: I was unable to come up with a concrete reason why there should be any relationship between helper functions for subtests. That is, I couldn't think of a case where you'd want to call t0.Helper but t1.Fatal, where t1 is, say, a subtest of t0.

This is quite possibly just a lack of imagination on my part. If someone can come up with a reason why there should be such a relationship, we could add subtest semantics to the proposal (perhaps: a function marked as a helper for t is considered a helper in all subtests of t as well.)

@rogpeppe
Copy link
Contributor

rogpeppe commented Jan 2, 2017

The Helper proposal doesn't seem quite right to me. I use a lot of test helper functions and it's not uncommon for them to nest 3 or 4 levels deep. In my view there is no single correct line number to print - the whole stack up to the top level Test* function is all useful context.

As one example, I often use @niemeyer's check package. When that prints an error, it will print up to two line numbers - the top level and the place where Fatalf was called. That's similar to what's purposed here, but it's not sufficient. I have my own local patch that changes that behaviour to print all levels of the stack because eliding steak frames in the middle can lose context that's needed to work out where the failure took place.

@cespare
Copy link
Contributor Author

cespare commented Jan 3, 2017

Thanks for the feedback @rogpeppe.

In your view, is the better change to add some facility to testing for reporting multiple file:lines? (As a straw man, t.Error2/t.Fatal2 which print out all the file:lines on the stack up to and including a TestXxx function.)

If not, do you think this proposal is an improvement over the status quo?

@rsc
Copy link
Contributor

rsc commented Jan 4, 2017

Keep the file name, but let's continue this discussion on #4899. Closing as dup of #4899.

@rsc rsc closed this as completed Jan 4, 2017
@cespare
Copy link
Contributor Author

cespare commented Jan 4, 2017

@rsc Will do.

I made a new issue because I didn't want to cannibalize #4899. Several alternatives were discussed in that issue; in theory, it seemed that this specific proposal could be rejected while the general need could remain to be solved by some other change.

@rsc
Copy link
Contributor

rsc commented Jan 4, 2017

@cespare Understood, but we already discussed the solution space there, so might as well keep at it. Especially if there are going to be other designs it helps to have everything in one place, since they will be compared against each other. I agree we don't have a lot of experience with this, but it seemed to work well that I did not create a new issue when 18130 transitioned from talking about the general problem to focusing on the specific type alias solution. Certainly not a big deal.

@golang golang locked and limited conversation to collaborators Jan 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants