search

Anilochka / technopolis-autotesting

0 stars 1 forks source link

Код ревью #16

Open artem24630 opened 2 years ago

artem24630 commented 2 years ago

MainPage

  1. Для однообразия лучше userNameLocator написать как USER_NAME_LOCATOR image
  2. Из селениума можно заюзать LoadableComponent вместо checkIfPresent, чтобы более наглядно было использование паттерна, но можно и так, как сейчас
artem24630 commented 2 years ago
image

Не очень хорошо использовать *

kov-pavel commented 2 years ago

Паттерны:

JUnit и структура:

AlexanderKiselyov commented 2 years ago

Паттерны:

  • Дополнительный обязательный паттерн DataProvider использован некорректно (авторизация под двумя валидными аккаунтами)

JUnit и структура:

  • Таймаут должен быть вынесен в BaseTest, от него унаследовать все тесты (иначе зачем он вообще нужен?), в нём же сделать BeforeAll авторизацию на странице

Паттерн DataProvider в JUnit5 реализован в виде аннотации ParametrizedTest. В JUnit4 это реализуется через аннотации RunWith и DataProvider. Мы решили использовать паттерн DataProvider из JUnit4. Класс BaseTest написан на JUnit5, а класс LoginTest - на JUnit4. Если перенести аннотацию RunWith (по сути эта аннотация - это наследование от класса, указанного в скобках) и отнаследоваться от класса BaseTest, то паттерн DataProvider работать не будет, потому что получится совокупность аннотаций из JUnit4 и JUnit5, которые вместе не работают. Смысл этого тестового класса в том, чтобы проверить, что все пользователи (пары: логин-пароль) валидные в классе UserData, а также в том, чтобы применить данный паттерн. Можно было бы добавить тест и не на валидные данные, но все же считать это за ошибку - неправильно.

Таймаут также был сделан для того, чтобы показать применение паттерна в тестах. Его можно вынести в аннотации для класса BaseTest, но при этом все равно придется переопределять время выполнения всех тестов внутри класса или отдельно взятого тестового метода в этих классах/методах. Поэтому логической ошибки здесь не видим.

kov-pavel commented 2 years ago

1) Так, может быть, напишите всё в едином стиле на JUnit5? Или на JUnit4? 2) Использование паттерна не должно быть самоцелью, Вы используете паттерн, потому что он Вам нужен и без него никак, а не просто так. Всё ещё настаиваю на том, что DataProvider в Вашем случае должен проверять валидный и невалидный аккаунты, иначе основания для проведения теста я не нахожу. 3) У меня есть стойкое ощущение, что для всех тестов без исключения, которые представлены, есть некая верхняя грань по времени выполнения. Почему бы её не обозначить в родительском тестовом классе и нигде боле не переопределять?

AlexanderKiselyov commented 2 years ago
  1. Мы вкладывали идею именно касательно DataProvider в наш проект. Да, писать и на JUnit4, и на JUnit5 не очень хорошая идея, но конкретно в рамках нашего курса не вижу в этом больших проблем, тем более аналог ParametrizedTest для JUnit5 - это отдельное дополнительное требование в задании.
  2. Паттерны не всегда используются из-за того, что без них не обойтись, в некоторых случаях они лишь приводят к более читабельному коду и удобству его написания, сокращения количества и вероятности ошибок. Мы показали возможности этого паттерна на рабочем кейсе. Насчет невалидных аккаунтов повторюсь: их можно добавить, но ошибкой считать их отсутствие не стоит.
  3. Согласен, имеет место быть, но все равно при добавлении нового теста эта верхняя граница все равно будет пересчитываться, потому что новые тесты могут занимать больше времени на выполнение.
kov-pavel commented 2 years ago

И тем не менее имеем: 1) Отсутствие структуры в Ваших тестах ("каждый сам по себе"), где-то кто-то наследуется от BaseTest, а в другом месте - нет; 2) Давайте с самого фундамента. Итак, тест - это способ изучения глубинных процессов деятельности системы, посредством помещения системы в разные ситуации и отслеживание доступных наблюдению изменений в ней (с) Википедия. Ваша система в разные ситуации не попадает, если Вы ей даёте 2 валидных аккаунта (если угодно, рассматривайте это как два теста, что на самом деле так и есть), а значит, логинизация с онными за тест не считается (вернее как - тест есть, но он один, с одной порцией данных, для других это уже не тест), а значит, нет основания для применения паттерна DataProvider (нет подходящего кейса); 3) Этот проект уже никем оцениваться не будет, судя по вектору наших занятий по тестированию, а значит, он на своей финальной стадии в некотором смысле. А значит, и тесты Вы добавлять никуда не будете (относительно рамок этого задания, да и времени уже почти не осталось). А из этого следует, что имеет место накинуть таймаут на родителя, и раз основную часть тестов составляет именно логинизация (сужу по собственному опыту), то дать оценку верхней границе выполнения тестов представляется вполне себе возможным. Сие также добавит централизации общим настройкам всех Ваших тестов, на что я неоднократно обращал внимание по ходу ревью PR'ов (использование @BeforeAll, @AfterAll).

eliizaveta commented 2 years ago

@PaaavelZ @artem24630 хорошее ревью, про авторизацию в BaseTest - не делаем так, тк могут быть не требующие авторизацию тесты Соглашусь про ParametrizedTest, все таки мы тестируем логин в одноклассники, а не корректность данных пользователя

kov-pavel commented 2 years ago

@PaaavelZ @artem24630 хорошее ревью, про авторизацию в BaseTest - не делаем так, тк могут быть не требующие авторизацию тесты Соглашусь про ParametrizedTest, все таки мы тестируем логин в одноклассники, а не корректность данных пользователя

@eliizaveta Извините, а с чем Вы согласны? Я не до конца понял. Вы видите смысл в использовании паттерна DataProvider для двух валидных кейсов? Это ведь уже не тестирование, а профанация тестирования, если мы устраиваем одинаковые тесты для нашей системы

eliizaveta commented 2 years ago

@PaaavelZ @artem24630 хорошее ревью, про авторизацию в BaseTest - не делаем так, тк могут быть не требующие авторизацию тесты Соглашусь про ParametrizedTest, все таки мы тестируем логин в одноклассники, а не корректность данных пользователя

@eliizaveta Извините, а с чем Вы согласны? Я не до конца понял. Вы видите смысл в использовании паттерна DataProvider для двух валидных кейсов? Это ведь уже не тестирование, а профанация тестирования, если мы устраиваем одинаковые тесты для нашей системы

Я наоборот с вашим ревью согласна 😄