review - http://pastebin.com/kgW1ArkG

1. 'Используй не TestNG, а именно JUnit'
    >>>>>>> не то чтоб я прям уверовал в JUnit, слишком плохо его знаю чтоб что то сравнивать наверно :)
/*
    ну, уверовать - большого смысла нету)
    почитай материалы по линкам с прошлого ревью
    если есть какие-то контраргументы - можно их обсудить

    это не вопрос веры, точно)
*/
********************************************************
2. 'Мне не кажется хорошей идея держать в проекте хромдрайвер как ресурс'
    >>>>>>> мне кажется есть смысл хранить подобные штуки в проекте, т.к. он становится более мобильным.
    для запуска таких тестов нужно минимально настроенное окружение, типа хрома и мавена.
    например сейчас у меня тесты бегут параллельно на нескольких виртуалках, и мне не нужно думать о том,
    что там с окружением у них может быть не так, хром обновляется автоматически и у меня локально и на виртуалках,
    и если есть проблема в совместимости хромдрайвера с браузером - я узнаю об этом первым еще во время локального рана,
    и пушну какие то последние фичи уже с хромдрайвером, который обновится автоматически из общего репозитория у всех слейвов.
    но т.к. с новым селенидом у меня так и не получилось нормально фаерфокс запустить,
    я даунгрейднулся до 3.11 и фф 47 и хрен с ним, возможность поставить хром вообще убрал с проекта)
/*
    представь, что на тех же виртуалках - крутятся еще с десяточек проектов)
    и у каждого - внутри свой эксемпляр хромдрайвера - как ресурс

    а еще представь - что запускать нужно прект на разных операционках

    и все, хатка поломается)

    почитай побольше про воможные варианты
    https://docs.google.com/document/d/1fodHkTunrtit-EiMBrb91Mc6rbnQ5LwtBL9rISQveKA/edit?usp=sharing
*/
***************************************************************
3. 'Тут пока применять пейджи - смысла большого не было
        Эта задача эмулирует ситуацию когда мы пишем “драфт тест” - чтобы проверить как вообще автоматизация заходит на этот новый проект -
        такой старт, потому наи пока и не нужны вспомогательные методы пейджа - степы
        особенно - если учесть - что повторений практически нету'
    >>>>>>>> мне просто так нравятся селенидовские пейджи, что я решил красивенько все сделать,
     чтоб в тесте только тестовая логика была, без локаторов и т.п.
/*
    не убирай пейджа - он ОК ) упрощать - не будем

    ну, это как раз очень понятное желание) - сделать сразу красиво

    но  - при старте какого-то коммерческого проекта -
    когда нас интересуют временнЫе затраты больше чем научные эксперименты
    когда мы не уверены в выборе инструментов - разумно первый драфт тест реализовывать максимально просто
    не тратясь на развитие красивой структуры проекта и не усложняя ничего раньше времени
    чтоб наша оценка - подходит инструмент или нет - была максимально просто и быстро выполнена

    а дальше - если все ок - конечно есть смысл все красиво реализовывать
*/
******************************************************************
4.  'Это можно проще делать
    у ElementsCollection - есть метод findBy
    который в качестве параметра получает Condition'
    >>>>>>>> спасибо за подсказку, но я так по пункту с KISS немного переделал страницу, поэтому теперь не получится :)
/*
    не поняла про  - по пункту с KISS
    я писала
    Ты много сущностей вынес в поля пейджа
            Этим тоже не надо злоупотреблять,т к это не KISS
            https://docs.google.com/document/d/13dNyFGbI7mV22UUhH8E0LJ7SzabAmX7Bw7VCHScYfiU/edit#heading=h.w4krdzi2dr1j

            правильнее создавать сущности - тогда, когда они нужны
            и если какой-то локатор используется только в одном методе - то как правило - там ему и место

    что нам нужно в различных методах
    именно - коллекция тасок
    это = одна сущность
    вот ее и стоило вынести в переменную ElementsCollection tasks = $$("#todo-list>li")
    и далее - мы только с этой коллекцией и будем работать

    про - поэтому теперь не получится :)
    получилось бы использовать tasks.findBy(exactText(text)) - если бы ты оперировал коллекцией
    а не контейнером для коллекции

    да и такой вариант - tasksContainer.$$("li").findBy(exactText(text)) - тоже был бы ок
    но - он хуже
    т к нам нигде - не нужен этот контейнер без тасок - всегда нужна именно коллекция тасок
*/
    private SelenideElement getTask(String text) {
        for (SelenideElement taskContainer : tasksContainer.$$("li")) {
            if (taskContainer != null && taskContainer.$("label").getText().equalsIgnoreCase(text))
                return taskContainer;
        }
        throw new IllegalArgumentException();
    }

    //и

    private SelenideElement getTask(String text) {
        return tasks.findBy(exactText(text));
    }
    /*
        где tasks = $$("#todo-list>li")
    */
