Conversation
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Общий вывод по проекту
Представленный проект демонстрирует хороший уровень владения инструментами тестирования (JUnit 5, Mockito) и понимание веб-архитектуры на базе сервлетов. Тесты покрывают основные позитивные и негативные сценарии, включая обработку исключений и валидацию данных.
Основные области для развития:
Архитектура: Повторяющаяся проверка авторизации в каждом контроллере указывает на необходимость внедрения фильтров (Filters).
Чистый код: Рекомендуется избегать дублирования в тестах с помощью @ParameterizedTest и вынести работу с сессионными ключами в строгие константы .
Инкапсуляция: Логика определения состояния игры (победа/поражение) должна принадлежать доменным моделям, а не контроллерам или тестам.
Проект выглядит крепким, структурированным и готовым к дальнейшему расширению. Продолжайте изучать паттерны проектирования и возможности современной Java для написания еще более элегантного кода!
Итоговая оценка: A
|
|
||
| public class EditQuestIT extends BaseIT { | ||
|
|
||
| private final QuestService questService = mock(QuestService.class); |
There was a problem hiding this comment.
Инициализация моков вручную в полях класса снижает гибкость тестов. Рекомендуется использовать аннотацию @mock совместно с @ExtendWith(MockitoExtension.class) для автоматизации процесса. [INFO]
|
|
||
| @Test | ||
| @DisplayName("when GET request admin authorized no questId then set edit false and default JSON") | ||
| void whenGetNoQuestId_ThenSetEditFalseAndDefaultJson() { |
There was a problem hiding this comment.
Название метода теста 'whenGetNoQuestId_ThenSetEditFalseAndDefaultJson' содержит подчеркивание, что нарушает camelCase конвенцию Java. Следует придерживаться единого стиля именования без подчеркиваний. [INFO]
| @Test | ||
| @DisplayName("when GET request admin authorized no questId then set edit false and default JSON") | ||
| void whenGetNoQuestId_ThenSetEditFalseAndDefaultJson() { | ||
| doNothing().when(authService).checkAdminAuthorization(req, EDIT_QUEST_AUTH_ERROR); |
There was a problem hiding this comment.
Использование doNothing() для методов, которые и так ничего не возвращают (void), является избыточным в Mockito, так как это поведение по умолчанию. [INFO]
| assertEquals(editQuest.getView(), resultView); | ||
| verify(req).setAttribute(eq(EDIT), eq(true)); | ||
| assertNull(req.getAttribute(QUEST_JSON)); | ||
| assertThrows(IOException.class, () -> questMapper.toJsonString(testQuest)); |
There was a problem hiding this comment.
В одном тестовом методе проверяется и возврат представления, и выброс исключения через анонимную функцию. Тест должен проверять один конкретный сценарий (Single Responsibility Principle для тестов). [WARNING]
| when(req.getParameter(QUEST_ID)).thenReturn(questIdStr); | ||
| when(questService.getValidatedQuest(questIdStr)).thenReturn(Optional.empty()); | ||
|
|
||
| when(questService.getAll()).thenReturn(List.of()); |
There was a problem hiding this comment.
Вызов questService.getAll() кажется лишним для сценария 'Quest Not Found'. Следует убедиться, что тесты не перегружены лишними действиями, не относящимися к проверяемому случаю. [INFO]
|
|
||
| assertEquals(expectedUrl, redirectUrl); | ||
| verify(req).setAttribute(GAME, testGame); | ||
| verify(req).setAttribute(USER, testUser); |
There was a problem hiding this comment.
Установка атрибута USER в запрос в методе doPost выглядит странно, так как пользователь обычно уже находится в сессии. Стоит проверить необходимость этого действия. [WARNING]
| verify(authService).checkAdminAuthorization(req, EDIT_QUEST_AUTH_ERROR); | ||
| verify(req).setAttribute(eq(QUESTS), anyList()); | ||
| verify(req).setAttribute(eq(EDIT), eq(false)); | ||
| verify(req).setAttribute(eq(QUEST_JSON), eq(JSON_SAMPLE)); |
There was a problem hiding this comment.
Использование магической строки JSON_SAMPLE. Если структура JSON изменится, тесты упадут. Лучше генерировать JSON из тестовых объектов динамически. [WARNING]
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| public class RegisterIT extends BaseIT { |
There was a problem hiding this comment.
Класс RegisterIT наследуется от BaseIT, но не использует @ExtendWith(MockitoExtension.class), в отличие от HomeIT. Желательно привести все тестовые классы к единому стилю. [INFO]
| verify(req).getSession(); | ||
| verify(session).getAttribute(Key.USER); | ||
| verify(statsService).getUserStats(testUser.getId()); | ||
| verify(req, never()).setAttribute(eq(Key.STATS), any()); |
There was a problem hiding this comment.
Проверка verify(never()) полезна для подтверждения того, что данные не передаются на фронтенд при ошибках. Это хороший подход к безопасности. [INFO]
| assertEquals(login.getView(), redirect); | ||
| verify(session).setAttribute(Key.ERROR, Value.INVALID_DATA_ERROR); | ||
| verify(userService).login(testAdmin.getEmail(), invalidPassword); | ||
| verifyNoMoreInteractions(userStatsService); |
There was a problem hiding this comment.
Использование verifyNoMoreInteractions помогает гарантировать отсутствие лишних вызовов к сервисам, что важно для производительности и корректности логики. [INFO]
2 participants
No description provided.