search

artem24630 / autotesting-2022

0 stars 0 forks source link

Ревью #2

Open a-yasevich opened 2 years ago

a-yasevich commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L48 Тут дублирование кода, эту строчку можно заменить на метод load()

a-yasevich commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/RingTest.java#L9 Тут нейминг неправильный - не camelCase

a-yasevich commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/MessagePageTest.java#L51 Это выглядит не очень, и по идее может быть заменено на should(condition, duration)

kov-pavel commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/RingTest.java#L9

Тут нейминг неправильный - не camelCase

Согласен, показалось, что с camelCase будет нечитаемо

artem24630 commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/MessagePageTest.java#L51

Это выглядит не очень, и по идее может быть заменено на should(condition, duration)

Я спрашивал у Эмилии, и она сказала, что так тоже можно. Еще есть вариант Selenide.sleep()

artem24630 commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L48

Тут дублирование кода, эту строчку можно заменить на метод load()

хорошо

nikita17th commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatPageElement.java#L1

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

nikita17th commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L44

Хорошо бы проверять на существвание через should, выбрал просто эту строчку но в других местах тоже видел)

nikita17th commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/AbstractTest.java#L16

Тут явно задаете путь через строку лучше использовать Path, ну и + расширение .exe только для Windows получается. На маке тесты не запустились, про линукс не знаю)

nikita17th commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L40

На сколько знаю использование таких asser неплохо для дебага, но использовать в конечном коде не очень хорошая практика.

nikita17th commented 2 years ago

Ну вообще странный подход) Если можно обойтись без явного sleep. Тут на мой взгляд можно либо при конструировании проверять либо сделать что-то подобное у MessagePage


 public static Duration TIMEOUT = Duration.ofSeconds(2);
 public void check() {
   $(selector).should(Condition.exist, TIMEOUT);
 }
artem24630 commented 2 years ago

Ну вообще странный подход) Если можно обойтись без явного sleep. Тут на мой взгляд можно либо при конструировании проверять либо сделать что-то подобное у MessagePage

 public static Duration TIMEOUT = Duration.ofSeconds(2);
 public void check() {
   $(selector).should(Condition.exist, TIMEOUT);
 }

переделал

kov-pavel commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatPageElement.java#L1

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

Убрали

kov-pavel commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L44

Хорошо бы проверять на существвание через should, выбрал просто эту строчку но в других местах тоже видел)

Так чат может существовать, а может и нет. Мы не дискриминируем пользователей, которые ни с кем не общаются.

kov-pavel commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/AbstractTest.java#L16

Тут явно задаете путь через строку лучше использовать Path, ну и + расширение .exe только для Windows получается. На маке тесты не запустились, про линукс не знаю)

1) Почему лучше использовать Path? 2) Сделали программу адаптивнее к операционке - проверьте

kov-pavel commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L48

Тут дублирование кода, эту строчку можно заменить на метод load()

Заменили

kov-pavel commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/RingTest.java#L9

Тут нейминг неправильный - не camelCase

Исправили

nikita17th commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/AbstractTest.java#L16

Тут явно задаете путь через строку лучше использовать Path, ну и + расширение .exe только для Windows получается. На маке тесты не запустились, про линукс не знаю)

  1. Почему лучше использовать Path?
  2. Сделали программу адаптивнее к операционке - проверьте

  1. В разных опрационнах системах могут использоваться разные разделители, https://docs.oracle.com/javase/tutorial/essential/io/path.html

  2. Не работает, но выглядит явно лучше чем было

nikita17th commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L44

Хорошо бы проверять на существвание через should, выбрал просто эту строчку но в других местах тоже видел)

Так чат может существовать, а может и нет. Мы не дискриминируем пользователей, которые ни с кем не общаются.

Что будет если элемента не существует и вы вызовете метод size()?

artem24630 commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/tests/AbstractTest.java#L16

Тут явно задаете путь через строку лучше использовать Path, ну и + расширение .exe только для Windows получается. На маке тесты не запустились, про линукс не знаю)

  1. Почему лучше использовать Path?
  2. Сделали программу адаптивнее к операционке - проверьте
  1. В разных опрационнах системах могут использоваться разные разделители, https://docs.oracle.com/javase/tutorial/essential/io/path.html
  2. Не работает, но выглядит явно лучше чем было

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

artem24630 commented 2 years ago

https://github.com/artem24630/autotesting-2022/blob/7a5214695bc12133e398a6a736259dcf1703eb8f/src/test/java/pages/elements/ChatsPageElement.java#L44

Хорошо бы проверять на существвание через should, выбрал просто эту строчку но в других местах тоже видел)

Так чат может существовать, а может и нет. Мы не дискриминируем пользователей, которые ни с кем не общаются.

Что будет если элемента не существует и вы вызовете метод size()?

если вы про то, что можно словить NPE, то это легко обходится проверкой на null, но если хотите, я поставил аннотацию Nullable для предупреждения

eliizaveta commented 2 years ago

@a-yasevich @nikita17th хорошие замечания по коду, но недостаточные, не хватило оценки работы с паттернами и junit Дополнила #3