/*
    Думаю, как раз второй вариант - гораздо проще

    taskContainer - мы к нему если и будем обращаться - то только
    к его внутренним элементам - таскам
    по-другому - не будем

    так зачем нам так реализованный taskContainer
    если гораздо проще и полезнее - оперировать коллекцией tasks = $$("#todo-list>li")
    ?

    Еще важный недостаток твоей реализации - помимо его сложности
    мы - уже ищем элемент, уже перебираем коллекцию
    уже анализируем тексты элементов
    (тут конечно на такие грабли сложно наступить - но
    в более сложных вариантах - можно будет запросто -
    taskContainer.$("label").getText() - не ждет загрузки элемента, на момент выполнения этого кода -
    текст элемента может еще недогрузиться)

    а вот такой вариант - tasks.findBy(exactText(text))
    еще ничего не ищет вообще
    мы лишь зафиксировали способ - как будем искать элемент
    а искать мы его будем - когда будем с ним работать
        то ли действия выполнять
        то ли проверять
    перед действиями - будет выполнено ожидание видимости элемента (в рамках таймаута)
    проверка - тоже ждущая
    в рамках любой ждущей проверки что происходит
        в цикле, пока не истечет таймаут или пока не будет выполнено условие проверки
         выполняется поиск элемента(или элементов)
         потом - проверка
         если все ок - уже прекращаем процесс
         если не ок - ждем небольшой интервал(сколько-то миллисекунд)
         и снова пробуем выполнить это же
         и так пока не истечет таймаут или не будет выполнено условие
    получается - что используя ждущие проверки - мы достаточно надежно проверим - даже если страница догружается постепенно
    и не только проверим - но и начнем работать с элементом, не раньше чем он станет видимым
    (ожидание видимости уже встроено в любое из действий над элементом - ведь нету смысла с элементом работать - пока он невидим)

    это если говорить о том - что under the hood

    а если смотреть с внешней стороны - то одна строка кода - всегда проще нескольких
    особенно - которые еще и с циклом, if-ом, и исключениями )

*/
*******************************************************************************************************
5. 'не знаю, стоило ли объединять создание пейджа и открытие урла'
    >>>>>>>> взял из примеров с селенид сайта
    https://github.com/selenide-examples/google/blob/master/test/org/selenide/examples/google/selenide_page_object/GoogleTest.java
    но переделал :)
/*
    не знаю, видел ли ты вот эту статью
    http://automation-remarks.com/2016/selenide-shadow-sides/index.html
    особенно ценны - комментарии Солнцева (автора Selenide) к ней
*/
************************************************************
6. 'создание пейджа - я бы делала вне тест-метода'
    >>>>>>>> а где? в бефоре нельзя, статики я не хотел делать.
/*
    чего нельзя? вполне можно)
    и вариантов есть поболее чем один бифор)
    еще и instance initialisation block вполне ок будет
    просто - нужно на уровне класса объявить переменную для пейджа
    а уже в бифор-методе или instance initialisation block - уже ее инициализировать

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

    Selenide - уже использует статический драйвер…
    и наша пейджа селенидовская - уже по факту “статическая”
    в контесте запуску тестов в браузере
    потому создавать ее не статической - в общем-то - обманывать себя )

    или вообще в момент объявления поля - пейджа - как ты и сделал
    самый лаконичный вариант

    JUnit для запуска каждого теста заново создает объект тест-класса
    в этот момент - будут созданы и все поля класса (не статические)
    так что все ок

    http://www.javamadesoeasy.com/2015/06/differences-between-instance.html
*/
**********************************************************
7. 'где в проекте располагается этот класс  (ветка проекта src\main - т к вещь это = универсальная)'
    >>>>>>>> я честно говоря вообще не понимаю к чему эти разделения на main и tests с ограничениями видимости, они мешают.
    почему не делать все в main, а уже в нем сделать разделение на tests, pages, elements, utils и так далее
