Developing on the our Team

This document is a collection of team preferences and agreements. It can be updated at any time (PRs welcome) when opinions change. The advice below will not apply to all situations. Please use your best judgement and ignore these preferences when it makes sense.

1. Creating a Pull Request (PR)

1.1 MUST ensure ownership of the feature

Each developer is responsible for their feature, from the first commit to testing it in production. The developer is responsible for adding tests, reaching out for PR reviews in Slack, testing in staging, and testing in production after it is released.

1.2 SHOULD NOT leave undeployed code on main for other teammates.

Leaving undeployed code on the main branch introduces risk. The developer who owns a commit is the best person to debug any issues that may occur in production and the best time to debug code is as close to when it was written as possible.

One common exception to this rule is letting changes soak in stage2. In this case, the developer should post in Slack to ensure no one else on the team accidentally deploys the changes to production.

1.3 SHOULD prefer small PRs

The larger a PR, the less likely someone will catch all nuances of it. Prefer small PRs over large PRs. If a PR is longer than 1000 lines changed, it should be broken up.

1.4 SHOULD improve test coverage

As a team, we strive to leave a code base better than it was before we touched it. Pull requests should improve the test coverage when possible. A feature being difficult to unit test is a code smell. If testing is difficult, the developer should investigate restructuring his code to make it easier to test.

1.5 SHOULD include testing instructions on the PR

Developers should include instructions to test the PR. Did you use curl or grpcurl? Assume a dedicated reviewer will pull the branch and test it locally. How would they do that? Try to provide any related context the reviewers might need to understand the PR.

1.6 SHOULD get feedback early

Large features should have a draft PR to get feedback as early as possible. When posting the draft PR, include a list of outstanding todos to help the reviewers.

1.7 SHOULD use Github's suggest feature for small changes

For small changes, especially typos, we can reduce the feedback loop by using Github's suggest feature.

1.8 reviewers SHOULD avoid personal preferences in feedback

Unless the reviewer can link to the style guide rule being violated, avoid personal preferences in PR feedback. Reviewers need to accept other developer's solutions/styles and should never block a PR over nit-picky feedback. Unless you can point to a specific rule in the style guide, stick avoid style comments.

2. Coding Style

2.1 SHOULD leverage existing style guides

2.2 SHOULD use consistent whitespace and remove trailing whitespace

Don't mix tabs and spaces for indentation. Update your editor to remove trailing whitespace on save.

2.3 SHOULD list imports in a consistent order

Imports be broken up into three sections separated by new lines for standart library imports, DO imports, and third-party imports. The team has decided that the order for imports should be as follows.

import (
  stdlib imports

  third-party imports
  
  project imports
)

2.4 Rules should be enforced via CI

All required styles should be enforced via a CI or linter. Developers should setup their IDE to lint on save.

3. Testing

3.1 SHOULD use table tests

Developers should write table tests for new code. Table tests provide the following benefits.

Setup of the test is separated from the test cases making the test conditions more obvious to developers.
Updating the tests is easier when the code structure changes. If a new variable is added to a method, the tests are changed in one spot instead of multiple funcs.
The simplicity of adding new test cases makes it easier for developers to include more edge cases in their unit tests.

3.2 SHOULD use standardized variable names in all tests

The tests should use the same variable names in tests. For example, the array of tests cases variable should be named tests. This will improve navigating and modifying tests in code.

tests := []struct {
  name     string
  setup    func(sqlmock.Sqlmock)
  expected struct{}
  err      error
}

for _, tt := range tests {
	t.Run(tt.name, func(t *testing.T) {
    tt := tt
		t.Parallel() // if applicable

	})
}

3.3 SHOULD use github.com/stretchr/testify/require to assert preconditions

https://pkg.go.dev/github.com/stretchr/testify/require

When setting up the test, use testify/require to assert certain conditions such as require.NoError(t, err) or require.NotNil(t, struct). When a require assertion fails, the tests will immediately exit.

3.4 SHOULD use github.com/stretchr/testify/assert to assert test conditions

https://pkg.go.dev/github.com/stretchr/testify/assert

Use testify/assert to have more specific assertions. There are assert methods for comparing JSON, that two arrays have the same elements, and many more.

3.5 SHOULD run table tests in parallel

Increase test througput by enabling table tests to run in parallel.

for _, tt := range tests {
	t.Run(tt.name, func(t *testing.T) {
    tt := tt     // capture the pointer to the test case within this scope
		t.Parallel() // signal that this test is ready to be run in parallel with other tests

	})
}

3.6 SHOULD override *http.Client.Transport with a custom round tripper to test http requests

First, functions that need to make http requests should share the same http.Client struct. http.Client is goroutine safe and will reuse connections if possible. Create your http.Client in your main.go, add your service's user agent to it, and pass it as a dependency to all of your packages.

When testing code that makes an http request, override the Transport field of your http.Client with a custom RoundTripper. This allows you to mock responses without having to run a httptest.Server.

4. Project Layout

https://github.com/golang-standards/project-layout

4.1 SHOULD put main.go in the cmd/<binary-name> directory

Put the binaries in the cmd directory under the binary's name. By default go build will compile main.go into a binary with the same name as the folder it is in. It is incredibly likely that your code base will have multiple binaries, such as a ctl or docc job.

cmd/<server-binary>/main.go
   /<command-line-tool>/main.go
   /<job-binary>/main.go

4.2 SHOULD put config.go in the cmd/<binary-name> directory

A code smell is passing your config struct to your dependencies. Ensure that packages can't import config types by putting it in the same directory/package as your main.go

4.3 SHOULD include standard make tasks

build        - Build the binary on the local system.
clean        - Remove the build directory and any other build artifacts.
coverage     - Run go test with coverage enabled.
docker-build - Build the binary in the same docker container it. This is used by docker-run an docker-push.
docker-deps  - Start the docker-compose dependencies in the background
docker-push  - Build the docker file and push the image to the docker registry.
docker-run   - Build and execute the docker binary. This is the equivalent of `make docker-build && docker-compose up`
docker-stop  - The equivalent of docker-compose stop.
lint         - Execute gofmt and Go's staticcheck tool.
gen          - Run Go generators. This should rebuild any mocks.
run          - Equivalent of `go run`.
test         - Execute tests in the current directory and all subdirectories.

countingtoten/Team Agreements.md