Fail redis and mongo tests if incorrect URI provided + some storages …

…tests refactoring (#1510)

* Smaller timeout for MongoStorage connection

By default serverSelectionTimeoutMS=30000. This is too much

* Correct ConnectionError for RedisStorage in tests

* Remove unused import in conftest.py

* Refactor skipping redis and mongo tests

* Fail redis and mongo tests if incorrect URI

If incorrect URIs provided to "--redis" and/or "--mongo" options
tests should fail with ERRORs instead of skipping.
Otherwise the next scenario is possible:
  1) developer breaks RedisStorage and/or MongoStorage code
  2) tests are run with incorrect redis and/or mongo URIs
     provided by "--redis" and "--mongo" options.
     For example, wrong port specified.
  3) tests pass because skipping doesn't fail tests run
  4) developer or reviewer doesn't notice
     that redis and/or mongo tests were skipped
  5) broken code gets in codebase

* Remove unused fixtures passing in storages tests

* Define create_storage_key fixture in conftest.py

* Linters formatting

* Changes description

* Revert "Smaller timeout for MongoStorage connection"

This reverts commit d88b7ec.

* Smaller timeout for MongoStorage connection in tests

The default 30s timeout is too long

* Add test for MongoStorage for 100% coverage

* Linters formatting

* Move skipping redis/mongo tests in earlier fixtures

* Replace vars with constants in conftest.py

* Linters formatting

CHANGES/1510.bugfix.rst

@@ -0,0 +1,11 @@
+Fail redis and mongo tests if incorrect URI provided + some storages tests refactoring
+
+If incorrect URIs provided to "--redis" and/or "--mongo" options tests should fail with errors instead of skipping.
+Otherwise the next scenario is possible:
+  1) developer breaks RedisStorage and/or MongoStorage code
+  2) tests are run with incorrect redis and/or mongo URIsprovided by "--redis" and "--mongo" options (for example, wrong port specified)
+  3) tests pass because skipping doesn't fail tests run
+  4) developer or reviewer doesn't notice that redis and/or mongo tests were skipped
+  5) broken code gets in codebase
+
+Also some refactorings done (related with storages and storages tests).

aiogram/filters/command.py

@@ -3,14 +3,14 @@
 import re
 from dataclasses import dataclass, field, replace
 from typing import (
+    TYPE_CHECKING,
     Any,
     Dict,
     Iterable,
     Match,
     Optional,
     Pattern,
     Sequence,
-    TYPE_CHECKING,
     Union,
     cast,
 )

examples/multibot.py

@@ -4,6 +4,7 @@
 from typing import Any, Dict, Union
 
 from aiohttp import web
+from finite_state_machine import form_router
 
 from aiogram import Bot, Dispatcher, F, Router
 from aiogram.client.session.aiohttp import AiohttpSession
@@ -18,7 +19,6 @@
     TokenBasedRequestHandler,
     setup_application,
 )
-from finite_state_machine import form_router
 
 main_router = Router()
 

tests/conftest.py

@@ -5,19 +5,27 @@
 from pymongo.errors import InvalidURI, PyMongoError
 from pymongo.uri_parser import parse_uri as parse_mongo_url
 from redis.asyncio.connection import parse_url as parse_redis_url
+from redis.exceptions import ConnectionError
 
 from aiogram import Dispatcher
+from aiogram.fsm.storage.base import StorageKey
 from aiogram.fsm.storage.memory import (
     DisabledEventIsolation,
     MemoryStorage,
     SimpleEventIsolation,
 )
 from aiogram.fsm.storage.mongo import MongoStorage
-from aiogram.fsm.storage.redis import RedisEventIsolation, RedisStorage
+from aiogram.fsm.storage.redis import RedisStorage
 from tests.mocked_bot import MockedBot
 
 DATA_DIR = Path(__file__).parent / "data"
 
+CHAT_ID = -42
+USER_ID = 42
+
+SKIP_MESSAGE_PATTERN = 'Need "--{db}" option with {db} URI to run'
+INVALID_URI_PATTERN = "Invalid {db} URI {uri!r}: {err}"
+
 
 def pytest_addoption(parser):
     parser.addoption("--redis", default=None, help="run tests which require redis connection")
@@ -29,37 +37,27 @@ def pytest_configure(config):
     config.addinivalue_line("markers", "mongo: marked tests require mongo connection to run")
 
 
-def pytest_collection_modifyitems(config, items):
-    for db, parse_uri in [("redis", parse_redis_url), ("mongo", parse_mongo_url)]:
-        uri = config.getoption(f"--{db}")
-        if uri is None:
-            skip = pytest.mark.skip(reason=f"need --{db} option with {db} URI to run")
-            for item in items:
-                if db in item.keywords:
-                    item.add_marker(skip)
-        else:
-            try:
-                parse_uri(uri)
-            except (ValueError, InvalidURI) as e:
-                raise UsageError(f"Invalid {db} URI {uri!r}: {e}")
-
-
 @pytest.fixture()
 def redis_server(request):
     redis_uri = request.config.getoption("--redis")
-    return redis_uri
+    if redis_uri is None:
+        pytest.skip(SKIP_MESSAGE_PATTERN.format(db="redis"))
+    else:
+        return redis_uri
 
 
 @pytest.fixture()
 @pytest.mark.redis
 async def redis_storage(redis_server):
