Add verification to ensure the watch events are in the expected range #18980
Conversation
Signed-off-by: Benjamin Wang <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 18 files with indirect coverage changes @@ Coverage Diff @@
## main #18980 +/- ##
==========================================
- Coverage 68.80% 68.76% -0.04%
==========================================
Files 420 420
Lines 35599 35606 +7
==========================================
- Hits 24494 24485 -9
- Misses 9675 9692 +17
+ Partials 1430 1429 -1 Continue to review full report in Codecov by Sentry.
|
| @@ -372,6 +374,17 @@ func (s *watchableStore) syncWatchers() int { | |||
| // Otherwise we will trigger SIGSEGV during boltdb re-mmap. | |||
| tx.RUnlock() | |||
|
|
|||
| verify.Verify(func() { | |||
There was a problem hiding this comment.
This just tests the results of range over revisions, this kind of things should be validated in tests, not during e2e tests.
I think the discussion in #17563 (comment) was about curRev decreasing
There was a problem hiding this comment.
We already have a couple of verifications to ensure the revision will never decrease.
This verification is only to ensure the watch events are in the expected range.
There was a problem hiding this comment.
We introduce verification for non-trivial properties that are based on state that is changed in so many places that it's impossible to test them in isolated way. In this case you are just validating range that is defined via minBytes and maxBytes variable 10 lines above. Would a unit test be a better match?
There was a problem hiding this comment.
It mainly verifies the keys(revisions) read from bbolt are in the expected range. We might should do it in bbolt instead of etcd.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.