Revisit tests

[pav-kv]

This is an umbrella issue for all kinds of unit-test improvements. Unit-tests in this repository are significantly outdated, and need a revamp. For example:

• raft_test.go
• rawnode_test.go

Examples of things to fix/improve:

• [ ] Fix incorrect initialization of nodes, make a standard way to do this. Some tests change RawNode / raft structs arbitrarily, which breaks internal invariants and makes tests prone to false failures on any change. This necessitates PR authors to spend time debugging and fixing incorrect tests.
• [ ] Eliminate boilerplate.
  • [x] For example, all assertions should use require/assert packages. https://github.com/etcd-io/raft/issues/146#issuecomment-1972343245
• [ ] Introduce helpers, to eliminate more boilerplate.
  • [x] For example, entry slices don't need to be copy-pasted many times, and can be generated with a simple function. #147
• [ ] Introduce test-only invariant checks that happen transparently in all tests. An example invariant: r.Term >= r.raftLog.lastTerm(). Tests that manipulate RawNode structs directly may accidentally break invariants, and end up testing arbitrary behaviour rather than real one.

[MrDXY]

Hi @pav-kv, For the code you mentioned in "Eliminate Boilerplate", I am interested in improving it and have the time to do so. If I understand correctly, the following code

if sm.state != tt.wstate {
        t.Errorf("#%d.%d state = %v , want %v", i, j, sm.state, tt.wstate)
}

Should it be changed into something like this?

assert.Equal(t, tt.wstate, sm.state, "#%d.%d state = %v , want %v", i, j, sm.state, tt.wstate)

If you were expecting that, I would like to work on this as a starting point to contribute to etcd.

[pav-kv]

@MrDXY Yes, the idea is right. Feel free to send a PR.

In most cases we don't need the formatted text part. It's enough to do this:

assert.Equal(t, tt.wstate, sm.state)

There are some cases in which we need to add some information to the error, to identify the sub-test that failed. The example you quoted would then be like:

assert.Equal(t, tt.wstate, sm.state, "#%d.%d", i, j)

Also, t.Fatalf should correspond to require package calls, and t.Errorf should correspond to assert package calls.

[MrDXY]

Noted. Thanks! I'll give it a try.

[MrDXY]

Hi, @pav-kv I found multiple files that use t.Error/Fatal. While not all the files require a refactor, I am listing them for awareness. I plan to begin with raft_test.go, and will split this work into multiple PRs to make it easy to review. Would that be acceptable to you?

• [x] raft_test.go: 383

  • 173

• [x] rawnode_test.go: 97

  • 177

• [x] node_test.go: 65

  • 179

• [x] raft_paper_test.go: 44

  • 182

• [x] raft_snap_test.go: 11

  • 184

• [x] raft_flow_control_test.go: 10

  • 185

• [x] confchange/quick_test.go: 8
• [x] confchange/restore_test.go: 4

  • 186

• [x] quorum/datadriven_test.go: 5
• [x] quorum/quick_test.go: 1

  • 187

• [x] rafttest/node_test.go: 3
• [x] rafttest/network_test.go: 2

  • 189

• [x] tracker/inflights_test.go: 3

  • 190

• [x] raftpb/raft_test.go: 1
• [x] raftpb/confstate_test.go: 1

  • 194

• [x] util_test.go: 1

  • 196

• [ ] ~node_util_test.go: 7~

[pav-kv]

@MrDXY Please don't reference this issue from individual commits. This spams the history of this issue (see above: every time the commit is updated, it posts a new link to this issue). Instead, please reference the issue only from the PR description.

[MrDXY]

Hey @pav-kv , it's been a while since I was on this. IMHO, I think the node_util_test.go file doesn't need refactoring. If you agree, we can consider the refactor job for require/assert complete. What do you think? 😄

[pav-kv]

@MrDXY SGTM. Thanks a lot for this clean-up!