Code Review by team 2 #2
Description
Что не понравилось:
-
Некоторые методы имеют слишком длинные названия, что неудобно: iSeeAnAlertThatTheMusicHasBeenAdded или theUserThatTheLinkPointsToIsInMyFriends;
-
Использовать реальные данные (логин, пароль) активных пользователей (не ботов) и загружать их на открытую площадку, на наш взгляд, не лучшая идея;
-
Селекторы для вкладки "Сообщения" можно сделать короче. Хоть теневой DOM позволяет использовать абсолютные пути, можно найти другие способы доступа к нужным элементам. Такие длинные селекторы весьма негибкие для случая изменения относительного положения элементов;
-
Не все тесты имеют Assert-ы. Хотя эти тесты и проверяют что-то (через shouldBe, например), но теряется смысл теста: нет конкретной проверки на тему теста.
-
Перенос логики теста в Page Object классы:
а) FriendsPage.getSpecificFriends - почему бы не получать список String ссылок(в идеале в виде wrapper объектов карт друга) и уже в тесте реализовывать логику, ведь если потребуется чуток изменить тест, то придется дописывать ещё метод. Такая узкая функция не даёт переиспользовать её в других тестах.
б) DialogsPage.successCreateChat - если изменится проверка то придется переписывать/дописывать код POM(Page object models) класса. Если тест упадет, то тестер залезет в successCreateChat, и увидит переменную se3 и ничего не поймет по названию, очень неинформативное название.
в) GroupPage.checkSubscription - с одной стороны хорошо - если вдруг "В группе" поменяется на что-то ещё, то поменять надо будет в одном месте, с другой стороны если ты не состоишь в группе этого элемента просто нет. И при баге состояния в группе тест упадет(хорошо), но причина будет не найденный элемент(это заставит подумать в сторону изменения локаторов, а не бага состояния)
г) MusicPage - POM класс написан для теста, а не наоборот. Из-за этого при малейшем изменении теста придется переписать MusicPage. Можно не хранить название песен в классе, а запоминать его в тесте и под это переписать класс.
-
LoginPage - нет возможности работать с чистой(без неверной попытки входа) страницей авторизации. Если перенести метод open в конструктор, то в будующем со страницей уже можно работать не пытаясь логинится.
-
Assert'ы просто проверяют выражения, нет сообщений об ошибке(CheckCountFriendsTest.iSeeTheNumberOfMyFriends и CheckSpecificFriend.theUserThatTheLinkPointsToIsInMyFriends)
-
Все тестовые проверки(should) внутри POM классов нарушают POM
-
В CheckSubscriptionToGroup.goToGroup захардкожен URL группы и передаётся имя группы, которое не используется. Этот тест можно параметризировать передавая ссылку на группу. Также в этом методе необходимо создавать страницу FriendsPage и присваивать её в переменную класса, чтобы показать, что при открытии страницы вы перешли на страницу группы. И если в конструкторе класса страницы группы будет проверка того что страница подгрузилась, то тест упадет на открытии страницы, а не на следующем тесте. И вообще, в большинстве своем в тестах прерывается путь открытия страниц, то сначала сраница создаеться а потом драйвер туда переходит(CreateNewChatTest), то страница создается не в том методе(CheckSubscriptionToGroup, CheckCountFriendsTest)
-
MainPage, если бы методы этого класса возвращали новые объекты страниц, то работать с ними было бы удобнее(не нужно запоминать какая страница откроется, чтобы вызвать её конструктор). Так ещё это даст(если в будующем в констуркторе страницы появятся проверки) падать тесту при создании страницы, а не где-то в других методах.