Skip to content

Tests: Add aclfile load and save tests #7765

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
Merged
oranagra merged 1 commit into from
Sep 9, 2020
Merged

Tests: Add aclfile load and save tests #7765

oranagra merged 1 commit into from
Sep 9, 2020

Conversation

@yangbodong22011
Copy link
Contributor

@yangbodong22011 yangbodong22011 commented Sep 9, 2020

Add aclfile load tests.

oranagra
oranagra reviewed Sep 9, 2020
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

@yangbodong22011 may i ask what's the trigger to adding this test? i understand we're lacking coverage, but i assume there was some reason for adding it now.

while you're at it, maybe you can also add coverage for ACL SAVE and ACL LOAD

@ShooterIT
Copy link
Member

ShooterIT commented Sep 9, 2020

Yes, please also add ACL SAVE and ACL LOAD tests. @yangbodong22011 One case we need cover is that the loaded ACL file has the user that current client is using, please see #7202

@yangbodong22011
Copy link
Contributor Author

yangbodong22011 commented Sep 9, 2020

@yangbodong22011 may i ask what's the trigger to adding this test?

I wanted to use aclfile, but found that it lacked tests, so I added tests.
@ShooterIT @oranagra OK, I will add ACL SAVE and ALC LOAD test.

@yangbodong22011 yangbodong22011 force-pushed the feature-add-aclfile-test branch 2 times, most recently from cf334ad to 7caeef4 Compare September 9, 2020 13:36
oranagra
oranagra reviewed Sep 9, 2020
View reviewed changes
tests/unit/acl.tcl Outdated
Copy link
Member

@oranagra oranagra Sep 9, 2020

Choose a reason for hiding this comment

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

these assertions are not needed.. if the command errors, the test will fail anyway (throws an exception).
you need this only if you're expecting different success responses (e.g. a number of an array)

Copy link
Contributor Author

@yangbodong22011 yangbodong22011 Sep 9, 2020

Choose a reason for hiding this comment

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

Thanks, already updated.

@yangbodong22011 yangbodong22011 force-pushed the feature-add-aclfile-test branch from 7caeef4 to 97ac74d Compare September 9, 2020 14:06
@oranagra oranagra changed the title Tests: Add aclfile load tests Tests: Add aclfile load and save tests Sep 9, 2020
@oranagra oranagra merged commit 0666267 into redis:unstable Sep 9, 2020
oranagra pushed a commit that referenced this pull request Sep 10, 2020
@yangbodong22011 @oranagra
Tests: Add aclfile load and save tests (#7765)
b55b0ea 
improves test coverage

(cherry picked from commit 0666267)
@ShooterIT ShooterIT mentioned this pull request Sep 17, 2020
Merged
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 4, 2020
@yangbodong22011
Tests: Add aclfile load and save tests (redis#7765)
9748117 
improves test coverage
jschmieg pushed a commit to memKeyDB/memKeyDB that referenced this pull request Nov 6, 2020
@yangbodong22011 @jschmieg
Tests: Add aclfile load and save tests (redis#7765)
f57796f 
improves test coverage

(cherry picked from commit 0666267)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yangbodong22011 @ShooterIT @oranagra