ревью на
sources (review 9 + new task 10 + new task 11) - https://github.com/AleksanderPopov/automician_course/tree/master/src/
    изменения после 9 реью, сделал pageObject pageModule, добавил тесты для google

http://joxi.ru/Dr860ybh4oqdQ2
/*
    исходя из того - что такие классы могут беть переиспользованы не только для todoMVC - разумно вынести это по структуре выше
    в пекедж core (или api)

    GivenHelpers - в пекедже given или helpers
    что-то более близкое по смыслу
    точно не api это
*/
***********************************
public class AllureHelper {

    @Attachment(value = "Page-screenshot", type = "image/png")
    public static byte[] publishScreenshot() {
        String path = getPathOf(screenshot());
        ...
    }

    @Attachment(value = "Page-source", type = "text/html")
    public static byte[] publishHtml() {
        String path = getPathOf(screenshot());
        ...
/*
    тут можно чуть оптимальнее

    если нам нужно приаттачить и скриншот, и html
    то получится - что мы дважды вызываем screenshot()

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

    было бы круто - если бы мы могли как-то вот так вызывать AllureHelper

    allureAttachment().withScreenshot().withHtml().build().publish() (тут без билдер-паттерна тоже не обойдешься ))

    ну или вариант попроще - метод с параметрами - что формировать
    мне меньше нравится - т к при вызове будет замудренее

    и дальше это можно использовать
    @Rule
    public Publisher publisher = Publisher.always().withAttachments(allureAttachment().withScreenshot().withHtml().build()).createPublisher();

    т е - вообще отвязать логику формирования аттачментов
    от логики работы правила
    правило - лишь диктует - когда что делать

    а что делать - будет определяться переданным объектом

    (этот параметр - параметр метода withAttachments - лучше сделать более абстрактного типа - типа
        interface AttachmentHelper
        с объявленными методами
            publish();
            clean(); (это если захочешь - я по-прежнему считаю - что это лишнее)
    )
    а AllureHelper - пусть имплементирует AttachmentHelper

    будет выполняться Single Responsibility Principle)
    получится крутое решение)
    оно и сейчас уже огого )
*/
***************************************
 private void addTasksToLocalStorage() {
        StringBuilder tasksJson = new StringBuilder("");
        tasksJson.append(doJsonfromList(true, activeTasks))
                .append(tasksJson.toString().isEmpty() ? "" : ",")
                .append(doJsonfromList(false, completedTasks));
/*
    вот - видишь
    получаем - всегда будут идти вначале - активные таски
    потом - закомпличеные
    как бы мы их не добавляли сами - картинка будет только такая

    а надо - как добавляли
    чтоб в такой последовательности они и шли

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

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

    заодно и код метода
    private Builder tasks(TaskType taskType, String... taskNames) {
    улучшится )

    я не сторонник применения switch (taskType)
    чаще всего без него можно обойтись, причем более простой логикой
    даже при такой как сейчас реализации - метод tasks просто лишний

    сравни
        Collections.addAll(activeTasks, taskNames);
        return this;
    и
        return tasks(TaskType.ACTIVE, taskNames);

    С учетом того - что вообще уйдет tasks - первый вариант мне кажется лучшим решением)
*/
***************************************
public enum Filter {
/*
    его пабликом делать ни к чему
    все внутри гивен хелпера ведь происходит

    а снаружи - просто вызываем соотвествующий метод
    не надо нам видеть сложное снаружи)

    пусть снаружи - будет просто
*/
********************************
    public enum Filter {
        ALL(""),
        ACTIVE("active"),
        COMPLETED("completed");

        private String subUrl;

        Filter(String subUrl) {
            this.subUrl = subUrl;
        }

        @Override
        public String toString() { return "https://todomvc4tasj.herokuapp.com/#/" + this.subUrl; }
    }
/*
    чуть более DRY вариант предлагаю

    не настаиваю - т к твой вариант более KISS )
    а это тоже важно)

    лично для себя я держу такую грань
    то что юзаю в тест-методах - должно быть более KISS
    а вот сама реализация пейджей/виджетов/хелперов/кондишенов - тут уже просто правила разработки ПО применяю
    DRY - тут уже важно )
    т е - снаружи все эти  пейджи/виджеты/хелперы/кондишены = то что видно при использовании этого всего
    должно конечно обеспечивать KISS
    а вот внутри - там где мы алгоритмы реализуем, какую-то сравнительно серьезную логику - там уже значимость DRY - гораздо выше

    потому бы - для enum Filter
    выбрала более DRY реализацию
*/
****************************
    private void openFilterPage() {
        switch (this.filter) {
            case ALL: {
                open(ALL.url);
                break;
            }
            case ACTIVE: {
                open(ACTIVE.url);
                break;
            }
            case COMPLETED: {
                open(COMPLETED.url);
                break;
            }
            default:
                throw new UnsupportedOperationException("Wrong FilterType parameter");
        }
    }
//сравни
    private void openFilterPage() {
        open(this.filter.toString())
    }
/*
    тут тоже без switch - вполне ок будет
*/
***************************************
 return tasks.findBy(cssClass("editing"))
                .find(" .edit")
/*
    тут - пробел перед  .edit не дает ровно ничего

    метод find для SelenideElement - ищет строго внутренние элементы
    согласно цсс селектору = маске

    так что от этого пробела - ни холодно, ни жарко

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

    не по теме этого задания
    просто на тему  find для SelenideElement и findElement для WebElement селениумского
    (тут логика один-в-один)
    http://pastebin.com/TivB2FzA
*/
************************
public class SmokeTodoMVCTest extends BaseTest {
/*
    уже в вопросах-ответах - обсудили
    не делаем тест-классы для такого-то покрытия

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

    а покрытие - категориями обозначаем и так обеспечиваем запуск нужных нам тестов
*/
********************************
    @Test
    public void basicTasksFlow() {
    /*
        перемудрил малость ты с тестом)
    */
        given().completed("compl1")
                .atAllFilter()
                .prepare();

        add("2");
        /*
            в этот момент = у тебя 2 таски в списке
            значит - операция edit("2"
            не проверит результат предыдущей операции add("2");
            т к проверит состояние только одной из 2-ух тасок

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

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

            и еще - не сокращай слова
            если сокращение не общепринятое - не используй его
        */
        edit("2", "edited2");
        shouldHave("compl1", "edited2");

        goActive();
        /*
            следующее действие - лишь проверит что "edited2" - есть
            а состояние всего списка - не проверено

            я б только тут добавляла вторую таску
            после проверки состояния списка
        */
        cancelEdit("edited2", "notedited2");
        toggle("edited2");
        /*
            проверка?
            идем по кругу)
        */

        goCompleted();
        /*
            проверка нужна
        */
        destroy("edited2");
        shouldHave("compl1");
        /*
            как-то без имени пейджа - shouldHave("compl1") - по-сиротски выглядит)
            не понятно - кто это кому должен)
            читай чуть ниже
        */
        toggle("compl1");
        /*
            нужна проверка
            эххх)
        */

        goAll();
        shouldHave("compl1");
    }
 ***************************************
public class TaskPage {
/*
    если юзаешь пейдж-модуль
    то в имени пейджа - уже не надо Page указывать

        Пейдж-модули удобно называть без слова Page в конце
            потому что если ты будешь использовать несколько пейдж-модулей в одном тесте

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

            и когда ты будешь писать
                TodoMVCPage.givenAtAll()
            то это явно не так прикольно как
                TodoMVC.givenAtAll()

            правда?

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


            вот это Page в конце - это дань общим conventions (или общепринятым предпочтениям)
            при именовании классов в мире ООП

            ми же тут юзаем подход "Модульного Программирования", потому тут этого "лишнего слова"
            не нужно

            А то, что это все же пейдж - скажет нам имя пекеджа - pages

    кстати - Tasks.shouldHave("compl1") - уже ниче так) попонятнее)
    а если не хочешь явно указывать для методов пейджа - имя его класса Tasks
    то тогда лучше для проверок используй имена assert....
    так оно логичнее будет

    учти - еесли будешь указывать имя пейджа-модуля
    то это надо делать всегда
    а не через раз
    иначе путать будет
*/
***************************
    @Step
    public static ItemsLeft itemsLeft() {
        return new ItemsLeft();
    }

    public static class ItemsLeft {
        private static final SelenideElement element = $("#todo-count>strong");

        public void shouldBe(int itemsCount) {
            element.shouldHave(exactText(String.valueOf(itemsCount)));
        }
    }
/*
    для подхода с пейджами-модулями
    я бы продолжила и тут тему

    Tasks.ItemsLeft.shouldBe...

    т е - от метода itemsLeft() - избавилась бы)

    раз пейджи-модули - знач пейджи-модули)
    никаких объектов)
*/
**********************************
    @Test
    public void delete() {
        given().active("1", "2")
                .atActiveFilter()
                .prepare();

        destroy("1");
        shouldHave("2");
        itemsLeft().shouldBe(1);
    }
/*
    тут delete() , там destroy() - не
    так не надо
    одно понятие = один термин
    нужна однозначность

    кстати, вызовы Tasks.... решили бы проблему
    см ниже вариант
*/
    @Test
    public void delete() {
        given().active("1", "2")
                .atActiveFilter()
                .prepare();

        Tasks.delete("1");
        Tasks.shouldHave("2");
        Tasks.ItemsLeft.shouldBe(1);
    }
*****************************
/*
    то же касается и других тест-методов

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

    это надо поправить

    однозначный код - всегда лучше
*/
****************************
public class TasksOperationsAtCompleteFilterTest extends BaseTest {

    @Test
    public void editConfirmEnter() {
        given().completed("1", "2")
                .atCompletedFilter()
                .prepare();

        edit("1", "2");
        shouldHave("2", "2");
    }

public class TasksOperationsAtActiveFilterTest extends BaseTest {

    @Test
    public void editConfirmedByEnter() {
        given().active("1", "2")
                .atActiveFilter()
                .prepare();

        editEnter("2", "edited2");
        shouldHave("1", "edited2");
    }
/*
    сравни имена вспомогательных методов
    имена тест-методов

    разброд и шатания)

    итемс лефт - проверяй в фиче-тестах всегда
    если это возможно
*/
***********************************

    @Test
    public void addTask() {
/*
    еще пример разброда - то применяешь Task в имени тест-метода
    то нет

    определись
    и делай одинаково
*/
***************************************
    public class TasksOperationsAtAllFilterTest extends BaseTest {

        @Test
        public void editConfirmByClickOutside() {
            given().active("1", "2")
                    .atAllFilter()
                    .prepare();

            editTab("1", "2");
            shouldHave("2", "2");
            itemsLeft().shouldBe(2);
        }
/*
    в имени тест-метода - написано одно
    а делаешь - другое

    к следующему разу
    с именами утряси все
    просмотри - чтоб в методах делалось то что заявлено в имени тест-метода
    чтоб проверок хватало
    чтоб тестовые ситуации ииспользовал разнообразные
    чтоб с покрытием было все ок

    используй тест-план

    самая скучная часть работы)
    но - это важные детали)
    так что придираться буду
*/