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

Test: Replace t.error/fatal with assert/request in [node_test.go] #179

Merged

Conversation

MrDXY
Copy link
Contributor

@MrDXY MrDXY commented Mar 8, 2024

Refactor: Replacing t.Error/t.Fatal with assert/require

This commit tries to eliminates boilerplate code in the testing suite by replacing occurrences
of t.Error and t.Fatal with assertions from the assert and require packages.

Changes Made:

  • Replaced instances of t.Error with assert
  • Replaced instances of t.Fatal with require

Affected Files:

  • [node_test.go]

Part of: #146

@MrDXY MrDXY mentioned this pull request Mar 8, 2024
6 tasks
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nits

node_test.go Outdated Show resolved Hide resolved
node_test.go Outdated Show resolved Hide resolved
node_test.go Outdated Show resolved Hide resolved
node_test.go Outdated Show resolved Hide resolved
@MrDXY MrDXY force-pushed the replace-test-errors-with-assert-(node_test.go) branch from 9a5304a to 046bcab Compare March 9, 2024 03:53
DescribeEntries(rd.CommittedEntries, func(data []byte) string { return fmt.Sprintf("%q", data) }),
)
}
assert.False(t, !IsEmptyHardState(rd.HardState) && rd.HardState.Commit < persistedHardState.Commit,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
assert.False(t, !IsEmptyHardState(rd.HardState) && rd.HardState.Commit < persistedHardState.Commit,
assert.True(t, IsEmptyHardState(rd.HardState) && rd.HardState.Commit < persistedHardState.Commit,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsEmptyHardState(rd.HardState) || rd.HardState.Commit >= persistedHardState.Commit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I did not see the &&.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall not a big deal, let's keep it as it's for now. Please feel free to update it in a followup PR if you want.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Thanks

@ahrtr ahrtr merged commit 60482e0 into etcd-io:main Mar 10, 2024
10 checks passed
@MrDXY MrDXY deleted the replace-test-errors-with-assert-(node_test.go) branch March 11, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants