Add keycloak tests #3157
Add keycloak tests #3157
Conversation
tylerpotts
requested review from
dcmcand and
viniciusdc
and removed request for
a team
|
@viniciusdc @dcmcand It looks like I misinterpreted the green check mark to mean these tests passed in the CI. They are passing locally. Once the "Await Workloads" test is fixed on the main branch I'll pull that into this branch. Then the CI running these in the following "Deployment Pytests" will run and I can confirm things are green |
|
Thanks, @tylerpotts. I am currently looking into the issue with await_workloads and hope to get it passing in a few minutes. |
|
The CI test is now fixed, small misjudgment on my part |
| f"users/{user_id}", method="PUT", json_data=user_data | ||
| ) | ||
|
|
||
| def delete_user(self, user_id: str) -> requests.Response: |
There was a problem hiding this comment.
This goes for all the delete functions, please add verification that resources are actually deleted. These tests should be able to be able to be run against a live instance without leaving resources behind
There was a problem hiding this comment.
I've added deletion verification and auto-delete fixtures
There was a problem hiding this comment.
Hey Tyler,
Thanks for this, it looks great overall. I have added inline notes where I think there are changes to be made.
also, can you add requests to pyproject.toml since it is now a test dependency?
Please add type hints throughout as well.
What happens if the tests fail before cleanup? Are we left with a bunch of orphan resources? not a problem for CI but running against live instances that would be an issue
| token_data = keycloak_api.authenticate(test_username, old_password) | ||
| assert "access_token" in token_data | ||
| except requests.exceptions.HTTPError: | ||
| # Some Keycloak configurations may not allow password auth immediately |
There was a problem hiding this comment.
❓ I am confused by this, can you elaborate?
There was a problem hiding this comment.
Some keycloak configurations for users require the user to reset the initial password before first login, and I think that's why this was added by Claude.
I tested with this try/except removed, and it passed. I think this is an LLM artifact and isn't needed.
| verify_ssl: bool, | ||
| ) -> KeycloakAPI: | ||
| """Create an authenticated KeycloakAPI instance for admin operations.""" | ||
| api = KeycloakAPI( |
There was a problem hiding this comment.
What happens if the token expires mid test (say the tests are slow)? Do we have token renewal handled for these tests?
There was a problem hiding this comment.
That's a fair point. I think the default is 5 mins, but adding auto-refresh isn't difficult.
I've added auto-refresh into the KeycloakAPI class
| @@ -0,0 +1,1845 @@ | |||
| """Keycloak API endpoint tests.""" | |||
There was a problem hiding this comment.
Can each of these tests be run independently without colliding with others? Could we run them in parallel to speed things up?
There was a problem hiding this comment.
The only thing that I see as an issue is that they all share the same API instance. Since it now has auto-refresh, it would technically be possible for one worker to overwrite another worker's token mid-test. If I change from session to function for the keycloak_api and authenticated_keycloak_api it alleviates this problem.
I tested locally with the added dependency of pytest-xdist with 10 workers to test the parallel execution. It reduced the testing time from ~20 seconds to ~10 seconds
My thought is that added dependency and complexity of parallel execution isn't worth the benefit we get of 10s faster execution time.
|
|
||
| import requests | ||
|
|
||
| TIMEOUT = 10 |
There was a problem hiding this comment.
Should this be configurable with a default? I am generally leery of hardcoded timeouts
|
|
||
|
|
||
| def decode_jwt_token(token: str) -> dict: | ||
| """Decode a JWT token without verification (for testing purposes). |
There was a problem hiding this comment.
Can we add a stronger warning here. "DO NOT USE THIS FOR PRODUCTION" or something like that. not verifying sigs in test is fine, I just don't want someone copying this pattern later in prod
| keycloak_api.authenticate("invalid_user", "invalid_password") | ||
|
|
||
| # Verify it's a 401 Unauthorized error | ||
| assert exc_info.value.response.status_code == 401 |
There was a problem hiding this comment.
For all the cases where we are using numbers for http status codes, let's switch them to actual http.HTTPStatus objects
assert exc_info.value.response.status_code == HTTPStatus.UNAUTHORIZED for example. That will make the code clearer without needing to look up what a 401 is
| endpoint = f"clients?clientId={client_id}" | ||
| return self._make_admin_request(endpoint) | ||
|
|
||
| def get_client_by_id(self, id: str) -> requests.Response: |
There was a problem hiding this comment.
id is a python builtin, in general it is a bad idea to use builtins for argument names. For all the places you have id as an argument, please change it to something slightly more descriptive that doesn't shadow a built in.
def get_client_by_id(self, client_id: str) -> requests.Response:
3 participants
Reference Issues or PRs
Competes and fixes #3156
What does this implement/fix?
Put a
xin the boxes that applyDocumentation
Testing
How to test this PR?
Any other comments?