tests: Migrate endpoint tests to common framework #13774
Conversation
kkkkun
force-pushed
the
endpoint-test
branch
2 times, most recently
from
ddac11c
to
2d7840e
Compare
Codecov Report
@@ Coverage Diff @@
## main #13774 +/- ##
==========================================
- Coverage 72.75% 72.74% -0.02%
==========================================
Files 467 467
Lines 38279 38643 +364
==========================================
+ Hits 27850 28110 +260
- Misses 8626 8721 +95
- Partials 1803 1812 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
kkkkun
force-pushed
the
endpoint-test
branch
3 times, most recently
from
31599aa
to
788aac0
Compare
kkkkun
changed the title
|
A generic comment, usually when we migrate the test cases, we move the test cases from one place to another place (Of course, there maybe some refactor if needed), so there should be some cases being added, and some cases being deleted. But I do not see any code deletion in this PR. If the code are reused somewhere else, then please try to migrate them together in ONE PR. On the other hand, try to keep each PR as small as possible, so as to make the code review easier. So please use your best judgement. If somehow you can't follow either rule, then you need to clearly state the reason and plan/steps. For example, if you have to only add test cases in this PR and no any deletion, then you need to provide a reasonable reason, and the plan/steps to delete the related cases/code. |
kkkkun
force-pushed
the
endpoint-test
branch
2 times, most recently
from
0ad40f1
to
45bf24c
Compare
|
|
||
| func (ctl *EtcdctlV3) Status() ([]*clientv3.StatusResponse, error) { | ||
| args := ctl.cmdArgs() | ||
| args = append(args, "endpoint", "status", "-w", "json") |
There was a problem hiding this comment.
It would be better to let users to configure the output format, and we can provide a default value. Ideally all the globalFlags should be configurable, we can even add the field into EtcdctlV3 (see below). Of course, it shouldn't be the scope/responsibility of this PR.
type EtcdctlV3 struct {
clusterCfg *EtcdProcessClusterConfig
globalCfg command.GlobalFlags{}
}
There was a problem hiding this comment.
We need to unmarshal it in line 197. And in line 182, it needs to print by json.
E2e test cases use one of format output. It should be different case by case?
There was a problem hiding this comment.
@kkkkun is right, the -w json is a binary specific flag that is needed to parse the output to return same proto response as integration tests. This flag and other e2e specific features will be useful when we also start working on e2e specific tests (tests that cannot be replicated for integration tests, like downgrade tests as they expect to run last release binary), however for now let's focus on migrating the common subset of tests.
| func (c integrationClient) Health() error { | ||
| cli := healthpb.NewHealthClient(c.Client.ActiveConnection()) | ||
| resp, err := cli.Check(context.TODO(), &healthpb.HealthCheckRequest{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if resp.Status != healthpb.HealthCheckResponse_SERVING { | ||
| return fmt.Errorf("status expected %s, got %s", healthpb.HealthCheckResponse_SERVING, resp.Status) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The integration test health implementation is different than e2e one which using etcdctl endpoints health that probes linearizable read and check if there is any active alarms. It's not a big issue but just want to call it out.
Part of #13637
cc @ptabor @spzala @ahrtr @serathius