etcd icon indicating copy to clipboard operation
etcd copied to clipboard

Unify testing framework

Open serathius opened this issue 3 years ago • 16 comments

Test flakiness and maintenance cost remains one of the larger issues for Etcd project. Proposals like https://github.com/etcd-io/etcd/issues/13167 to track flakiness and contributions to address singular tests have definitely helped however to fully resolve the problem we also need to address the root cause. Etcd projects currently maintains multiple disconnected ways to run tests (unit, integration, e2e, functional) that mostly verify the same scenarios. Testing features on different level is itself desired as it allows to isolate failures and speed up resolution, however not in a way that is currently done in Etcd. Almost zero test scenario and test framework is reused. This means that we have multiplied test code lines without any benefit.

Goal:

  • Reduce number of test code without affecting test coverage
  • Simplify process of adding new integration/e2e test
  • Make it easier to isolate test failures by comparing test results from different methods

I propose to unify testing by identifying common test scenarios, test framework operations and making them accessable the same way no matter which test method is used (unit, integration, e2e, functional). For example simple put&get test scenario goes through very same stages no matter if this is integration or e2e cluster: create cluster, execute put, execute get, compare results, cleanup. This test should be written once and executed on different layers, no matter if underneath framework runs a fake grpc client or starts whole process. With this we will reduce number of code lines as each test will be needed to be implemented once, hide all the differences between test types reducing knowledge needed to add new tests and improve deflaking process by comparing same test results from different methods (we can isolate test location, if both e2e and integration tests fail then feature is broken, if only one of them then the test framework)

At this moment I would be most effective to target unifying integration and e2e tests as they are most similar in both cluster setup and test scenarios. We should be able to identify minimal common interface for cluster creation and communication that would allow us to start rewriting tests.

Plan:

  • Create new Cluster and Client interfaces in tests/framework.
  • Find common function in tests/framework/e2e/cluster.go and tests/framework/integration/cluster.go and migrate them one by one.
  • Deduplicate test scenarios between e2e and integration tests
  • Create common tests located in tests/common that will be invoked by e2e and integration.

cc @ptabor @ahrtr @spzala

serathius avatar Jan 24 '22 13:01 serathius

It's really a good proposal. I am still very new to the existing test framework & cases, but I think the high level architecture would be something like below,

test_framework

ahrtr avatar Jan 27 '22 00:01 ahrtr

