Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit tests #146

Open
2 of 6 tasks
pav-kv opened this issue Jan 31, 2024 · 7 comments
Open
2 of 6 tasks

Revisit tests #146

pav-kv opened this issue Jan 31, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@pav-kv
Copy link
Contributor

pav-kv commented Jan 31, 2024

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:

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.
  • Introduce helpers, to eliminate more boilerplate.
  • 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.
@pav-kv pav-kv added the enhancement New feature or request label Jan 31, 2024
@MrDXY
Copy link
Contributor

MrDXY commented Feb 28, 2024

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
Copy link
Contributor Author

pav-kv commented Feb 28, 2024

@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
Copy link
Contributor

MrDXY commented Feb 28, 2024

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

@MrDXY
Copy link
Contributor

MrDXY commented Mar 1, 2024

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?

MrDXY added a commit to MrDXY/raft that referenced this issue Mar 1, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 1, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 1, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 2, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 2, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 2, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 2, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 4, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 4, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 4, 2024
MrDXY pushed a commit to MrDXY/raft that referenced this issue Mar 4, 2024
MrDXY added a commit to MrDXY/raft that referenced this issue Mar 4, 2024
MrDXY added a commit to MrDXY/raft that referenced this issue Mar 4, 2024
@pav-kv
Copy link
Contributor Author

pav-kv commented Mar 4, 2024

@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
Copy link
Contributor

MrDXY commented Apr 15, 2024

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
Copy link
Contributor Author

pav-kv commented Apr 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants