table tests: make guidance less prescriptive (#174)

Author: tyler-french

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 2023-05-09
+
+- Test tables: Discourage tables with complex, branching test bodies.
+
 # 2023-04-13
 
 - Errors: Add guidance on handling errors only once.

src/test-table.md

@@ -1,7 +1,11 @@
 # Test Tables
 
-Use table-driven tests with [subtests] to avoid duplicating code when the core
-test logic is repetitive.
+Table-driven tests with [subtests] can be a helpful pattern for writing tests
+to avoid duplicating code when the core test logic is repetitive.
+
+If a system under test needs to be tested against _multiple conditions_ where
+certain parts of the the inputs and outputs change, a table-driven test should
+be used to reduce redundancy and improve readability.
 
   [subtests]: https://blog.golang.org/subtests
 
@@ -100,6 +104,136 @@ for _, tt := range tests {
 }
 ```
 
+## Avoid Unnecessary Complexity in Table Tests
+
+Table tests can be difficult to read and maintain if the subtests contain conditional
+assertions or other branching logic. Table tests should **NOT** be used whenever
+there needs to be complex or conditional logic inside subtests (i.e. complex logic inside the `for` loop).
+
+Large, complex table tests harm readability and maintainability because test readers may
+have difficulty debugging test failures that occur.
+
+Table tests like this should be split into either multiple test tables or multiple
+individual `Test...` functions.
+
+Some ideals to aim for are:
+
+* Focus on the narrowest unit of behavior
+* Minimize "test depth", and avoid conditional assertions (see below)
+* Ensure that all table fields are used in all tests
+* Ensure that all test logic runs for all table cases
+
+In this context, "test depth" means "within a given test, the number of
+successive assertions that require previous assertions to hold" (similar
+to cyclomatic complexity).
+Having "shallower" tests means that there are fewer relationships between
+assertions and, more importantly, that those assertions are less likely
+to be conditional by default.
+
+Concretely, table tests can become confusing and difficult to read if they use multiple branching
+pathways (e.g. `shouldError`, `expectCall`, etc.), use many `if` statements for
+specific mock expectations (e.g. `shouldCallFoo`), or place functions inside the
+table (e.g. `setupMocks func(*FooMock)`).
+
+However, when testing behavior that only
+changes based on changed input, it may be preferable to group similar cases
+together in a table test to better illustrate how behavior changes across all inputs,
+rather than splitting otherwise comparable units into separate tests
+and making them harder to compare and contrast.
+
+If the test body is short and straightforward,
+it's acceptable to have a single branching pathway for success versus failure cases
+with a table field like `shouldErr` to specify error expectations.
+
+<table>
+<thead><tr><th>Bad</th><th>Good</th></tr></thead>
+<tbody>
+<tr><td>
+
+```go
+func TestComplicatedTable(t *testing.T) {
+	tests := []struct {
+		give          string
+		want          string
+		wantErr       error
+		shouldCallX   bool
+		shouldCallY   bool
+		giveXResponse string
+		giveXErr      error
+		giveYResponse string
+		giveYErr      error
+	}{
+		// ...
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.give, func(t *testing.T) {
+			// setup mocks
+			ctrl := gomock.NewController(t)
+			xMock := xmock.NewMockX(ctrl)
+			if tt.shouldCallX {
+				xMock.EXPECT().Call().Return(tt.giveXResponse, tt.giveXErr)
+			}
+			yMock := ymock.NewMockY(ctrl)
+			if tt.shouldCallY {
+				yMock.EXPECT().Call().Return(tt.giveYResponse, tt.giveYErr)
+			}
+
+			got, err := DoComplexThing(tt.give, xMock, yMock)
+
+			// verify results
+			if tt.wantErr != nil {
+				require.EqualError(t, err, tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, want, got)
+		})
+	}
+}
+```
+
+</td><td>
+
+```go
+func TestShouldCallX(t *testing.T) {
+	// setup mocks
+	ctrl := gomock.NewController(t)
+	xMock := xmock.NewMockX(ctrl)
+	xMock.EXPECT().Call().Return("XResponse", nil)
+
+	yMock := ymock.NewMockY(ctrl)
+
+	got, err := DoComplexThing("inputX", xMock, yMock)
+
+	require.NoError(t, err)
+	assert.Equal(t, "want", got)
+}
+
+func TestShouldCallYAndFail(t *testing.T) {
+	// setup mocks
+	ctrl := gomock.NewController(t)
+	xMock := xmock.NewMockX(ctrl)
+
+	yMock := ymock.NewMockY(ctrl)
+	yMock.EXPECT().Call().Return("YResponse", nil)
+
+	_, err := DoComplexThing("inputY", xMock, yMock)
+	assert.EqualError(t, err, "Y failed")
+}
+```
+</td></tr>
+</tbody></table>
+
+This complexity makes it more difficult to change, understand, and prove the
+correctness of the test.
+
+While there are no strict guidelines, readability and maintainability should
+always be top-of-mind when deciding between Table Tests versus separate tests
+for multiple inputs/outputs to a system.
+
+## Parallel Tests
+
 Parallel tests, like some specialized loops (for example, those that spawn
 goroutines or capture references as part of the loop body),
 must take care to explicitly assign loop variables within the loop's scope to

style.md

@@ -3588,8 +3588,12 @@ See also [go vet: Printf family check](https://kuzminva.wordpress.com/2017/11/07
 
 ### Test Tables
 
-Use table-driven tests with [subtests](https://blog.golang.org/subtests) to avoid duplicating code when the core
-test logic is repetitive.
+Table-driven tests with [subtests](https://blog.golang.org/subtests) can be a helpful pattern for writing tests
+to avoid duplicating code when the core test logic is repetitive.
+
+If a system under test needs to be tested against *multiple conditions* where
+certain parts of the the inputs and outputs change, a table-driven test should
+be used to reduce redundancy and improve readability.
 
 <table>
 <thead><tr><th>Bad</th><th>Good</th></tr></thead>
@@ -3686,6 +3690,136 @@ for _, tt := range tests {
 }
 ```
 
+#### Avoid Unnecessary Complexity in Table Tests
+
+Table tests can be difficult to read and maintain if the subtests contain conditional
+assertions or other branching logic. Table tests should **NOT** be used whenever
+there needs to be complex or conditional logic inside subtests (i.e. complex logic inside the `for` loop).
+
+Large, complex table tests harm readability and maintainability because test readers may
+have difficulty debugging test failures that occur.
+
+Table tests like this should be split into either multiple test tables or multiple
+individual `Test...` functions.
+
+Some ideals to aim for are:
+
+* Focus on the narrowest unit of behavior
+* Minimize "test depth", and avoid conditional assertions (see below)
+* Ensure that all table fields are used in all tests
+* Ensure that all test logic runs for all table cases
+
+In this context, "test depth" means "within a given test, the number of
+successive assertions that require previous assertions to hold" (similar
+to cyclomatic complexity).
+Having "shallower" tests means that there are fewer relationships between
+assertions and, more importantly, that those assertions are less likely
+to be conditional by default.
+
+Concretely, table tests can become confusing and difficult to read if they use multiple branching
+pathways (e.g. `shouldError`, `expectCall`, etc.), use many `if` statements for
+specific mock expectations (e.g. `shouldCallFoo`), or place functions inside the
+table (e.g. `setupMocks func(*FooMock)`).
+
+However, when testing behavior that only
+changes based on changed input, it may be preferable to group similar cases
+together in a table test to better illustrate how behavior changes across all inputs,
+rather than splitting otherwise comparable units into separate tests
+and making them harder to compare and contrast.
+
+If the test body is short and straightforward,
+it's acceptable to have a single branching pathway for success versus failure cases
+with a table field like `shouldErr` to specify error expectations.
+
+<table>
+<thead><tr><th>Bad</th><th>Good</th></tr></thead>
+<tbody>
+<tr><td>
+
+```go
+func TestComplicatedTable(t *testing.T) {
+	tests := []struct {
+		give          string
+		want          string
+		wantErr       error
+		shouldCallX   bool
+		shouldCallY   bool
+		giveXResponse string
+		giveXErr      error
+		giveYResponse string
+		giveYErr      error
+	}{
+		// ...
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.give, func(t *testing.T) {
+			// setup mocks
+			ctrl := gomock.NewController(t)
+			xMock := xmock.NewMockX(ctrl)
+			if tt.shouldCallX {
+				xMock.EXPECT().Call().Return(tt.giveXResponse, tt.giveXErr)
+			}
+			yMock := ymock.NewMockY(ctrl)
+			if tt.shouldCallY {
+				yMock.EXPECT().Call().Return(tt.giveYResponse, tt.giveYErr)
+			}
+
+			got, err := DoComplexThing(tt.give, xMock, yMock)
+
+			// verify results
+			if tt.wantErr != nil {
+				require.EqualError(t, err, tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, want, got)
+		})
+	}
+}
+```
+
+</td><td>
+
+```go
+func TestShouldCallX(t *testing.T) {
+	// setup mocks
+	ctrl := gomock.NewController(t)
+	xMock := xmock.NewMockX(ctrl)
+	xMock.EXPECT().Call().Return("XResponse", nil)
+
+	yMock := ymock.NewMockY(ctrl)
+
+	got, err := DoComplexThing("inputX", xMock, yMock)
+
+	require.NoError(t, err)
+	assert.Equal(t, "want", got)
+}
+
+func TestShouldCallYAndFail(t *testing.T) {
+	// setup mocks
+	ctrl := gomock.NewController(t)
+	xMock := xmock.NewMockX(ctrl)
+
+	yMock := ymock.NewMockY(ctrl)
+	yMock.EXPECT().Call().Return("YResponse", nil)
+
+	_, err := DoComplexThing("inputY", xMock, yMock)
+	assert.EqualError(t, err, "Y failed")
+}
+```
+</td></tr>
+</tbody></table>
+
+This complexity makes it more difficult to change, understand, and prove the
+correctness of the test.
+
+While there are no strict guidelines, readability and maintainability should
+always be top-of-mind when deciding between Table Tests versus separate tests
+for multiple inputs/outputs to a system.
+
+#### Parallel Tests
+
 Parallel tests, like some specialized loops (for example, those that spawn
 goroutines or capture references as part of the loop body),
 must take care to explicitly assign loop variables within the loop's scope to