migrate e2e/users tests to common framework #13819
Conversation
endocrimes
force-pushed
the
dani/auth_test.go
branch
from
46c9ea9 to
1a57bab
Compare
seems unrelated - will file an issue for the flake |
|
|
||
| // If no password is provided, and NoPassword isn't set, the CLI will always | ||
| // wait for a password, send an enter in this case for an "empty" password. | ||
| if !opts.NoPassword && password == "" { |
There was a problem hiding this comment.
Could we simplify code by always providing password in interactive mode?
There was a problem hiding this comment.
We could! - But this way we maintain a semblance of the previous coverage that included password on stdin and password in the command.
I'm not particularly tied to either tho - this actually caught me out pretty last minute bc I was surprised by the behaviour of "non-interactive, but still waiting on stdin"
There was a problem hiding this comment.
I don't think we need UserAddOptions added in tests/framework/config/client.go. We need to follow the same logic as etcdctl, refer to user_command.go#L135-L163.
There was a problem hiding this comment.
This is the same logic - the CLI will only pass NoPassword: true when --no-password is provided, because it's explicitly distinct behavior from an empty password. (--no-password as documented requires CN auth, but an empty password is also apparently valid on normal accounts?)
There was a problem hiding this comment.
OK, sounds good. The logic should be OK, and it's a valid case as well. When password is empty and NoPassword is false, it makes sense to send an enter. It aligns with the existing user experience.
|
|
||
| // If no password is provided, and NoPassword isn't set, the CLI will always | ||
| // wait for a password, send an enter in this case for an "empty" password. | ||
| if !opts.NoPassword && password == "" { |
There was a problem hiding this comment.
I don't think we need UserAddOptions added in tests/framework/config/client.go. We need to follow the same logic as etcdctl, refer to user_command.go#L135-L163.
| @@ -1265,3 +1265,24 @@ func ctlV3EndpointHealth(cx ctlCtx) error { | |||
| } | |||
| return e2e.SpawnWithExpects(cmdArgs, cx.envMap, lines...) | |||
| } | |||
|
|
|||
| func ctlV3User(cx ctlCtx, args []string, expStr string, stdIn []string) error { | |||
There was a problem hiding this comment.
Please add a comment here something like: This function may need to be deleted when the tests in ctl_v3_auth_test.go are migrated.
There was a problem hiding this comment.
I think this can be said about every function, delete it when it's not used :P
There was a problem hiding this comment.
Yeah I'm not sure a comment makes sense - anyone migrating the tests will presumably also delete any code they can. (And that person is probably going to be next week me)
There was a problem hiding this comment.
It is OK because the function ctlV3User is in the same file as the test cases.
endocrimes
force-pushed
the
dani/auth_test.go
branch
from
1a57bab to
5b148bc
Compare
|
|
||
| // If no password is provided, and NoPassword isn't set, the CLI will always | ||
| // wait for a password, send an enter in this case for an "empty" password. | ||
| if !opts.NoPassword && password == "" { |
There was a problem hiding this comment.
OK, sounds good. The logic should be OK, and it's a valid case as well. When password is empty and NoPassword is false, it makes sense to send an enter. It aligns with the existing user experience.
tests/common/user_test.go
Outdated
| username: "foo", | ||
| password: "", | ||
| noPassword: false, | ||
| expectError: false, |
There was a problem hiding this comment.
A non-empty password is always required when NoPassword is false, so the integration test should fail in this case. Please double check.
|
@endocrimes Could you please record to #13637, Let us know which cases have been taken? |
endocrimes
force-pushed
the
dani/auth_test.go
branch
from
5b148bc to
3416042
Compare
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.