Add coverage for kv range operations

[nwnt]

Adding a small unit test as a small start step for #20386

cc @serathius @joshjms

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nwnt
Once this PR has been reviewed and has the lgtm label, please assign serathius for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.53%. Comparing base (aef0361) to head (38b7005).

Additional details and impacted files

see 15 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #21025      +/-   ##
==========================================
+ Coverage   68.43%   68.53%   +0.10%     
==========================================
  Files         429      429              
  Lines       35281    35281              
==========================================
+ Hits        24144    24180      +36     
+ Misses       9734     9698      -36     
  Partials     1403     1403              

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aef0361...38b7005. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nwnt]

The lint error is now fixed.

[serathius]

Tests should cover whole range execution, not just rangeLimit function that will be might not be used in the future.

For regression testing you want to run as high level test as possible to prove that changing implementation below doesn't change the output.

[nwnt]

Yep, I can add the whole tests for range. Thought I was going to start adding them one by one to keep the PRs small, but I can do that too.

[serathius]

We don't do testing to hardcode the reverse behavior of functions, we use them to prevent regressions. While doing small steps and small PRs is very good, we need to remember the goal test that ensures that the behavior of limit in range will not change. This test doesn't achieve this.

[nwnt]

Totally understood. Still working on the unit tests for the Range function and will be sending that shortly.

[nwnt]

@serathius this should look better this time. I believe server's Range ultimately calls *bucketBuffer.Range which currently lacks any test coverage. This method of bucketBuffer is also where the values are returned together with the matched keys. If this is ok, I will have follow PRs for related functions until we have enough coverage before looking at adding the option for excluding the values.

Feel free to leave any comments.

[serathius]

Bucket buffer is just a buffer for a operations that have not been committed. While testing range should cover this buffer again testing it by itself is not enough

[nwnt]

@serathius can you have another look at this? I have removed the previous unit tests and added what I think would provide the coverage in common/kv_test.go.

I tested these test cases with the following command:

PASSES=integration PKG=./tests/common TESTCASE='\bTestKVGet\b' ./scripts/test.sh

[k8s-ci-robot]

@nwnt: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-coverage-report	38b7005	link	true	/test pull-etcd-coverage-report
pull-etcd-e2e-arm64	38b7005	link	true	/test pull-etcd-e2e-arm64
pull-etcd-e2e-386	38b7005	link	true	/test pull-etcd-e2e-386
pull-etcd-e2e-amd64	38b7005	link	true	/test pull-etcd-e2e-amd64

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.

Details

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.

[serathius]

Please pair this test case with one that shows that second update. Test cases by themselves should show how changing argument impacts results.

[serathius]

The test cases below remind me of https://github.com/etcd-io/etcd/blob/main/tests/integration/cache_test.go#L579 Might be good to take a look if there are any interesting cases there that are missing here, and maybe unify them at some point in the future.

[serathius]

On think I don't like in the test here is that we use a indirect way to express expectation and compare the results. It compares only key/value and skips all the other metadata like ModRevision, CreateRevision etc. Those are still important fields used by clients to build a TXN conditions. I'm worried that a bug can easily slip if we forget to validate them.

Please see how we did it in

etcd/tests/integration/cache_test.go

Line 732 in 05deb60

var getTestCases = []getTestCase{

where we had a reference to each write and compare one to one the []*mvccpb.KeyValue.