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

tests: Migrate compact tests to common framework #13770

Merged
merged 1 commit into from
Mar 11, 2022
Merged

tests: Migrate compact tests to common framework #13770

merged 1 commit into from
Mar 11, 2022
+139 −2

Conversation

kkkkun
Copy link
Contributor

@kkkkun kkkkun commented Mar 9, 2022

@kkkkun kkkkun mentioned this pull request Mar 9, 2022
Open
@kkkkun kkkkun force-pushed the add-compact-test branch from 4999892 to b009628 Compare March 9, 2022 11:46
@serathius
Copy link
Member

Overall looks good, can you remove the test that you are migrating?

@kkkkun kkkkun force-pushed the add-compact-test branch from b009628 to 91bfb12 Compare March 9, 2022 11:59
@kkkkun
Copy link
Contributor Author

kkkkun commented Mar 9, 2022

Commets Fixed!

@kkkkun
Copy link
Contributor Author

kkkkun commented Mar 9, 2022

Overall looks good, can you remove the test that you are migrating?

ctlV3Compact will be used in ctl_v3_alarm_test and ctl_v3_defrag_test.
So when it all migrated, then i will delete ctl_v3_compact_test.

@kkkkun kkkkun force-pushed the add-compact-test branch from 91bfb12 to ef139bf Compare March 9, 2022 13:42
@ahrtr
Copy link
Member

ahrtr commented Mar 9, 2022

Can you resolve the failures in the pipeline?

@kkkkun kkkkun force-pushed the add-compact-test branch 3 times, most recently from f773526 to 4894c80 Compare March 10, 2022 08:55
@kkkkun kkkkun force-pushed the add-compact-test branch from 4894c80 to 34cd8ae Compare March 10, 2022 09:02
@kkkkun
Copy link
Contributor Author

kkkkun commented Mar 10, 2022

Can you resolve the failures in the pipeline?

Fixed. semaphoreci failed which is not about testcases.

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022 •
edited
Loading

Codecov Report

Merging #13770 (1184d3d) into main (8ac44ff) will decrease coverage by 0.21%.
The diff coverage is 70.00%.

❗ Current head 1184d3d differs from pull request most recent head 34cd8ae. Consider uploading reports for the commit 34cd8ae to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #13770      +/-   ##
==========================================
- Coverage   72.83%   72.61%   -0.22%     
==========================================
  Files         467      467              
  Lines       38280    38368      +88     
==========================================
- Hits        27880    27861      -19     
- Misses       8606     8698      +92     
- Partials     1794     1809      +15
Flag Coverage Δ
all 72.61% <70.00%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
etcdutl/etcdutl/migrate_command.go 75.00% <0.00%> (ø)
etcdutl/snapshot/v3_snapshot.go 65.54% <ø> (-0.15%) ⬇️
pkg/expect/expect.go 82.35% <ø> (ø)
server/storage/schema/membership.go 59.85% <0.00%> (ø)
server/embed/config_logging.go 72.50% <100.00%> (ø)
server/etcdserver/version/monitor.go 96.46% <100.00%> (ø)
server/storage/schema/migration.go 81.48% <100.00%> (ø)
server/storage/schema/schema.go 92.72% <100.00%> (ø)
server/proxy/grpcproxy/register.go 69.76% <0.00%> (-9.31%) ⬇️
server/auth/simple_token.go 80.00% <0.00%> (-8.47%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ac44ff...34cd8ae. Read the comment docs.

ahrtr
ahrtr reviewed Mar 11, 2022
View reviewed changes
tests/framework/testutils/helpters.go
@@ -14,11 +14,24 @@

package testutils

import clientv3 "go.etcd.io/etcd/client/v3"
import (
clientv3 "go.etcd.io/etcd/client/v3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment: It isn't necessary to define a PackageName ("clientv3") here. You can still use clientv3 in the following code without the PackageName here.

Copy link
Member

@ahrtr ahrtr Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not asking for change on this, because it isn't a big deal. And it looks even clear to clearly define a PackageName clientv3 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I commit other tests, i will fixed this.

ahrtr
ahrtr approved these changes Mar 11, 2022
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you!

tests/framework/testutils/helpters.go
@@ -14,11 +14,24 @@

package testutils

import clientv3 "go.etcd.io/etcd/client/v3"
import (
clientv3 "go.etcd.io/etcd/client/v3"
Copy link
Member

@ahrtr ahrtr Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not asking for change on this, because it isn't a big deal. And it looks even clear to clearly define a PackageName clientv3 here.

spzala
spzala approved these changes Mar 11, 2022
View reviewed changes
Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
Thanks @kkkkun

@spzala
Copy link
Member

spzala commented Mar 11, 2022

semaphoreci failure is not related.

@spzala spzala merged commit 4e97271 into etcd-io:main Mar 11, 2022
@kkkkun kkkkun deleted the add-compact-test branch March 11, 2022 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

@spzala spzala spzala approved these changes

@serathius serathius serathius approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kkkkun @serathius @ahrtr @codecov-commenter @spzala