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

Removes global test client and calls setup for each client test #766

Merged
merged 16 commits into from Dec 2, 2017

Conversation

Projects
None yet
4 participants
@chriskingnet
Contributor

chriskingnet commented Oct 27, 2017

This is my attempt at removing the global client, as outlined in #762.

As I am a new contributor to this lib, I thought this might be a good issue to have a go at as suggested - as you get to see quite a lot of the test code. Also put my find and replace skills to the test! :D

This seemed to work, all the test keep running and it was achieved I think pretty much exactly as outlined in the issue.

@googlebot googlebot added the cla: yes label Oct 27, 2017

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 28, 2017

Member

This is a great start @chriskingnet, thanks! But see #762 (comment).

Member

dmitshur commented Oct 28, 2017

This is a great start @chriskingnet, thanks! But see #762 (comment).

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Oct 28, 2017

Contributor

Have made a few changes, based on the discussion from #762, and now also removed the mux and server objects from being package global.

Contributor

chriskingnet commented Oct 28, 2017

Have made a few changes, based on the discussion from #762, and now also removed the mux and server objects from being package global.

Show outdated Hide outdated CONTRIBUTING.md
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 31, 2017

Member

On the the one hand, I think it'd be nice if this PR actually made use of the fact that tests are independent and made them run in parallel. That is the reason to be making these changes; if we don't make tests parallel, these changes have no positive effect. Also, making the tests run in parallel may uncover unexpected issues, ones that might result in us deciding to abort this change. It'd better to find that out sooner rather than later.

On the other hand, we should merge this PR sooner, because it'll keep getting merge conflicts when other PRs get merged.

What do you think we should aim for, @gmlewis and @chriskingnet?

Member

dmitshur commented Oct 31, 2017

On the the one hand, I think it'd be nice if this PR actually made use of the fact that tests are independent and made them run in parallel. That is the reason to be making these changes; if we don't make tests parallel, these changes have no positive effect. Also, making the tests run in parallel may uncover unexpected issues, ones that might result in us deciding to abort this change. It'd better to find that out sooner rather than later.

On the other hand, we should merge this PR sooner, because it'll keep getting merge conflicts when other PRs get merged.

What do you think we should aim for, @gmlewis and @chriskingnet?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 31, 2017

Member

Ok, I see the original motivation for doing this in #732 (comment)...

So even if making tests in parallel won't work, it's still worth to make the tests not share globals. In that case, I'm okay with this PR not making tests run in parallel, that can be a separate PR.

Member

dmitshur commented Oct 31, 2017

Ok, I see the original motivation for doing this in #732 (comment)...

So even if making tests in parallel won't work, it's still worth to make the tests not share globals. In that case, I'm okay with this PR not making tests run in parallel, that can be a separate PR.

chriskingnet added some commits Oct 31, 2017

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Oct 31, 2017

Contributor

Thanks for the comments, @shurcooL. I agree with the two step approach here - first remove the dependencies on the global variables, and then do the work to parallelise the tests.

I am not sure what is going on here, though, I have tried to merge and resolve the conflicts, but it is still reporting them. I must be doing something wrong.

Contributor

chriskingnet commented Oct 31, 2017

Thanks for the comments, @shurcooL. I agree with the two step approach here - first remove the dependencies on the global variables, and then do the work to parallelise the tests.

I am not sure what is going on here, though, I have tried to merge and resolve the conflicts, but it is still reporting them. I must be doing something wrong.

@gmlewis

This is looking good, @chriskingnet! Thank you for doing this.
Just a few things requested in order to better document the new-and-improved setup function and its return values, please.

@@ -42,9 +31,9 @@ const (
// setup sets up a test HTTP server along with a github.Client that is
// configured to talk to that test server. Tests should register handlers on
// mux which provide mock responses for the API method being tested.

This comment has been minimized.

@gmlewis

gmlewis Oct 31, 2017

Collaborator

I would like to capture the removed comments from lines 26, 29, 32, and 73 into the description of setup so that it is clear what the purpose of the return values are.

@gmlewis

gmlewis Oct 31, 2017

Collaborator

I would like to capture the removed comments from lines 26, 29, 32, and 73 into the description of setup so that it is clear what the purpose of the return values are.

This comment has been minimized.

@chriskingnet

chriskingnet Nov 16, 2017

Contributor

Thanks for spotting, @gmlewis. Have re-inserted in the new places they are built :)

@chriskingnet

chriskingnet Nov 16, 2017

Contributor

Thanks for spotting, @gmlewis. Have re-inserted in the new places they are built :)

Show outdated Hide outdated CONTRIBUTING.md
@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Nov 16, 2017

Contributor

Sorry for the delay on this. I have now merged master, fixed the newly added tests and reverted all comments that were caught in the find-replace.

Contributor

chriskingnet commented Nov 16, 2017

Sorry for the delay on this. I have now merged master, fixed the newly added tests and reverted all comments that were caught in the find-replace.

@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Nov 18, 2017

Contributor

Have named return values. Good idea, thanks @gmlewis!

Contributor

chriskingnet commented Nov 18, 2017

Have named return values. Good idea, thanks @gmlewis!

// server is a test HTTP server used to provide mock API responses.
server *httptest.Server
)

This comment has been minimized.

@dmitshur

dmitshur Nov 22, 2017

Member

👌

@dmitshur
@dmitshur

dmitshur requested changes Nov 22, 2017 edited

I have one major change request (and 2 minor nits, see above), but everything else looks great.

You've addressed Glenn's earlier concerns, so I think I can merge this after you apply these changes.

Thanks again for taking this on @chriskingnet!

Show outdated Hide outdated github/github_test.go
@chriskingnet

This comment has been minimized.

Show comment
Hide comment
@chriskingnet

chriskingnet Dec 2, 2017

Contributor

Hi @shurcooL, I have merged master and I think I have changed everything. Sorry it took a few days for me to get around from it. I'm pleased with how this looks now - this parameter ordering is much better. Let me know if there is anything else you spot :)

Contributor

chriskingnet commented Dec 2, 2017

Hi @shurcooL, I have merged master and I think I have changed everything. Sorry it took a few days for me to get around from it. I'm pleased with how this looks now - this parameter ordering is much better. Let me know if there is anything else you spot :)

@dmitshur

LGTM. This looks really nice. Thank you for doing this @chriskingnet and @gmlewis for the idea!

@dmitshur dmitshur merged commit df88dd9 into google:master Dec 2, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

nbareil pushed a commit to nbareil/go-github that referenced this pull request 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