Feature/auth 002 by AlexandrSmolyachkovGH · Pull Request #4 · AlexandrSmolyachkovGH/skill_tracker_auth

AlexandrSmolyachkovGH · 2025-06-30T12:57:31Z

No description provided.

mitrandir-pl

Всё надо повыносить в фикстуры. К тестам надо добавить коменты, неясно что вообще тестируется в некоторых.

mitrandir-pl · 2025-07-01T06:19:19Z

+def test_generate_email_payload() -> None:
+    email_payload = ses_handler.generate_email_payload(
+        message="Test message",
+        subject="Test subject"
+    )
+    assert isinstance(email_payload, EmailPayloadScheme)
+    assert isinstance(email_payload.message, str)
+    assert isinstance(email_payload.subject, str)


что проверяет этот тест? ты по сути проверил что "Test message" и "Test subject" это строки. Чего ради?

def generate_email_payload(
self,
message: str,
subject: str,
source: str | None = None,
) -> EmailPayloadScheme:
payload = {
'message': message,
'subject': subject,
}
if source:
payload['source'] = source
return EmailPayloadScheme(**payload) функция просто генерирует пейлоад для сообщения, она отдает EmailPayloadScheme где есть поля релевантные тем, что проверялись в тесте, т.е. мне казалось, что нужно проверить: а) создание ответа, б) поля в ответе, но так как EmailPayloadScheme - пайдантик модель - пункт б оверхед. Получается, достаточно оставить что на выходе у меня генерится результат инстанс от модели.

mitrandir-pl · 2025-07-01T06:21:39Z

+    with patch.object(ses_handler, "generate_email_payload") as mock_gen_payload, \
+        patch.object(ses_handler, "send_email", new_callable=AsyncMock) as mock_send_email, \
+        patch.object(ses_handler, "generate_otp") as mock_generate_otp:
+        mock_send_email.return_value = None
+        mock_generate_otp.return_value = "TestOTP123"
+        mock_gen_payload.return_value = EmailPayloadScheme(
+            message="Test message",
+            subject="Test subject",
+            source="sender@example.com",
+        )


ты замокал кучу объектов. Если будут добавляться новые тесты, придётся в них повторять этот код, и так множество раз. Такое надо выносить в фикстуры

понял, поправил

mitrandir-pl · 2025-07-01T06:22:34Z

+    with patch.object(ses_handler, "generate_otp") as mock_generate_otp, \
+        patch.object(ses_handler, "send_email", new_callable=AsyncMock) as mock_send_email, \
+        patch.object(msg_creator, "get_ses_confirmation_message") as mock_get_ses_message, \
+        patch.object(redis_client, "set", new_callable=AsyncMock) as mock_set_redis:
+        mock_generate_otp.return_value = "TestOTP123"
+        mock_get_ses_message.return_value = {
+            "message": "test message",
+            "subject": "test subject",
+            "response_message": "test response",
+        }


аналогично, такое надо в фикстуры выносить

поправил

mitrandir-pl · 2025-07-01T06:29:26Z

+auth_data = AuthUserScheme(
+    **{
+        "email": "example@email.com",
+        "password_hash": "examplePassword123!",
+    }
+)


Тут вообще не понимаю в чём прикол? Зачем так объект класса создавать ??

Suggested change

auth_data = AuthUserScheme(

**{

"email": "example@email.com",

"password_hash": "examplePassword123!",

}

)

auth_data = AuthUserScheme(

email="example@email.com",

password_hash="examplePassword123!",

)

согласен, не вспомню уже причину такого оформления)

mitrandir-pl · 2025-07-01T06:30:45Z

+    }
+)
+refresh_orm = RefreshTokenORM(
+    id=UUID("cfe82c6f-7d8c-4172-bc33-04a24a065c20"),


Suggested change

id=UUID("cfe82c6f-7d8c-4172-bc33-04a24a065c20"),

id=uuid.uuid4(),

mitrandir-pl · 2025-07-01T06:40:02Z

Почему ты в этом файле делаешь какие-то объекты для тестов. Это же всё надо было делать в фикстурах

В тестах нет аннотаций типов, надо добавить

mitrandir-pl · 2025-07-01T06:56:15Z

+        return_value=token_data,
+    )
+    mock_dependencies["token_repo"].update_refresh.return_value = refresh_orm
+    token_service = TokenService(**mock_dependencies)
+    res = await token_service.exchange_refresh_token(token_data=token_data)


