hashicorp / hashicorp/consul

Use separate packages for production code and test helpers

Open
#12,194 0 comments 0 reactions 0 assignees View on GitHub
theme/testing
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

There are a few packages in Consul that provide test helpers along with the production code. These test helpers are generally for creating fixtures for setting up test state. The test helper functions often start with `Test`.

It can be nice to provide these to other packages. Although sometimes they are probably better kept as internal-only, because each package likely has different needs. Providing a standard set of fixtures increases the chance that all tests use the same structure of data, and miss testing an important permutation.

Including these test helpers in the same package as production code makes it a lot more difficult for the reader to see the important interface provided by the package. The test helpers can often crowd out the real production code and make it difficult to find in godoc.

This issue proposes the following changes:
1. For any test helpers that are used across packages, move them to a separate test-only package. The stdlib has examples of this: https://pkg.go.dev/testing#section-directories, and https://pkg.go.dev/net/http/httptest
2. For any test helpers that are not used across packaged, un-export them
3. For all test helpers, stop naming them `Test` ! This naming convention is confusing (because it collides with the names of actual test cases) and does read well. `NewTest` can work if it's a constructor, but if we include `test` in the package name, we shouldn't even need that. Often `Fake` or some other descriptor is better than `Test`.

Some packages that would benefit from this change are:
* `agent/proxycfg` - I attempted this change in #9465, but the PR rotted
* `agent/testagent.go`
* `agent/xds`
* etc

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the examples named in agent/proxycfg, agent/testagent.go, and agent/xds, and inspect the prior attempt in PR #9465. Determine which helpers are shared across packages versus local; done means shared helpers are in separate test-only packages, local helpers are unexported, and helper names no longer use the confusing Test prefix.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing-qa, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.