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

migrate 6 auth test cases to common #3 #15320

Merged
merged 1 commit into from
Feb 16, 2023
Merged

migrate 6 auth test cases to common #3 #15320

merged 1 commit into from
Feb 16, 2023

Conversation

chaochn47
Copy link
Member

Related to the ask from #14041 to split tests into smaller pieces.

So code review will be reviewer friendly and easier to reason about.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@serathius
Copy link
Member

serathius commented Feb 15, 2023 •
edited

Related to the ask from #14041 to split tests into smaller pieces.

So code review will be reviewer friendly and easier to reason about.

6 x 50+ line of code tests is also not reviewer friendly :(

@serathius
Copy link
Member

Overall note, the way you validate authorization errors is inconsistent. Sometimes you use putFailPerm, sometimes if err == nil || !strings.Contains(err.Error(), rpctypes.ErrAuthFailed.Error()) or require.ErrorContains(t, err, "authentication failed").

Let's be consistent in this file. I would recommend using require.ErrorContains(t, err, "authentication failed") in all cases.

@chaochn47 chaochn47 force-pushed the migrate_auth_tests_to_common branch 2 times, most recently from 7b579e1 to 625512f Compare February 15, 2023 18:27
@chaochn47
migrate auto tests to common #3
c36ea3f 
Signed-off-by: Chao Chen <chaochn@amazon.com>
@chaochn47
Copy link
Member Author

Appreciate all the comments. They are resolved. PTAL again, thanks!!

ahrtr
ahrtr approved these changes Feb 16, 2023
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.

Overall looks good to me.

If you want quick review and approval, please always try to keep each PR (or commit) as simple/small as possible.

Re-triggered the E2E tests to double check whether there are some flaky cases.

@serathius serathius merged commit c24ba4d into etcd-io:main Feb 16, 2023
@chaochn47 chaochn47 deleted the migrate_auth_tests_to_common branch February 16, 2023 17:15
@serathius serathius mentioned this pull request Apr 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serathius serathius serathius approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chaochn47 @serathius @ahrtr