тесты получились крайне непонятными, что за токен дата, откуда она взялась. Аннотаций типов нет, коментов нет.

mitrandir-pl · 2025-07-01T06:59:48Z

+        "auth_app.services.users.msg_creator.get_code_message",
+        return_value="Code sent",
+    )
+    user_service = UserService(**mock_dependencies)


опять-таки, ты в каждом тесте этот сервис создаёшь, почему тогда не вынести это в фикстуру?

mitrandir-pl · 2025-07-01T07:04:48Z

+def test_hash_password() -> None:
+    hashed_password = hash_password(pwd)
+    assert isinstance(hashed_password, str)


нет никакого смысла проверять тип, проверять надо логику

тут сложно что-то глубоко проверить тк на базе либы хеширую, т.е. я базово чекнул что результат хеширования нужный мне тип и добавил верификацию в данный тест

mitrandir-pl · 2025-07-01T07:05:37Z

+email = "email@example.com"
+pwd = "12345Password!"
+hashed_pwd = hash_password(pwd)


почему это не фикстуры

mitrandir-pl · 2025-07-03T14:25:15Z

+        self.__redis = redis  # pylint: disable=W0238
+        self.__ses = ses  # pylint: disable=W0238


А зачем тебе поля которые ты не используешь?

почему-то думал, что буду их тоже юзать, поэтому делал заглушки. сейчас убрал.

mitrandir-pl · 2025-07-03T14:28:49Z

+@pytest.fixture
+def mock_dependencies() -> dict:
+    return {
+        "user_repo": Mock(
+            create_user=AsyncMock(),
+            get_user=AsyncMock(),
+            update_user=AsyncMock(),
+            get_users=AsyncMock(),
+        ),
+        "token_repo": Mock(
+            create_refresh=AsyncMock(),
+            get_refresh=AsyncMock(),
+            update_refresh=AsyncMock(),
+        ),
+        "redis": Mock(),
+        "ses": Mock(
+
+        ),
+    }


видимо забыл удалить, но старайся перепроверять такие штуки

точно, сори, поправил

mitrandir-pl · 2025-07-03T14:32:32Z

+@pytest.fixture(scope="session")
+def user_id() -> UUID:
+    return uuid4()


я не вижу ни одного теста для которого это было бы необходимо. Можно просто обойтись uuid4()

идея была в том, чтобы просто 1 раз создать uuid и переиспользовать его, как это делаю с имейлом, паролем и другими данными для юзера. пусть будет так в этом сервисе, если ты не против, в других учту момент

mitrandir-pl · 2025-07-03T14:45:08Z

+    assert email_payload.message == "Test message"
+    assert email_payload.subject == "Test subject"


непонятно откуда данные вылазят
правильнее было бы так

Suggested change

assert email_payload.message == "Test message"

assert email_payload.subject == "Test subject"

assert email_payload.message == generate_email_payload_data["message"]

assert email_payload.subject == generate_email_payload_data["subject"]

полностью поддерживаю, проглядел

mitrandir-pl · 2025-07-03T14:45:27Z

