Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify testing framework #13637

Open
serathius opened this issue Jan 24, 2022 · 20 comments · Fixed by #14020
Open

Unify testing framework #13637

serathius opened this issue Jan 24, 2022 · 20 comments · Fixed by #14020
Labels
help wanted stage/tracked

Comments

@serathius
Copy link
Member

Test flakiness and maintenance cost remains one of the larger issues for Etcd project. Proposals like #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 serathius mentioned this issue Jan 26, 2022
Merged
@ahrtr
Copy link
Member

ahrtr commented Jan 27, 2022

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

@serathius serathius mentioned this issue Feb 17, 2022
Merged
@serathius
Copy link
Member Author

serathius commented Mar 2, 2022 •
edited
Loading

We have managed to merged enough PRs to establish fundaments for the new framework (#13708, #13740, #13753, #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:

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

@kkkkun
Copy link
Contributor

kkkkun commented Mar 9, 2022 •
edited
Loading

I will work on follow tests：

This was referenced Mar 9, 2022
Merged
Merged
@nic-chen
Copy link
Contributor

hi @serathius I would like to take some

@nic-chen
Copy link
Contributor

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

@kkkkun kkkkun mentioned this issue Mar 18, 2022
Merged
@nic-chen nic-chen mentioned this issue Mar 18, 2022
Merged
@kkkkun kkkkun mentioned this issue Mar 19, 2022
Merged
@nic-chen
Copy link
Contributor

nic-chen commented Mar 20, 2022 •
edited
Loading

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

@kkkkun kkkkun mentioned this issue Mar 20, 2022
Closed
@nic-chen
Copy link
Contributor

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!

@serathius
Copy link
Member Author

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.

@nic-chen
Copy link
Contributor

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 nic-chen mentioned this issue Apr 17, 2022
Closed
@vimalk78
Copy link
Contributor

vimalk78 commented Apr 30, 2022 •
edited
Loading

i am working on

@vimalk78 vimalk78 mentioned this issue May 1, 2022
Merged
@chaochn47
Copy link
Member

chaochn47 commented May 5, 2022 •
edited
Loading

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

@serathius
Copy link
Member Author

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.

@chaochn47 chaochn47 mentioned this issue May 8, 2022
Merged
@vimalk78
Copy link
Contributor

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?

@serathius serathius reopened this May 11, 2022
@serathius
Copy link
Member Author

serathius commented May 11, 2022 •
edited
Loading

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.

@chaochn47
Copy link
Member

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

@chaochn47 chaochn47 mentioned this issue May 13, 2022
Closed
@clement2026
Copy link
Contributor

clement2026 commented Jul 25, 2022 •
edited
Loading

I'm working on:

This was referenced Jul 26, 2022
Closed
Merged
Merged
@clement2026 clement2026 mentioned this issue Aug 3, 2022
Closed
@nic-chen nic-chen mentioned this issue Aug 15, 2022
Merged
@ahrtr ahrtr mentioned this issue Aug 18, 2022
Merged
@clement2026 clement2026 mentioned this issue Sep 7, 2022
Merged
@ahrtr ahrtr mentioned this issue Oct 26, 2022
Closed
@serathius serathius mentioned this issue Nov 22, 2022
Closed
@stale
Copy link

stale bot commented Mar 18, 2023

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 stale bot added the stale label Mar 18, 2023
@stale stale bot removed the stale label Mar 18, 2023
@serathius serathius mentioned this issue Mar 30, 2023
Closed
@padlar
Copy link

padlar commented Apr 20, 2023

Going to work on e2e/ctl_v3_grpc_test.go

@tayaleelin
Copy link

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

@cdalar
Copy link

cdalar commented Apr 20, 2023 •
edited
Loading

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

@padlar padlar mentioned this issue Apr 20, 2023
Closed
@serathius serathius mentioned this issue Jan 26, 2024
Open
@serathius serathius mentioned this issue Jun 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted stage/tracked
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

migrate e2e & integration role_test to common chaochn47/etcd
10 participants
@cdalar @padlar @vimalk78 @serathius @kkkkun @clement2026 @ahrtr @nic-chen @tayaleelin @chaochn47