Migrate key value Get to common framework #13740
Conversation
Codecov Report
@@ Coverage Diff @@
## main #13740 +/- ##
==========================================
- Coverage 72.61% 72.57% -0.05%
==========================================
Files 467 467
Lines 38286 38286
==========================================
- Hits 27802 27785 -17
- Misses 8672 8685 +13
- Partials 1812 1816 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
serathius
force-pushed
the
common-get
branch
from
f350f2b
to
e6ba780
Compare
| } | ||
| if o.CountOnly { | ||
| clientOpts = append(clientOpts, clientv3.WithCountOnly()) | ||
| } |
There was a problem hiding this comment.
Is it possible to use a for loop to process all the GetOptions? It would be better to follow the same logic as op.go#L326-L330, so as to make the code more readable and extensible.
There was a problem hiding this comment.
This was explicitly avoided when working on e2e implementation. Due to differences between e2e and integration code, one prefers using config as a struct and second options as a list of functions that can be applied on config. No matter what which approach we peak it will be always awkward in one of the approaches.
In the end I decided to go with configuration struct as it requires least amount of code making it more flexible to re-implementation. Implementing both interfaces would be even worse as it would double the required code without any benefits as having a clean interface is not as important for internal test framework.
serathius
force-pushed
the
common-get
branch
3 times, most recently
from
1d049bf
to
95dc51a
Compare
| @@ -22,28 +22,14 @@ import ( | |||
| "go.etcd.io/etcd/tests/v3/framework/e2e" | |||
| ) | |||
|
|
|||
| func TestCtlV3PutNoTLS(t *testing.T) { testCtl(t, putTest, withCfg(*e2e.NewConfigNoTLS())) } | |||
There was a problem hiding this comment.
@serathius I am curious why are we removing bunch of test functions? Thanks!
There was a problem hiding this comment.
I believe the tests are moved into tests/common/kv_test.go.
There was a problem hiding this comment.
Yes, this are exact tests that are migrated, as we are combining e2e and integration tests I remove test cases from both directories.
|
Hi, serathius you need someone else to share the work? I would like to participate in etcd's contribution from the test. thanks. |
cc @ahrtr @ptabor @spzala