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.
  • For example, all assertions should use require/assert packages. Revisit tests #146 (comment)
• Introduce helpers, to eliminate more boilerplate.
  • For example, entry slices don't need to be copy-pasted many times, and can be generated with a simple function. tests: add helpers for generating entry slices #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?

• raft_test.go: 383
  • Test: Replace t.error/fatal with assert/request in [raft_test.go] #173
• rawnode_test.go: 97
  • Test: Replace t.error/fatal with assert/request in [rawnode_test.go] #177
• node_test.go: 65
  • Test: Replace t.error/fatal with assert/request in [node_test.go] #179
• raft_paper_test.go: 44
  • Test: Replace t.error/fatal with assert/request in [raft_paper_test.go] #182
• raft_snap_test.go: 11
  • Test: Replace t.error/fatal with assert/request in [raft_snap_test.go] #184
• raft_flow_control_test.go: 10
  • Test: Replace t.error/fatal with assert/request in [raft_flow_control_test.go] #185
• confchange/quick_test.go: 8
• confchange/restore_test.go: 4
  • Test: Replace t.error/fatal with assert/request in [/confchange] #186
• quorum/datadriven_test.go: 5
• quorum/quick_test.go: 1
  • Test: Replace t.error/fatal with assert/request in [/quorum] #187
• rafttest/node_test.go: 3
• rafttest/network_test.go: 2
  • Test: Replace t.error/fatal with assert/request in [/rafttest] #189
• tracker/inflights_test.go: 3
  • Test: Replace t.error/fatal with assert/request in [/tracker/inflights_test.go] #190
• raftpb/raft_test.go: 1
• raftpb/confstate_test.go: 1
  • Test: Replace t.error/fatal with assert/request in [/raftpb]  #194
• util_test.go: 1
  • Test: Replace t.error/fatal with assert/request in [util_test.go]  #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!