+
+
+def test_generate_email_payload(
+    generate_email_payload_data,


почему нет аннотаций типов

все поправил

mitrandir-pl · 2025-07-03T14:46:54Z

+    send_email_patch,
+    get_user_data,


Аннотации типов, и так везде

все поправил

mitrandir-pl · 2025-07-03T14:58:36Z

+    role_auth_data = RoleDataScheme(
+        **auth_user_data_mock.model_dump(),
+    )


здесь точно надо так создавать объект? а то опять через словарь и нифига неясно какие там поля

в этом, мне кажется, есть логика, поясню:
RoleDataScheme - расширенная схема AuthUserScheme (где кроме имейла и пароля есть ролевые данные),
но они не являются обязательными и имеют дефолты, поэтому я от базового класса (он уже мокнут) делаю дамп для дикта и просто прокидываю данные необходимые, а остальные - дефолт тянут

я ничего не понял. Переделай под нормальное создание. Явность всегда важнее чем то что ты описываешь. А здесь нифига не явно. Плюс, меняется схема -> ломаются такие создания объектов. А что там сломалось - непонятно, так как создание неявное

mitrandir-pl · 2025-07-07T06:10:07Z

+    env:
+      #POSTGRES
+      POSTGRES_DB: default_db
+      POSTGRES_USER: default_user
+      POSTGRES_PASSWORD: default_password
+      POSTGRES_HOST: localhost
+      POSTGRES_PORT: '5438'
+      # REDIS
+      REDIS_PASSWORD: default_password
+      REDIS_PORT: 6380
+      REDIS_HOST: localhost
+      # PASSWORD HASHING
+      HASHING_ALGORITHM: bcrypt
+      HASHING_DEPRECATED: auto
+      # JWT SECRET KEY
+      KEY: secret_key
+      ALGORITHM: HS256
+      REFRESH_LASTING: 3000
+      ACCESS_LASTING: 300
+      ADMIN_SECRET: default_secret
+      # LOCALSTACK
+      SERVICES: ses,s3
+      AWS_DEFAULT_REGION: us-east-1
+      LOCALSTACK_HOST: localstack
+      DEBUG: 1
+      AWS_ACCESS_KEY_ID: test
+      AWS_SECRET_ACCESS_KEY: test
+      # LOCAL AWS
+      AWS_ENDPOINT: http://localhost:4566
+      RESET_PWD_LENGTH: 10
+      VERIFICATION_CODE_LENGTH: 5


почему переменные энва прямо в файле хранятся

mitrandir-pl · 2025-07-07T06:11:09Z

+      - name: Install requirements
+        run: |
+          python -m pip install --upgrade pip
+          pip install poetry
+          poetry config virtualenvs.create false
+          poetry install


надо ещё покрытие проверять, если < 80, то пайплайн должен падать

mitrandir-pl · 2025-07-07T06:15:13Z

+    role_auth_data = RoleDataScheme(
+        **auth_user_data_mock.model_dump(),
+    )


я ничего не понял. Переделай под нормальное создание. Явность всегда важнее чем то что ты описываешь. А здесь нифига не явно. Плюс, меняется схема -> ломаются такие создания объектов. А что там сломалось - непонятно, так как создание неявное

AlexandrSmolyachkovGH added 3 commits June 27, 2025 00:56

feat: Create tests.

64b2ad9

test: Adding new tests

21ab00f

tests: Add new tests for token service.

6bc1dfd

AlexandrSmolyachkovGH requested a review from mitrandir-pl June 30, 2025 15:22

mitrandir-pl reviewed Jul 1, 2025

View reviewed changes

AlexandrSmolyachkovGH added 3 commits July 2, 2025 00:40

tests: Test refactoring

f6996a2

tests: Refactoring of token_service tests

4b2a7be

tests: One more part of test refactoring

83923eb

mitrandir-pl reviewed Jul 3, 2025

View reviewed changes

AlexandrSmolyachkovGH added 2 commits July 3, 2025 23:55

tests: Test refactoring

f5a371d

feature: Create .yml for github actions

692ea56

mitrandir-pl reviewed Jul 7, 2025

View reviewed changes

AlexandrSmolyachkovGH added 3 commits July 8, 2025 00:42

feat: Add access token verification router and relevant tests

31ce0ef

feat: Add CreateUser feature on the core side. Other edits.

8b6226c

feat: add soft_delete routers

edd1dfb

	id=UUID("cfe82c6f-7d8c-4172-bc33-04a24a065c20"),
	id=uuid.uuid4(),

		self.__redis = redis # pylint: disable=W0238
		self.__ses = ses # pylint: disable=W0238

		assert email_payload.message == "Test message"
		assert email_payload.subject == "Test subject"



		def test_generate_email_payload(
		generate_email_payload_data,

Conversation

AlexandrSmolyachkovGH commented Jun 30, 2025

Uh oh!

mitrandir-pl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!