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.

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

hotfix(*) credentials endpoints test suites with Cassandra #3051

Merged

Conversation

thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Nov 25, 2017

The recent commits adding endpoints to manipulate the credentials of
authentication plugins had test cases that were flawed when running with
Cassandra.

With Cassandra, the offset query argument must be URL-encoded, and the
offset parameter in the Admin API response will also be present in the
response for the last page.

The recent commits adding endpoints to manipulate the credentials of
authentication plugins had test cases that were flawed when running with
Cassandra.

With Cassandra, the `offset` query argument must be URL-encoded, and the
`offset` parameter in the Admin API response will also be present in the
response for the last page.
@thibaultcha
Copy link
Member Author

@hbagdi Would you mind reviewing this change?

Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

I just tested this.
Like you said, the last page still returns an offset and the next URI which return an empty data with no offset.

A better test would be to check if an offset is present, and then GET it to make sure that the page is empty.

If there exist elaborative test cases for testing the pagination logic in other modules, then we could skip.
I can help add a patch to this PR. Let mae know.

Also, I'm wondering why the CI tests passed then.. since, we don't mock Cassandra.

@thibaultcha
Copy link
Member Author

A better test would be to check if an offset is present, and then GET it to make sure that the page is empty.

The pagination feature is tested in other locations, this present test is more to ensure that 2 subsequent pages are indeed different.

Also, I'm wondering why the CI tests passed then.. since, we don't mock Cassandra.

Unfortunately as of today, all of our tests do not run on both databases yet. This has already changed in current WIP branches we will be releasing soon. Sorry about that!

Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

Look good!
Thanks again for taking care of this.

@thibaultcha thibaultcha merged commit c077250 into master Nov 27, 2017
@thibaultcha thibaultcha deleted the hotfix/credentials-endpoints-tests-with-cassandra branch November 27, 2017 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants