Cleanup of the server test #20984
Cleanup of the server test #20984
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: serathius The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
serathius
force-pushed
the
refactor-server-test
branch
from
a48244d to
068f042
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 17 files with indirect coverage changes @@ Coverage Diff @@
## main #20984 +/- ##
=======================================
Coverage 69.14% 69.15%
=======================================
Files 422 422
Lines 34841 34841
=======================================
+ Hits 24092 24095 +3
- Misses 9342 9345 +3
+ Partials 1407 1401 -6 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
server/etcdserver/server_test.go
Outdated
| uberApply: uberApplierMock{}, | ||
| } | ||
| s := newServer(t, nil, testEtcdServerConfig{ | ||
| lg: zaptest.NewLogger(t), |
There was a problem hiding this comment.
lg can be ignored and just use the default logger in newServer.
server/etcdserver/server_test.go
Outdated
|
|
||
| func newServer(t *testing.T, recorder *nodeRecorder, cfgs ...testEtcdServerConfig) *EtcdServer { | ||
| var cfg testEtcdServerConfig | ||
| if len(cfgs) > 0 { |
There was a problem hiding this comment.
can you add an assertion that len(cfgs)<2 so that the intent is clear the plural is here just to handle 0 and 1 config?
server/etcdserver/server_test.go
Outdated
| srv := &EtcdServer{ | ||
| lgMu: new(sync.RWMutex), | ||
| srv := newServer(t, n, testEtcdServerConfig{ | ||
| lg: zaptest.NewLogger(t), |
There was a problem hiding this comment.
lg can be ignored and just use the default logger in newServer.
server/etcdserver/server_test.go
Outdated
| if lg == nil { | ||
| lg = zaptest.NewLogger(t) | ||
| } | ||
| be := cfg.be |
There was a problem hiding this comment.
what happens if cl.Backend is inconsistent with cfg.be?
There was a problem hiding this comment.
good point.
I think we should remove cluster *membership.RaftCluster from testEtcdServerConfig, and dynamically create a cluster *membership.RaftCluster based on the cfg in newServer
server/etcdserver/server_test.go
Outdated
| consistIndex: cindex.NewConsistentIndex(be), | ||
| } | ||
| srv := newServer(t, nil, testEtcdServerConfig{ | ||
| lg: zaptest.NewLogger(t), |
There was a problem hiding this comment.
lg can be ignored and just use the default logger in newServer.
server/etcdserver/server_test.go
Outdated
| cancel: cancel, | ||
| } | ||
| srv := newServer(t, nil, testEtcdServerConfig{ | ||
| lg: zaptest.NewLogger(t), |
There was a problem hiding this comment.
lg can be ignored and just use the default logger in newServer.
serathius
force-pushed
the
refactor-server-test
branch
from
8e683af to
46678e4
Compare
…nstructing EtcdServer by themselves Signed-off-by: Marek Siarkowicz <[email protected]>
serathius
force-pushed
the
refactor-server-test
branch
from
46678e4 to
c94a88d
Compare
|
@serathius: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Cleanup of the server test. First iteration on unifying the server_test.go to have a single constructor in tests. Constructing EtcdServer is very complicated and having it done per each test has high risk of test validating fake behavior instead of real one.
cc @fuweid @ahrtr @siyuanfoundation