Add validate_usernames method #8
Conversation
includes caching. Also renames clients for less confusing stack traces
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
===========================================
+ Coverage 99.07% 100.00% +0.92%
===========================================
Files 4 4
Lines 217 269 +52
===========================================
+ Hits 215 269 +54
+ Misses 2 0 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/kbase/auth/_async/client.py
Outdated
| raise ValueError("timer is required") | ||
| self._token_cache = LRUCache(maxsize=cache_max_size, timer=timer) | ||
| self._user_cache = LRUCache(maxsize=cache_max_size, timer=timer) | ||
| self._user_name_cache = LRUCache(maxsize=cache_max_size, timer=timer) |
There was a problem hiding this comment.
minor thing, but it's slightly annoying to have usernames and user_name. Can we pick a way to refer to the names of users and stick to it?
src/kbase/auth/_async/client.py
Outdated
| if not tk: # minor optimization, don't get the token until it's needed | ||
| tk = await self.get_token(token) | ||
| # Usernames are permanent but can be disabled, so we expire based on time | ||
| # Don't cache non-existant names, could be created at any time and would |
src/kbase/auth/_async/client.py
Outdated
| exists = u in res | ||
| to_return[u] = exists | ||
| if exists: |
There was a problem hiding this comment.
can you just do
to_return[u] = u in res
if to_return[u]:| async def test_validate_usernames_noop(auth_users): | ||
| with KBaseAuthClient.create(AUTH_URL) as cli: | ||
| res1 = cli.validate_usernames(auth_users["user"]) | ||
| res2 = cli.validate_usernames(auth_users["user"], " \t ") | ||
| async with await AsyncKBaseAuthClient.create(AUTH_URL) as cli: | ||
| res3 = await cli.validate_usernames(auth_users["user"]) | ||
| res4 = await cli.validate_usernames(auth_users["user"], " ") | ||
|
|
||
| assert res1 == {} | ||
| assert res2 == {} | ||
| assert res3 == {} | ||
| assert res4 == {} |
There was a problem hiding this comment.
looks like a good candidate for parametrize
There was a problem hiding this comment.
Since half the methods are sync and half are async I'm not really sure about that. If it was 4 operations that were all the same I'd agree
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_token_basic_fail(auth_users): | ||
| async def test_get_token_fail(auth_users): |
There was a problem hiding this comment.
Are you able to generate sync tests from the async template in the same way that you have done the code itself? There's a lot of repetition -- doing things the async way and then doing them the sync way.
There are also places where you could parametrize the tests but haven't done so.
There was a problem hiding this comment.
Maybe? I thought about that but decided I'd rather make it blindingly obvious to future devs that they needed to test both the sync and async versions of the code by putting the tests side by side.
Also, from personal experience, maintaining both isn't that hard
|
I suspect that changes are going to be few and far between once the basics are done |
includes caching.
Also renames clients for less confusing stack traces