proposal: table test guidance is too prescriptive

[tyler-french]

Table tests are great for reducing repetition, but as the SUT becomes more complex, they become harder to read, maintain, and debug.

Making tests harder to read and maintain at scale negatively impacts velocity, productivity, and overall sentiment on testing.

One common pattern I notice is with logical branches based on variables like shouldError, shouldCallExecutor, shouldDoXYZ. This creates an extremely difficult-to-read set of tests. As an exmple, consider the following test:

tests := []struct{
  give     string
  wantHost string
  wantPort string
  wantErr error
  shouldCallXYZ bool
  giveXYZResponse string
}{
  // ...
}

for _, tt := range tests {
  t.Run( ... ) {
    err := DoThing(tt.stuff)
    if tt.wantErr != nil {
      require.EqualError(t, err, tt.wantErr)
    }
    require.NoError(t, err)
    if tt.shouldCallXYZ{
      xyzMock.EXPECT().Call(...).Return(tt.giveXYZResponse, nil)
    }
  })
}

These tests are confusing. It's hard to tell what's going to be checked in which conditions, and unclear when certain fields of the tests struct are even used. The reader has to reference logic within the tests structs, as well as the complex behavioral logic within the test loop to understand what's happening.

Consider again that I need to add another subsystem to the service that has a mock which is called only some of the time. This adds another layer of complexity to the table test, and potentially obstructs all previously written parts of the table.

Having the guidance push to use Table Tests for "repeated patterns" actually can negatively impact code quality. Rather than recommending Table Tests for such patterns, we should recommend using Table tests for situations when ONLY the inputs and outputs of the system differ. It would also be good to recommend to AVOID Table Tests with logic existing inside the test loop.

Aside:
Somewhat unrelated to style, but it's also harder to maintain tests at scale with the dynamic assignment of subtest names. Splitting up table tests more granularly into separate TestXXX functions makes it easier to keep track of the speed and reliability of tests.

[xytan0056]

+1
https://testing.googleblog.com/2019/12/testing-on-toilet-tests-too-dry-make.html

[abhinav]

Agreed! This came up in #167 as well.

The high-level guidance that some of us discussed in the past includes:

Keep table tests simple. Don't add complex, branching pathways.
Don't write complex code inside table tests (e.g., a setupMocks func(*MockFoo) field in the table).
The test code of a table test should be straightforward and easy to understand.
If a reader has to keep jumping between a test case and the test body to understand the flow of the test, that is not a good fit for a table test.

We're completely in agreement on this.
If you'd like to take a stab at updating the test tables guidance, I'll be happy to provide a review.
However, note that we can't just say "separate the error and non error tables" because if the input/output contract is simple enough, it's fine (and sometimes preferable) to stick the error cases next to the success cases, like "here's valid input and here's the same input slightly invalid".

CC @mway @prashantv since we were discussing this recently.

A separate guidance we've discussed similar to DRY vs DAMP tests is:
test functions should largely be independent of each other.
It's okay to have duplication between test functions if that makes the test code stand alone.
e.g. don't have a single shared "setup state" function where only some of the state is used by test A, and only some of the state by test B. set up each test's state independently.
Haven't had a chance to formulate that into anything more concrete.

[peterbourgon]

+1 to @abhinav. A specific func TestWhatever should, in general, not include complex or branching code paths that would make table-based testing difficult. Parameters like shouldDoXYZ are pretty clearly red flags that should be caught, and eliminated, during code review.