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

Unit tests should use local client instead of package-global client #762

Closed
gmlewis opened this Issue Oct 26, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@gmlewis
Collaborator

gmlewis commented Oct 26, 2017

In #732 it became clear that having a package-global client for unit testing will prevent the unit tests from being run in parallel in the future. Ideally, each test will have its own client... something like this:

client, teardown := setup()
defer teardown()

and the global client in github_test.go will be removed.

This would make for a very large PR, but could possibly be done with the assistance of gofmt -r or your favorite text editor. 😄

This would also be a great project for a new contributor to the repo. Let it be known that all help is greatly appreciated! Thank you in advance.

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Oct 27, 2017

Contributor

I decided to have a go at this issue, sorry I forgot to comment here first. This was quite a nice one to do as a new contributor :)

Contributor

chriskingnet commented Oct 27, 2017

I decided to have a go at this issue, sorry I forgot to comment here first. This was quite a nice one to do as a new contributor :)

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 28, 2017

Member

@gmlewis I don't think it'll be sufficient to have setup return only client. I think it'll need to return server and mux as well. Otherwise, it's not sufficient to achieve the goal of removing global shared state between tests.

Edit: Actually, I think only mux will need to be returned by setup in addition to client. server won't be needed. server.Close is the only thing needed, and it's already a part of teardown.

But we can't reuse the package scoped server *httptest.Server, it needs to go away.

Member

dmitshur commented Oct 28, 2017

@gmlewis I don't think it'll be sufficient to have setup return only client. I think it'll need to return server and mux as well. Otherwise, it's not sufficient to achieve the goal of removing global shared state between tests.

Edit: Actually, I think only mux will need to be returned by setup in addition to client. server won't be needed. server.Close is the only thing needed, and it's already a part of teardown.

But we can't reuse the package scoped server *httptest.Server, it needs to go away.

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Oct 28, 2017

Contributor

Makes sense, thanks for input @shurcooL. From a quick check, there does actually seem to be a couple of tests using the global server in order to get its URL.

So in order to not change any of the tests, we would have setup() always return the four things (I like explicitly returning teardown rather than just having every test call server.Close):

client, mux, server, teardown := setup()

Or we could do something like (naming tbc):

client, mux, globalConfig, teardown := setup()

where global config contains configuration details that might be needed in tests - i.e. the server base URL value.

Any thoughts?

Contributor

chriskingnet commented Oct 28, 2017

Makes sense, thanks for input @shurcooL. From a quick check, there does actually seem to be a couple of tests using the global server in order to get its URL.

So in order to not change any of the tests, we would have setup() always return the four things (I like explicitly returning teardown rather than just having every test call server.Close):

client, mux, server, teardown := setup()

Or we could do something like (naming tbc):

client, mux, globalConfig, teardown := setup()

where global config contains configuration details that might be needed in tests - i.e. the server base URL value.

Any thoughts?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 28, 2017

Member

How about this:

server, mux, client := setup()
defer server.Close()

Since everything that teardown does fits on one line, I think we can just get rid of it.

Edit: I missed this thing you said:

I like explicitly returning teardown rather than just having every test call server.Close

Hmm...

Is the server base URL value the only thing used by tests? If so, maybe just return that url instead of globalConfig?

Member

dmitshur commented Oct 28, 2017

How about this:

server, mux, client := setup()
defer server.Close()

Since everything that teardown does fits on one line, I think we can just get rid of it.

Edit: I missed this thing you said:

I like explicitly returning teardown rather than just having every test call server.Close

Hmm...

Is the server base URL value the only thing used by tests? If so, maybe just return that url instead of globalConfig?

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Oct 28, 2017

Contributor

That's true. Although I quite liked that it is explicit, and that to "forget" to teardown I have to purposefully ignore the variable and use _, otherwise I will get an error for not using it. If we rely on calling server.Close(), I think that this step might be more easily missed.

That said, I imagine almost all tests written will just copy a previous one, which will have the code you mentioned in it. So in practice I'm not sure how much of a problem that is.

Contributor

chriskingnet commented Oct 28, 2017

That's true. Although I quite liked that it is explicit, and that to "forget" to teardown I have to purposefully ignore the variable and use _, otherwise I will get an error for not using it. If we rely on calling server.Close(), I think that this step might be more easily missed.

That said, I imagine almost all tests written will just copy a previous one, which will have the code you mentioned in it. So in practice I'm not sure how much of a problem that is.

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Oct 28, 2017

Contributor

Ah, sorry, I replied before I saw your updated comment!

Is the server base URL value the only thing used by tests? If so, maybe just return that url instead of globalConfig?

Yes, that sound like a good approach. So we're now at:

serverBaseURL, mux, client, teardown := setup()
defer teardown()

...giving the tests what they need and not over complicating when we only need that one value at the moment.

Contributor

chriskingnet commented Oct 28, 2017

Ah, sorry, I replied before I saw your updated comment!

Is the server base URL value the only thing used by tests? If so, maybe just return that url instead of globalConfig?

Yes, that sound like a good approach. So we're now at:

serverBaseURL, mux, client, teardown := setup()
defer teardown()

...giving the tests what they need and not over complicating when we only need that one value at the moment.

@dmitshur dmitshur closed this in df88dd9 Dec 2, 2017

nbareil pushed a commit to nbareil/go-github that referenced this issue May 1, 2018

Remove global test variables in favor of local. (google#766)
This refactor removes the global test variables used by all tests,
replacing them with local variables that are independent in each test.

This is better style, easier to read and be confident about correctness,
and allows tests to be parallelized in the future.

Resolves google#762.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment