tests/integration: Get rid of integration build constraint; skip tests instead.

[dmitshur]

We're currently using a build constraint "integration" to prevent tests/integration from running when go test ./... happens in CI (and locally). This is because integration tests are heavier-weight and meant to be run explicitly. They need some environment variables with tokens to be set, etc.

go-github/tests/integration/github_test.go

Line 6 in 23d6cb9

// +build integration

The problem with that is that the code in that package is not included when one does go list ./... (as opposed to go list -tags=integration ./...), so doing things like go build ./..., go vet ./..., staticcheck ./..., doesn't catch issues. As a result, the integration tests tend to get build errors (forgetting to update APIs), miss out on lint/vet/staticcheck fixes, etc.

I suggest we get rid of the build constraint and instead rely on skipping tests when necessary integration environment variables are not set (perhaps rename them to be more specific to go-github integration tests, rather than more generic, to avoid false matches).

Some example of this being done in Go standard library are:

• https://github.com/golang/go/blob/go1.7/src/cmd/go/go_test.go#L132
• https://github.com/golang/go/blob/go1.7/src/cmd/go/go_test.go#L1066
• https://github.com/golang/go/blob/go1.7/src/cmd/go/go_test.go#L2793-L2795

I don't see any downsides to doing that, are there any?

[willnorris]

In general, sounds fine to me, especially given how easy it is to overlook these when doing cleanups.

It's worth noting that we can't just rely on the existing environment vars. For example, the one you linked to requires the GITHUB_AUTH_TOKEN for some tests, but will still run others even if it's not present. So we may need an explicit INTEGRATION_TEST (or whatever) var. I want to be really careful about not sending real network traffic to the GitHub API unless the user has explicitly indicated they want to do that.

[dmitshur]

Yep, completely agreed. That's why I noted:

(perhaps rename them to be more specific to go-github integration tests, rather than more generic, to avoid false matches)

Checking for GITHUB_AUTH_TOKEN is not okay, because it's very generic and someone may have it inadvertently set (for another purpose). But checking for GOGITHUB_INTEGRATION_TEST_AUTH_TOKEN or something like that would be explicit enough.

[dmitshur]

The importance of this issue has been reduced with d4206f9.

[gmlewis]

Closing issue as obsolete. Feel free to reopen if I'm mistaken.