Add sorting test cases for range operations#21246
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 21 files with indirect coverage changes @@ Coverage Diff @@
## main #21246 +/- ##
==========================================
+ Coverage 68.43% 68.45% +0.01%
==========================================
Files 429 429
Lines 35291 35291
==========================================
+ Hits 24153 24160 +7
+ Misses 9748 9742 -6
+ Partials 1390 1389 -1 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
nwnt
force-pushed
the
add-range-test-sorting
branch
from
6c679d9 to
fc05e86
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nwnt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tests/common/kv_test.go
Outdated
| {name: "all KVs sorted by key descending", begin: "", options: config.GetOptions{Prefix: true, SortBy: clientv3.SortByKey, Order: clientv3.SortDescend}, wantResponse: &clientv3.GetResponse{Header: createHeader(firstHeader, 7), Count: 6, Kvs: reversedKvs}}, | ||
| {name: "all KVs sorted by create rev", begin: "", options: config.GetOptions{Prefix: true, SortBy: clientv3.SortByCreateRevision}, wantResponse: &clientv3.GetResponse{Header: createHeader(firstHeader, 7), Count: 6, Kvs: allKvs}}, | ||
| {name: "all KVs sorted by create rev desc", begin: "", options: config.GetOptions{Prefix: true, SortBy: clientv3.SortByCreateRevision, Order: clientv3.SortDescend}, wantResponse: &clientv3.GetResponse{Header: createHeader(firstHeader, 7), Count: 6, Kvs: reversedKvs}}, | ||
| {name: "all KVs sorted by mod rev", begin: "", options: config.GetOptions{Prefix: true, SortBy: clientv3.SortByModRevision}, wantResponse: &clientv3.GetResponse{Header: createHeader(firstHeader, 7), Count: 6, Kvs: allKvs}}, |
There was a problem hiding this comment.
What's the benefit of testing sorting by modRev if the result is the same as sorting by create rev and key?
There was a problem hiding this comment.
There are scenarios that they won't be I believe. However, to differentiate that I'll have to modify the data, which would make this PR ballooned and harder to understand.
What I can do is either:
- Leave as is and I'll have a follow up PR to adjust the data to differentiate the scenario for mod rev.
- Remove them for now and add them back later with that PR.
What do you think?
There was a problem hiding this comment.
I would not merge noop tests for the risk for forgetting and people assuming things are properly tested. If we want to split PR we should first improve the input data.
There was a problem hiding this comment.
Yep, removed the mod rev cases for now. Will have 2 follow-up PRs: one for the data rearrangement, the second is to add the cases.
Signed-off-by: Nont <nont@duck.com>
nwnt
force-pushed
the
add-range-test-sorting
branch
from
fc05e86 to
d7a0909
Compare
|
/retest |
3 participants
Breaking #21229 down. This PR adds the additional test cases, and groups the existing sorting and ordering ones so they stay close to each other.
cc @serathius