Skip to content

Fix/issue170#171

Open
CaseAsLimbo wants to merge 10 commits into
mainfrom
fix/issue170
Open

Fix/issue170#171
CaseAsLimbo wants to merge 10 commits into
mainfrom
fix/issue170

Conversation

@CaseAsLimbo

@CaseAsLimbo CaseAsLimbo commented May 19, 2026

Copy link
Copy Markdown

Изменения

  • Был исправлен баг из-за которого после падения не анониманого комментария с ERROR падало 57 тестов
  • Была исправлена причина падения теста test_create_comment с тестовым набором включающим не анонимные комментарии
  • Добавлена проверка на корректность переданных в userdata параметров
  • Добавлен uv.lock в .gitignore

Детали реализации

  • Баг из-за которого падало 57 тестов с ERROR связан с не очевидным поведением sqlalchemy, где на основе того, были ли подготовлены на вставку (INSERT) в текущей не завершенной транзакции одновременно записи в таблицах LecturerUserComment и Comment. Если тест падал с TypeError и успевала добавиться только одна из этих таблиц, алхимия в tear-down фикстуры lecturers в итоговый SQL запрос не добавляла строку на удаление записи LecturerUserComment - сразу после этого происходило удаление лектора из таблицы Lecturer, что приводило к конфликту ведь не удаленный LecturerUserComment по-прежнему ссылался на него.
    Решение: В модели Lecturer и LectureUserComment добавил атрибуты связи relationship с параметром cascade, где явно указал удалять сначала дочерние таблицы.
  • Тест test_create_comment с не анонимаными комментариями падал, из-за того, что для user-а передавалась не корректная userdata. Были добавлены мок фикстуры authlib_user, authlib_mock и user_mock для подмены данных user-а возвращаемых UnionAuth

Check-List

  • Вы проверили свой код перед отправкой запроса?
  • Вы написали тесты к реализованным функциям?
  • Вы не забыли применить форматирование black и isort для Back-End или Prettier для Front-End?

@CaseAsLimbo CaseAsLimbo requested a review from petrCher May 19, 2026 19:17
@CaseAsLimbo CaseAsLimbo linked an issue May 19, 2026 that may be closed by this pull request
@github-actions

github-actions Bot commented May 19, 2026

Copy link
Copy Markdown

Code Coverage

Coverage Report
FileStmtsMissCoverMissing
rating_api
   __main__.py440%1–7
   exceptions.py43784%35–37, 48–50, 58
rating_api/models
   base.py64494%24–27
   db.py1641889%118, 120, 122, 124, 135, 174, 192, 202, 216, 225–227, 261, 275–287
rating_api/routes
   base.py16194%41
   comment.py1513577%77, 95, 144–145, 167–176, 192, 265–266, 268–269, 277–282, 291, 298–300, 330, 353, 393–404, 431
   exc_handlers.py32391%36, 43, 50
   lecturer.py109992%72–73, 83, 89–90, 230, 238, 260, 266
rating_api/schemas
   base.py12467%6–9
   models.py155398%189, 191, 201
rating_api/utils
   mark.py880%1–20
TOTAL7929688% 

Summary

Tests Skipped Failures Errors Time
88 0 💤 0 ❌ 0 🔥 20.572s ⏱️

Comment thread .gitignore

# Pyre type checker
.pyre/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

нигде не используется в текущей реализации

Comment thread tests/conftest.py
lecturers[-1].is_deleted = True
for lecturer in lecturers:
dbsession.add(lecturer)
dbsession.add_all(lecturers)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

думаю, что это можно не добавлять, зачем

Comment thread tests/conftest.py
"user_scopes": [{"id": 0, "name": "string", "comment": "string"}],
"indirect_groups": [{"id": 0, "name": "string", "parent_id": 0}],
"groups": [{"id": 0, "name": "string", "parent_id": 0}],
def authlib_user():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

верни исходную структуру, как было все
надо добавить только
"userdata": [ {"category": "Личная информация", "param": "Полное имя", "value": "Тестовый Тест"}, ],
что собственно ты и сделал, остальное не нужно

Comment thread tests/conftest.py
}


@pytest.fixture()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

на разные фикстуры тоже разбивать это не надо

def test_create_comment(client, dbsession, lecturers, body, lecturer_n, response_status):
params = {"lecturer_id": lecturers[lecturer_n].id}
post_response = client.post(url, json=body, params=params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

это не нужно будет при правильном моке(см ниже)

@@ -177,6 +178,16 @@
def test_create_comment(client, dbsession, lecturers, body, lecturer_n, response_status):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

добавь здесь в конце assert на проверку грамотности некоторых полей по типу что действительно при неанонимном комменте ставится имя не null

import logging

import pytest
from auth_lib.fastapi import UnionAuth

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

надо будет убрать, см комменты ниже

@petrCher

Copy link
Copy Markdown
Member
Снимок экрана — 2026-06-24 в 15 57 05 у меня еще локально при тестировании такие ошибки, у тебя есть? проблема в том что есть незамоканный вызов по https чего не должно быть при тестировании. мы должны без http запросов работать, для этого и созданы моки В общем посмотри, подумай что не так и как лучше исправить

Comment thread rating_api/models/db.py
avatar_link: Mapped[str] = mapped_column(String, nullable=True, comment="Ссылка на аву препода")
timetable_id: Mapped[int]
comments: Mapped[list[Comment]] = relationship("Comment", back_populates="lecturer")
lecturer_user_comments: Mapped[list[LecturerUserComment]] = relationship(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

каскадное удаление нам не нужно, relationship можно не делать, предположительно проблема в самих эндпоинтах в этот раз
Честно немного не понял и запутался что не так с логикой и почему из-за этого 57 тестов падает, объясни пожалуйста)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

и еще, у нас же софтделиты, поэтому каскад точно не нужен
каскад это для хардделита

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Исправить падающие тесты

2 participants