-    if not redis_server:
-        pytest.skip("Redis is not available here")
+    try:
+        parse_redis_url(redis_server)
+    except ValueError as e:
+        raise UsageError(INVALID_URI_PATTERN.format(db="redis", uri=redis_server, err=e))
     storage = RedisStorage.from_url(redis_server)
     try:
         await storage.redis.info()
     except ConnectionError as e:
-        pytest.skip(str(e))
+        pytest.fail(str(e))
     try:
         yield storage
     finally:
@@ -71,19 +69,27 @@ async def redis_storage(redis_server):
 @pytest.fixture()
 def mongo_server(request):
     mongo_uri = request.config.getoption("--mongo")
-    return mongo_uri
+    if mongo_uri is None:
+        pytest.skip(SKIP_MESSAGE_PATTERN.format(db="mongo"))
+    else:
+        return mongo_uri
 
 
 @pytest.fixture()
 @pytest.mark.mongo
 async def mongo_storage(mongo_server):
-    if not mongo_server:
-        pytest.skip("MongoDB is not available here")
-    storage = MongoStorage.from_url(mongo_server)
+    try:
+        parse_mongo_url(mongo_server)
+    except InvalidURI as e:
+        raise UsageError(INVALID_URI_PATTERN.format(db="mongo", uri=mongo_server, err=e))
+    storage = MongoStorage.from_url(
+        url=mongo_server,
+        connection_kwargs={"serverSelectionTimeoutMS": 2000},
+    )
     try:
         await storage._client.server_info()
     except PyMongoError as e:
-        pytest.skip(str(e))
+        pytest.fail(str(e))
     else:
         yield storage
         await storage._client.drop_database(storage._database)
@@ -130,6 +136,11 @@ def bot():
     return MockedBot()
 
 
+@pytest.fixture(name="storage_key")
+def create_storage_key(bot: MockedBot):
+    return StorageKey(chat_id=CHAT_ID, user_id=USER_ID, bot_id=bot.id)
+
+
 @pytest.fixture()
 async def dispatcher():
     dp = Dispatcher()

tests/test_fsm/storage/test_isolation.py

@@ -5,12 +5,6 @@
 
 from aiogram.fsm.storage.base import BaseEventIsolation, StorageKey
 from aiogram.fsm.storage.redis import RedisEventIsolation, RedisStorage
-from tests.mocked_bot import MockedBot
-
-
-@pytest.fixture(name="storage_key")
-def create_storage_key(bot: MockedBot):
-    return StorageKey(chat_id=-42, user_id=42, bot_id=bot.id)
 
 
 @pytest.mark.parametrize(

tests/test_fsm/storage/test_mongo.py

@@ -1,17 +1,19 @@
 import pytest
+from pymongo.errors import PyMongoError
 
 from aiogram.fsm.state import State
 from aiogram.fsm.storage.mongo import MongoStorage, StorageKey
-from tests.mocked_bot import MockedBot
+from tests.conftest import CHAT_ID, USER_ID
 
 PREFIX = "fsm"
-CHAT_ID = -42
-USER_ID = 42
 
 
-@pytest.fixture(name="storage_key")
-def create_storage_key(bot: MockedBot):
-    return StorageKey(chat_id=CHAT_ID, user_id=USER_ID, bot_id=bot.id)
+async def test_get_storage_passing_only_url(mongo_server):
+    storage = MongoStorage.from_url(url=mongo_server)
+    try:
+        await storage._client.server_info()
+    except PyMongoError as e:
+        pytest.fail(str(e))
 
 
 async def test_update_not_existing_data_with_empty_dictionary(

tests/test_fsm/storage/test_storages.py

@@ -1,12 +1,6 @@
 import pytest
 
 from aiogram.fsm.storage.base import BaseStorage, StorageKey
-from tests.mocked_bot import MockedBot
-
-
-@pytest.fixture(name="storage_key")
-def create_storage_key(bot: MockedBot):
-    return StorageKey(chat_id=-42, user_id=42, bot_id=bot.id)
 
 
 @pytest.mark.parametrize(
@@ -18,25 +12,23 @@ def create_storage_key(bot: MockedBot):
     ],
 )
 class TestStorages:
-    async def test_set_state(self, bot: MockedBot, storage: BaseStorage, storage_key: StorageKey):
+    async def test_set_state(self, storage: BaseStorage, storage_key: StorageKey):
         assert await storage.get_state(key=storage_key) is None
 
         await storage.set_state(key=storage_key, state="state")
         assert await storage.get_state(key=storage_key) == "state"
         await storage.set_state(key=storage_key, state=None)
         assert await storage.get_state(key=storage_key) is None
 
-    async def test_set_data(self, bot: MockedBot, storage: BaseStorage, storage_key: StorageKey):
+    async def test_set_data(self, storage: BaseStorage, storage_key: StorageKey):
         assert await storage.get_data(key=storage_key) == {}
 
         await storage.set_data(key=storage_key, data={"foo": "bar"})
         assert await storage.get_data(key=storage_key) == {"foo": "bar"}
         await storage.set_data(key=storage_key, data={})
         assert await storage.get_data(key=storage_key) == {}
 
-    async def test_update_data(
-        self, bot: MockedBot, storage: BaseStorage, storage_key: StorageKey
-    ):
+    async def test_update_data(self, storage: BaseStorage, storage_key: StorageKey):
         assert await storage.get_data(key=storage_key) == {}
         assert await storage.update_data(key=storage_key, data={"foo": "bar"}) == {"foo": "bar"}
         assert await storage.update_data(key=storage_key, data={}) == {"foo": "bar"}