/*
    все же порядок и какие-то договоренности - всегда лучше
    чем решения, принятые в одно лицо, исходя из индивидуальных предпочтений и чувства прекрасного )

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

    так или иначе - мы - командные игроки)
    добавь к этому - что никто в команде - не навсегда
    и хорошо бы после себя оставить такой код, в который будет сравнительно просто въехать

    предсказуемая структура, выполненные конвеншенсы - очень способствуют более быстрому разбору чужого кода
    такая отправная точка

    но это только верхушечка айсберга)
    попробуй  - запусти тесты, которые лежат  в ветке src\main
    или тест-методы из тест-классов, имя которых заканчивается не на Test
    с командной строки попробуй)
    получилось? ))

    а ограничения видимости - они на самом деле - не мешают
    а не допускают ошибок

    пример
    в src\main - расположили пейджи - как вещь универсальную и переиспользуемую
    также в src\main - расположили некие универсальные методы в классе-хелпере

    если пытаться из этих универсальных классов - обратиться к содержимому классов из src \ test - ничего не получится
    и это хорошо и правильно
    т к - инструменты служат тест-классам
    а не тест-классы- инструментам

    аналогично - если речь идет о классах с тестовыми данными (такие должны находиться в src \ test)
    это очень хорошо - что к таким классам невозможно будет из пейджа обратиться
    т к в таком случае - пейдж не будет завязан на какие-то конкретные тестовые данные
    все тестовые данные будут получены методми пейджа - как значения параметров
    и тест-метод  - будет проще читаться, не будет магии
    и пейджи - будут максимально универсально реализованы - их можно будет более разнообразно использовать при развитии проекта
    думаю, этот момент со временем тоже станет понятнее

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

        в принципе - src \ main - можно вообще не использовать
        туда кладут core фреймворка, который будет переиспользоваться или сам продукт
        (бывает что продукт и его тесты - живут в одном проекте)
        и по большому счету - такое распределение - именно для этого и нужно

        также - пейджи - туда тоже кладут (если продукт не живет вместе с тестами в одном проекте - так получится неаккуратно)
        если в проекте только тесты (дальше - буду приводить пример структуры проекта, из расчета что продукт и тесты живут в разных проектах)
        и это на самом деле - защита от дурака
        т к нельзя к тестовым данным подрубиться из пейджа
        да и к самим тест-классам
        (из кода src \ main - не видно ничего из src\test)
        а из моего опыта - таких умников достаточно - которые так пытаются делать

*/
******************************
public void endToEnd() {
/*
    почитай вот эти советы
    https://docs.google.com/document/d/13dNyFGbI7mV22UUhH8E0LJ7SzabAmX7Bw7VCHScYfiU/edit#bookmark=id.2gjiy0o1o48q

    и подправь имя тест-метода

    endToEnd - совсем не информативно
    варианты tasksLifeCycle, tasksCommonFlow
    дадут нам понять - что это е2е тест, причем тестирует он - именно работу с тасками
    т е - да, мы по-прежнему не уточнились до конкретных покрытых действий (т к получится днинно)
    но - все равно - мы точнее - чем в варианте  endToEnd
*/
**************************
 page.addTask....
                .tasksShouldHaveSize(4);
/*
    после добавления тасок - проверки размера списка - недостаточно
    т к - могли таски добавиться - и не с теми текстами, и не в той последовательности
    для коллекции элементов - есть кондишен exactTexts

    нам достаточно проверки shouldHave(exactTexts("...", "...", ...))
                как осуществяется проверка по кондишену exactTexts
                сверяется количество, порядок и тексты
                    количество элементов коллекции должно быть равно количеству переданных текстов
                    иначе - проверка не прошла
                    и далее - по порядку сверяются текст элемента и переданный текст
                    нулевой - с нулевым
                    первый с первым
                    и  т д
                таким образом - уже не нужно проверять размер списка
                раз уже проверили  exactTexts
    касается всех реализованных проверок
*/
***************************
public class BaseTest {

    @BeforeClass
    public static void beforeClass() {
        Configuration.browser = "firefox";
        Configuration.reportsFolder = "target" + separator + "screens";
        Helper.setDriver(getWebDriver());
    }

}
/*
    по умолчанию - и так будет использован firefox
    переназначать Configuration.reportsFolder - не могу предположить даже, какая от этого может быть польза)

    в самом Helper - можно просто получить getWebDriver()
    и не создавать ни дополнительного поля - для хранения драйвера
    в рамках селенидовского проекта (любого) - getWebDriver() - вернет тебе веблрайвер для текущего потока
    так что все ок будет

    да и таймаутом можно пользоваться селенидовским)
    Confifuration.timeout (учти что он задан в миллисекундах)
    но то уже так, на твое усмотрение

*/
******************************************
new WebDriverWait(driver, 4).until(Conditions.waitUntilAjaxCompleted);
//или
new WebDriverWait(driver, 4).until(waitUntilAjaxCompleted);
//или
new WebDriverWait(driver, 4).until(ajaxCompleted);
/*
    используй import static и переименуй кондишен
    ведь сам кондишен - ничего не ждет
    это значение параметра для нашего ожидания

    также советую переименовать класс  Conditions на CustomConditions
    и дать имя классу Helper - более точное (если он будет служить исключительно контейнером для ожидания загрузки Ajax

    а если Helper - будет контейнером для разнообразных универсальных методов
    то стоит назвать его Helpers
    и оставить там - лишь вспомогательные методы
    убрать какую-то общую логику для класса
    это сделать просто
     достаточно убрать поля в классе и их сеттеры
     и реализовать
     public static void waitUntilAjaxComplete(long timeout)
     public static void waitUntilAjaxComplete() - тут - вызываем waitUntilAjaxComplete(long timeout)
     с использованием селенидовского таймаута
     т е - если нужно указать явно таймаут - вызовем первый
     а если можно не уточняться и значение Configuration.timeout нас устраивает - то и ок - вызовем второй

    однозначно - эти 2 класса (Condition & Helper) - должны жить в ветке src \ main

    приведу сразу - пример удачной структуры проекта
*/
/*
    http://joxi.ru/nAyqEx7HXvxQoA

    вот пример хорошей структуры проекта

    в src \ main

      core - универсальное, что можно переиспользовать в разных проектах
      pages - пейджи тоже можно переиспользовать для других тестов этого же приложения


    в src \ test

      testdata - тестовые данные (если такие есть и они вынесены в отдельный класс)
      testconfigs - предки тест-класса (так можно их изолировать от  собственно тест-классов - чтоб легче было ориентироваться


    про пекеджи еще немного)
    если GroupID = com.somesite
    а проект todomvctest
    то пакет корневой должен быть com.somesite.todomvctest

    логика  - чтобы "не смешивались имена сущностей"

    внутри одной компании - может быть несколько проектов)
    и у всех у них один com.somesite  - базовый пекедж
    но для каждого проекта должен быть свой  “базовый пекедж проекта"
    иначе все смешается)
    важно то, что когда этот проект выльется в отдельную библиотеку,
    то не будет конфликтов при его подключении
*/
*************************************
public class MainPage {
/*
    если пейдж единственный - я бы назвала его TodoMvcPage - все равно точнее будет - что за пейдж
    не настаиваю
*/
    private SelenideElement tasksContainer = $("#todo-list");
/*
    писала выше - нам от #todo-list - нужны только и только таски
    так почему сразу - не оперировать коллекцией $$("#todo-list>li") ?

*/
***************************************

    public MainPage tasksShouldHaveSize(int quantity) {
        try {
            tasksContainer.$$("li").shouldHaveSize(quantity);
            return this;
        } catch (NoSuchElementException e) {
            if (quantity == 0) return this;
            throw e;
        }
    }
/*
    try-catch - блока не нужно

    проверка shouldHaveSize если и вызовет исключение - то это значит - проверка не прошла
    и это ок - что тест упадет

    да и исключение будет никак не NoSuchElementException
    а потомка UIAssertionError (https://github.com/codeborne/selenide/blob/master/src/main/java/com/codeborne/selenide/ex/UIAssertionError.java)
    точно ловить ничего не нужно
    и тем более = как-то обрабатывать вариант когда quantity == 0
*/
************************************************************
    public MainPage completedTasksShouldHaveSize(int quantity) {
        try {
            tasksContainer.$$(".completed").shouldHaveSize(quantity);
            return this;
        } catch (NoSuchElementException e) {
            if (quantity == 0) return this;
            throw e;
        }
    }
/*
    про try-catch  и обработку quantity == 0 - все то же самое

    если мы оперируем коллекцией тасок tasks = $$("#todo-list>li")
    то тут ее - просто отфильтруй по классу completed
    есть такой метод для коллекции - filter
    который оперирует значением типа Condition
    и есть кондишен cssClass, который в качестве параметра - принимает имя класса
*/
*******************************************************************
    public MainPage addTask(String text) {
        $("#new-todo").setValue(text);
        $("#new-todo").pressEnter();
        return this;
    }
/*
    можно переписать в одну строку - $("#new-todo").setValue(text).pressEnter();
*/
***********************************************
    public MainPage toggleTask(String text) {
//сравни
    public MainPage toggle(String taskText) {
/*
    поскольку все действия - выполняем над тасками - можно
    в именах методов-действий - убрать вот это уточнение Task

    и уточнить это уже в имени параметра
    получишь в итоге - более аконичный код в тест-методе
    причем - наглядности и точности не потеряешь

    такое решение нужно распростарнить на все методы пейджа
    которые служат для выполнения действий

    для методов-проверок - не стоит сокращать имена
    тут лучше явно все описать - что мы проверяем
    а то будет не очевидно
*/
******************************
    private SelenideElement getTask(String text) {
        for (SelenideElement taskContainer : tasksContainer.$$("li")) {
            if (taskContainer != null && taskContainer.$("label").getText().equalsIgnoreCase(text))
                return taskContainer;
        }
        throw new IllegalArgumentException();
    }
/*
    писала выше про недостатки такого метода

    эта версия - неоправданно сложная и не стабильная
*/