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

wiki: CodeReviewComments should more thoroughly describe test helper functions #30246

Open
posener opened this issue Feb 15, 2019 · 2 comments

Comments

@posener
Copy link

commented Feb 15, 2019

The document already covers testing helper functions, and how to use them. I propose to add instructions how to write testing helper functions. Two things that I thought of are:

  1. The first argument should always be t *testing.T.
  2. The first line of the function should be t.Helper().

The reason for this issue is that currently context is instructed to be the first argument of a function. A testing helper function that I wrote testCtx(t *testing.T, ctx context.Context) result in a lint warning - context should always be the first argument. I opened golang/lint#422 and created a fix golang/lint#423, but it was declined for the reason that lint enforces only what's in EffectiveGo and CodeReviewComments.

The current state is that if I want to have this function I should declare it as testCtx(ctx context.Context, t *testing.T) which fills wrong to me.

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

2. The first line of the function should be t.Helper().

That isn't necessarily the case: sometimes you really do want to report line numbers from inside the helper-function rather than outside. (It largely depends on whether the function is implementing the body of the test, or just doing a bit of opaque setup.)

@katiehockman katiehockman changed the title wiki: CodeReviewComments: Add instructions for test helper functions wiki: CodeReviewComments should describe test helper function arguments Feb 15, 2019

@katiehockman katiehockman changed the title wiki: CodeReviewComments should describe test helper function arguments wiki: CodeReviewComments should more thoroughly describe test helper functions Feb 15, 2019

@katiehockman

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

  1. The first argument should always be t *testing.T.

I'm also not convinced of this one. The documentation for context indicates that context should almost always be the first argument. I interpret the "almost" to mean that the cases where you use a newly created context in the body of the function, rather than as a function param, are pretty rare. Not to mean there are cases where you pass a context as anything other than the first param. It is also a rare case that a test would need both the context and t *testing.T, so adding that contingency seems superfluous. The linter always expecting that context be the first argument seems preferable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.