Create common framework for e2e and integration tests and migrate TestKVPut test #13708
Conversation
serathius
force-pushed
the
common
branch
5 times, most recently
from
3944080
to
a161514
Compare
|
PTAL @ahrtr |
| } | ||
|
|
||
| func (c integrationClient) Put(key, value string) error { | ||
| _, err := c.Client.Put(context.Background(), key, value) |
There was a problem hiding this comment.
better not to ignore the first return value in framework, instead, let users to decide whether ignore it or not.
There was a problem hiding this comment.
The first argument is the PutResponse, I decided no not return it as most tests don't check the response assuming that request succeed. Implementing parsing PutResponse would make E2e client more complicated. Please remember that current interface is not final and is expect to evolve over time to handle new test cases.
| *e2e.EtcdctlV3 | ||
| } | ||
|
|
||
| func (c e2eClient) Get(key string, opts ...testutils.GetOption) (*clientv3.GetResponse, error) { |
There was a problem hiding this comment.
I do not see Put method in this file.
tests/framework/framework.go
Outdated
| var ( | ||
| TestFramework testFramework = noFrameworkSelected{} | ||
| RunE2eTests testFramework = e2eFramework{} | ||
| RunIntegrationTests testFramework = integrationFramework{} |
There was a problem hiding this comment.
The names E2eFramework and IntegrationFramework make more sense than RunE2eTests and RunIntegrationTests
There was a problem hiding this comment.
Still working out the naming, I decided on this name because it's more readable from user perspective. As a user name testFramework = framework.E2eFramework doesn't really explain what the change does. On the other handl testFramework = framework.RunE2eTests explains that it will configure tests to run E2e scenario.
There was a problem hiding this comment.
I agree with Artur: Having boring names (that we name both enum and type the same) helps with code readability.
However we name it: runner / framework / harness, we should use this name consequently.
There was a problem hiding this comment.
I think going with TestRunner would be nicer. So E2eTestRunner and IntegrationTestRunner
|
The changes look much better than the existing design & implementation. But It would be even better if you can have a overall design (a big picture) and the directory layout. Personally, I think the framework should be just a generic/common framework, it shouldn't have any code specific to e2e or integration tests. Both E2e and integration are just two extensions or implementations of the framework. The detailed design isn't super clear yet in my mind. I have a picture in the relate ticket although it may not accurate yet. It's OK to merge this change for now because at least it's a big improvement. We can continue to refactor it later. |
tests/framework/framework.go
Outdated
| var ( | ||
| TestFramework testFramework = noFrameworkSelected{} | ||
| RunE2eTests testFramework = e2eFramework{} | ||
| RunIntegrationTests testFramework = integrationFramework{} |
There was a problem hiding this comment.
I agree with Artur: Having boring names (that we name both enum and type the same) helps with code readability.
However we name it: runner / framework / harness, we should use this name consequently.
Codecov Report
@@ Coverage Diff @@
## main #13708 +/- ##
==========================================
- Coverage 72.76% 72.64% -0.12%
==========================================
Files 467 467
Lines 38278 38287 +9
==========================================
- Hits 27854 27815 -39
- Misses 8623 8661 +38
- Partials 1801 1811 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Context #13637
After trying different approaches, I decided to go with picking single test and migrating them one by one.
cc @ahrtr @spzala @ptabor