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

RedisStorage major fixes + tests updates #676

Merged
merged 6 commits into from
Sep 5, 2021

Conversation

Olegt0rr
Copy link
Contributor

@Olegt0rr Olegt0rr commented Aug 30, 2021

Description

Forget to add async for Redis .close() coro.
Thanks @darksidecat for comment #649 (review)

By the way fixed some bugs and fixed tests.

  • adapter redis init
  • await close
  • string typing (to prevent aioredis compatibility issues)
  • skip tests for RedisStorage v1 with aioredis v2
  • fixed lazy fixture issue
  • fixed connection pool test issue

Sorry for large PR, but only merge of all of this issue fixes make able to test redis support correctly.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • Operating System:
  • Python version:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@Olegt0rr Olegt0rr requested a review from a team August 30, 2021 14:05
@Olegt0rr Olegt0rr added the bug Something is wrong with the framework label Aug 30, 2021
@Olegt0rr Olegt0rr added this to the 2.15 milestone Aug 30, 2021
@Olegt0rr Olegt0rr mentioned this pull request Aug 30, 2021
7 tasks
@darksidecat
Copy link
Contributor

darksidecat commented Aug 31, 2021

See that redis_storage_2 tests failing with

self = <aiogram.contrib.fsm_storage.redis.AioRedisAdapterV1 object at 0x7f78aa61c9d0>
name = 'fsm:1234:1234:data', kwargs = {}

    async def get(self, name, **kwargs):
>       return await self._redis.get(name, encoding="utf8", **kwargs)
E       AttributeError: 'NoneType' object has no attribute 'get'

../aiogram/contrib/fsm_storage/redis.py:310: AttributeError

image

Seems like await self.get_redis() should be called in some of AioRedisAdapterBase, AioRedisAdapterV1 methods for settling self._redis

@Olegt0rr
Copy link
Contributor Author

@darksidecat, fixed, try again :)

@Olegt0rr Olegt0rr changed the title Redis async close Redis async close + tests update Aug 31, 2021
@Olegt0rr
Copy link
Contributor Author

Olegt0rr commented Aug 31, 2021

with aioredis>=2

Screenshot 2021-09-01 at 01 12 21

with aioredis<2

Screenshot 2021-09-01 at 01 14 50

@Olegt0rr Olegt0rr changed the title Redis async close + tests update RedisStorage major fixes + tests updates Sep 1, 2021
@JrooTJunior JrooTJunior merged commit 3ee2828 into dev-2.x Sep 5, 2021
@JrooTJunior JrooTJunior deleted the bugfix/redis-close-async branch December 19, 2021 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong with the framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants