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: separate integration and unit tests #30595

Closed
nim-nim opened this issue Mar 5, 2019 · 14 comments
Closed

proposal: testing: separate integration and unit tests #30595

nim-nim opened this issue Mar 5, 2019 · 14 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Milestone

Comments

@nim-nim
Copy link

nim-nim commented Mar 5, 2019

$ go version
go version go1.11 linux/amd64

Right now go mixes integration and unit tests in a single category.

Unfortunately they are not the same: a unit test is something that only needs the go compiler to run, and an integration test needs some form of specific environment (running system daemon, database, working network, server, cloud, credentials, etc) to be executed.

That severely limits the ability of third parties to check existing Go codebases, since after the nth test that can not be run because of specific constrains, you just tend to give up, disable them all and pray for the best.

Separating unit and integration tests would incite third parties to actually run unit tests and would improve testing coverage, since third parties have often access to a wider range of systems and architectures than the original project authors.

@mvdan
Copy link
Member

mvdan commented Mar 5, 2019

There are many ways to do this; for example, see https://stackoverflow.com/questions/25965584/separating-unit-tests-and-integration-tests-in-go.

I personally prefer a mix of testing.T.Skip and testing.Short. That way, -short doesn't run integration or slow tests, and the integration tests are skipped when the external component isn't available.

Like in the other issues, if you have a particular enhancement in mind, please file a proposal; this reads more like a complaint than a suggestion for a particular change to the testing package.

@bcmills
Copy link
Contributor

bcmills commented Mar 5, 2019

There is a broad spectrum between “integration” and “unit” tests. Ideally, tests should start their own (hermetic) dependency services, and then you can run the tests even in a completely isolated environment. (Does that make them “unit tests”? I don't know, and I don't think the distinction is particularly meaningful.)

When that isn't possible, it is still usually possible for tests to probe for the existence of dependencies and call t.Skip if they are missing.

@andybons
Copy link
Member

andybons commented Mar 9, 2019

@nim-nim what do you propose we do (concretely) as a solution to your problem?

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2019
@andybons andybons added this to the Unplanned milestone Mar 9, 2019
@andybons andybons changed the title go needs to separate integration and unit tests proposal: testing: separate integration and unit tests Mar 9, 2019
@andybons andybons added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed Proposal labels Mar 9, 2019
@nim-nim
Copy link
Author

nim-nim commented Mar 12, 2019

@bcmills

Yes there is a broad spectrum of tests, but practically there are only two cases:

  • things which are safe to run by third-parties as-is (the test only needs the compiler, and if it needs something else like network, server of whatever it tests for the existence of this something else and PASSES if not available)
  • things which are not safe and need special preparation/handling for the result to be meaningful (that will generate spurious FAILS if the preparation/handling/test env is not done right)

It would be very useful to have the ability to mark those two categories of tests in a different way (via naming conventions like _integration_test postfixes, source annotations, whatever) so go list could put both in separate buckets and one could run something like go test all -safe by default, knowing its result is meaningful and does not require sorting out integration tests first.

Right now when a test run is polluted by integration things, and upstream does not want to bother testing for the things the integration test needs, you have no way to ask them to just mark it as integration so everyone is happy and plays nice with the others

@ianlancetaylor
Copy link
Contributor

In my opinion, tests that require special handling should define their own flag and should only be run if that flag is passed. If the flag is not passed, they should call t.Skip.

I don't see a reason to handle this explicitly in the testing package. There are too many different requirements to satisfy.

@nim-nim
Copy link
Author

nim-nim commented Mar 12, 2019

But test writers are not that careful in the real world. They don't want to code all that testing (in their own setup, integration tests just work).

They need a cheap standard way to mark "this test is not careful about its requisites and I can't be bothered to fix it"

@ianlancetaylor
Copy link
Contributor

Adding and checking a flag is literally four lines of code, and one of those lines is }.

Adding functionality to the testing package is just more API that very few people will use.

@mvdan
Copy link
Member

mvdan commented Mar 12, 2019

But test writers are not that careful in the real world.

Writing bad tests is certainly possible, but adding more API to the testing package is unlikely to improve that situation. If someone doesn't bother to write good tests today, they're even less likely to take the time if we make the testing package more complex.

@agnivade agnivade removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 12, 2019
@nim-nim
Copy link
Author

nim-nim commented Mar 12, 2019

A single unified way of marking unsafe tests is way more likely to be adopted in the real world that something that requires projects to invent one-of-a-kind flags.

A single unified way of marking unsafe tests can be used in code inspection tools like go list or go/packages while a bit of coding to add a custom flag is just a bit of coding for analysis tools

And so on

@beoran
Copy link

beoran commented Mar 15, 2019

You can solve this problem with Go as it is now if you follow the following simple convention.

Put all your integration tests into seperate files, named XXX_integration_test.go, and then add //+build integration on top of these files. Then, to run your normal tests, do go test, and your integration tests will not be run thanks to the build tag. To run all tests including integration, you can, then do go test -v -tags integration.

As you can see build tags are very useful to solve problems like these. Therefore I'd say Go needs no new mechanism to handle your use case.

@nim-nim
Copy link
Author

nim-nim commented Mar 15, 2019

@beoran
You solution only works for your own code. So either you never use someone else's code, or you use it blindly without executing its unit tests.

To be able to use third-party code safely (testing it) you need a language-level standard/convention/whatever to separate safe tests from the rest, not everyone-invents-his-own.

@mvdan
Copy link
Member

mvdan commented Mar 15, 2019

Like @ianlancetaylor, doing this properly with a test flag and t.Skip only takes four lines of code. If that's too much for test authors, I struggle to see what we could change in the testing package to have them switch over to.

In any case, please answer @andybons's question. If this is a proposal to make a change to the testing package, please clearly outline what change you're proposing. "Separate integration tests" or "improve support for integration tests" are not specific proposals that we can evaluate.

@beoran
Copy link

beoran commented Mar 15, 2019

I do run tests, but in my experience, for most Go projects, integration tests are in a separate package, so I don't run into the problem.

If I may, a concrete proposal could be to make go test build any XXX_integration_test.go as if it had a //build:integration on top, and have an additional switch go test -integration that will run the integration tests also. Such a change would be largely backwards-compatible, I assume not many people are already using this kind of naming for their integration tests.

Of course, even if go test adds such a convention, then still we need to make people actually use it which will take time. There is no quick fix.

@ianlancetaylor
Copy link
Contributor

We aren't going to make any standard change here, per @golang/proposal-review . Perhaps conventions can develop over time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Projects
None yet
Development

No branches or pull requests

8 participants