We have managed to merged enough PRs to establish fundaments for the new framework (https://github.com/etcd-io/etcd/pull/13708, https://github.com/etcd-io/etcd/pull/13740, https://github.com/etcd-io/etcd/pull/13753, https://github.com/etcd-io/etcd/pull/13754). I think we can open the effort to more contributors to scale the migration. I think it would be a good idea to encourage new tests to be implemented in new framework.

I think we can do migration file by file each assigned to single person. I as already started I will continue working on e2e/ctl_v3_kv_test.go and e2e/ctl_v3_kv_no_quorum_test.go. I would start from migrating E2e tests as there are less of them and they are easier to migrate. However we should try to remove integration test cases if new common tests covers it.

Tests to be migrated:

  • [x] e2e/ctl_v3_defrag_test.go @kkkkun https://github.com/etcd-io/etcd/pull/13820
  • [ ] e2e/ctl_v3_auth_test.go @chao
  • [x] e2e/ctl_v3_endpoint_test.go @kkkkun https://github.com/etcd-io/etcd/pull/13774
  • [ ] e2e/ctl_v3_auth_no_proxy_test.go @chao
  • [ ] e2e/ctl_v3_test.go
  • [ ] e2e/ctl_v3_snapshot_test.go @vimalk78
  • [ ] e2e/discovery_v3_test.go
  • [ ] e2e/v2store_deprecation_test.go
  • [ ] e2e/ctl_v3_elect_test.go
  • [x] e2e/ctl_v3_kv_no_quorum_test.go - @serathius https://github.com/etcd-io/etcd/pull/13754
  • [ ] e2e/v3_curl_lease_test.go
  • [x] e2e/ctl_v3_alarm_test.go @nic-chen
  • [x] e2e/ctl_v3_user_test.go @endocrimes https://github.com/etcd-io/etcd/pull/13819
  • [ ] e2e/ctl_v3_watch_test.go @nic-chen
  • [ ] e2e/etcd_config_test.go
  • [ ] e2e/ctl_v3_txn_test.go @serathius
  • [x] e2e/etcd_corrupt_test.go
  • [x] e2e/ctl_v3_compact_test.go @kkkkun https://github.com/etcd-io/etcd/pull/13770
  • [x] e2e/ctl_v3_member_test.go @clarkfw
  • [x] e2e/ctl_v3_role_test.go @chao
  • [ ] e2e/metrics_test.go @kkkkun
  • [ ] e2e/ctl_v3_make_mirror_test.go
  • [ ] e2e/ctl_v3_completion_test.go
  • [ ] e2e/discovery_test.go (@tayaleelin )
  • [ ] e2e/v3_cipher_suite_test.go
  • [x] e2e/ctl_v3_kv_test.go - @serathius https://github.com/etcd-io/etcd/pull/13740 https://github.com/etcd-io/etcd/pull/13708 https://github.com/etcd-io/etcd/pull/13753
  • [ ] e2e/v3_curl_test.go
  • [ ] e2e/ctl_v3_move_leader_test.go @clarkfw
  • [ ] e2e/ctl_v3_watch_cov_test.go @nic-chen
  • [ ] e2e/ctl_v3_lease_test.go @cdalar
  • [ ] e2e/ctl_v3_grpc_test.go @padlar
  • [ ] e2e/gateway_test.go
  • [ ] e2e/ctl_v3_watch_no_cov_test.go @nic-chen
  • [ ] e2e/ctl_v3_lock_test.go

Note for contributors: Feel free to pick one of the directories and just leave a comment that you want to work on it.

serathius avatar Mar 02 '22 11:03 serathius

I will work on follow tests:

  • [x] e2e/ctl_v3_compact_test.go https://github.com/etcd-io/etcd/pull/13770
  • [x] e2e/ctl_v3_endpoint_test.go https://github.com/etcd-io/etcd/pull/13774
  • [x] e2e/ctl_v3_defrag_test.go https://github.com/etcd-io/etcd/pull/13820
  • [x] e2e/ctl_v3_completion_test https://github.com/etcd-io/etcd/pull/13825
  • [ ] e2e/metrics_test.go

kkkkun avatar Mar 09 '22 09:03 kkkkun

hi @serathius I would like to take some

nic-chen avatar Mar 11 '22 22:03 nic-chen

I would start with the simple one: e2e/ctl_v3_alarm_test.go

nic-chen avatar Mar 15 '22 01:03 nic-chen

will continue with: e2e/ctl_v3_txn_test.go e2e/ctl_v3_watch_test.go e2e/ctl_v3_watch_cov_test.go e2e/ctl_v3_watch_no_cov_test.go

nic-chen avatar Mar 20 '22 02:03 nic-chen

hi @serathius, I tried to migrate ctl_v3_txn_test, but I found it difficult to be compatible with both e2e and integration.

Because the cli commands like version("key") < "0" need to be converted to and from Txn and Cmp, which is too complicated for testing, and we should use the original call to do it Tested rather than converted.

So I think maybe it shouldn't be migrated.

What is your suggestion? thanks!

nic-chen avatar Mar 21 '22 15:03 nic-chen

Thanks for looking into this. Please skip the ctl_v3_txn_test for now. I will take a look and see what we can do.

serathius avatar Mar 22 '22 09:03 serathius

Thanks for looking into this. Please skip the ctl_v3_txn_test for now. I will take a look and see what we can do.

got it, thanks.

nic-chen avatar Mar 22 '22 16:03 nic-chen

i am working on

  • [x] e2e/ctl_v3_txn_test.go https://github.com/etcd-io/etcd/pull/14000

vimalk78 avatar Apr 30 '22 08:04 vimalk78

I can take a few more.

  • [ ] e2e/etcd_release_upgrade_test.go
  • [ ] e2e/cluster_downgrade_test.go
  • [ ] e2e/utl_migrate_test.go

Update...

I just realized above 3 test files should not be classified as common tests shared by e2e and integration tests. For example, KeepDataDir, ExecPath and DataDir config is unique to e2e test. Also the last release version's etcd binary path is hard-coded and integration test with embed etcd won't have that concept..

I am inclined to skip the above tests and leave them as they are. WDYT? @serathius

Will continue the following tests tomorrow..

  • [ ] e2e/ctl_v3_auth_test.go
  • [ ] e2e/ctl_v3_auth_no_proxy_test.go

chaochn47 avatar May 05 '22 21:05 chaochn47

Makes sense, those tests cannot don't make sense for integration tests and will require different approach. Let's leave them for now. Thanks for pointing this out.

serathius avatar May 06 '22 09:05 serathius

anyone working on

  • [ ] e2e/ctl_v3_snapshot_test.go if not i will take up this. @serathius why is this issue closed? are all tests migrated?

vimalk78 avatar May 11 '22 05:05 vimalk78

It got closed because of the PRs that was merged included text fixes #issue-id in top comment, which is very (nice/bad) Github issue that allows anyone to close issue. I didn't notice it and merged the PR so I was marked as actor closing the issue. Sorry.

serathius avatar May 11 '22 07:05 serathius

oops. I will avoid the fixes #issue-id statement in the following tests migration PR then. I did not realize that functionality ==

chaochn47 avatar May 13 '22 00:05 chaochn47

I'm working on:

  • [ ] e2e/ctl_v3_member_test.go #14278 #14281 #14304 #14437
  • [ ] e2e/ctl_v3_move_leader_test.go

clement2026 avatar Jul 25 '22 11:07 clement2026

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 18 '23 09:03 stale[bot]

Going to work on e2e/ctl_v3_grpc_test.go

padlar avatar Apr 20 '23 09:04 padlar

I'm a new contributor. I will try migrating the test: e2e/discovery_test.go

tayaleelin avatar Apr 20 '23 09:04 tayaleelin

New Contributor. Will try to migrate e2e/ctl_v3_lease_test.go

cdalar avatar Apr 20 '23 09:04 cdalar