Test front end

[dyuvzhenko]

Обсуждение

[dyuvzhenko]

@dhilt У меня пока что нет ни малейшего понимания как тестировать front end. Но я так понимаю главной целью будет протрясти все существующие actions и то, как должен выглядеть reducers после actions? Как тестировать внешний вид или какое-то пользовательское взаимодействие с UI - совсем не представляю.

[dyuvzhenko]

@dhilt И собственно где в файловой структуре должны будут писаться эти тесты. Возможно в самой верхней папке ./test по соседству c back end? Допустим, создадим папку ./test/frontend, далее создадим _shared.js, в который будем импортировать файл ./app/index.js. А дальше, хм, возможно надо с помощью webpack'а собирать все приложение и как-то внутри браузера запускать тест...

[dyuvzhenko]

@dhilt Больше всего я пока что уверен только в том, что весь тестирующий код должен как-то запуститься именно в браузере. Как если бы написали весь этот код и просто за-inject'или в html-страничку.

[dhilt]

@BitDen На сервере мы используем mocha+chai, на клиенте я бы посмотрел в сторону jest+chai. Надо брать готовую инфраструктуру, посмотри https://blog.logrocket.com/testing-react-applications-part-1-of-3-ebd8397917f3 например...

[dyuvzhenko]

@dhilt Пока что сделал только первый бессмысленный тест - https://github.com/dhilt/dharmadict/commit/bdfe2b751473a68a68cbf62f1d2df6dc07e5b457. Папка test/frontend должна помимо папки components еще и actions содержать, но пока что без подключенного redux просто было нечего там написать как пример.

Jest'а там пока нет, но двигался я по документации от этой библиотеки - http://facebook.github.io/jest/docs/en/tutorial-react.html#content.

[dyuvzhenko]

@dhilt Нужно как-нибудь в пределах frontend-тестов создавать store, чтобы можно было connect проводить к компонентам в тестах.

[dyuvzhenko]

@dhilt Кажется нашел подход к этому вопросу - http://redux.js.org/docs/recipes/WritingTests.html. Сегодня попробую закоммитить нормальные тесты для actions и components.

[dyuvzhenko]

@dhilt Сделал первый вроде бы вменяемый образец тестирования actions и отражения данных в redux - https://github.com/dhilt/dharmadict/commit/803f89f65694ea0044dd6417ab5244970e77ab60. Только не могу никак заставить работать nock.js, не получается корректно сымитировать http-запрос (https://github.com/dhilt/dharmadict/commit/803f89f65694ea0044dd6417ab5244970e77ab60#diff-a811e103fdc6b10d9ada1f7b20d515e8R99), застрял пока что на этом...

[dhilt]

@BitDen Я надеюсь сегодня-завтра добраться до кода, рад, что у тебя получается! главное, следовать устоявшимся практикам

[dyuvzhenko]

@dhilt Тесты вроде бы доделал-починил - https://github.com/dhilt/dharmadict/commit/05a2b27392da7e2d5d73d47c77f71264c552b21f.

[dyuvzhenko]

@dhilt Хотя с этим имитированием запроса запутался, может это и неверный сам по себе тест. В общем, тут нужно экспертное мнение :)

[dyuvzhenko]

@dhilt И еще добавил тест на случай неудачного добавления термина - https://github.com/dhilt/dharmadict/commit/de41af202978925f81cdcf7c94cebfb696742096. Плюс немного упростил код.

[dyuvzhenko]

@dhilt Покрыл тестами actions/common.js - https://github.com/dhilt/dharmadict/commit/f59fe1f5ed85e86de10d39bbe046d90af53c2266. Плюс добавил защиту на сервер, чтоб ответ гарантированно приходил, даже если это ошибка (до этого запрос GET_COMMON_DATA_ASYNC зависал на неограниченное время не выдавая ни ошибки, ни результата).

[dhilt]

@BitDen Это все звучит очень обнадеживающе! я еще не смотрел эту часть, надеюсь, скоро добраться

[dyuvzhenko]

@dhilt Ну, лучше не торопитесь, еще есть некоторые нюансы которые надо доделать. Сейчас вот понял в чем была проблема о которой говорил. Жаловался на nock и якобы неправильное имитирование запроса на сервер, на самом деле я просто по глупости подсовывал пустой state в коде и функции, которые по идее берут данные внутри себя с помощью getState, оставались ни с чем. Сегодня постараюсь все что есть довести до нормального вида.

[dyuvzhenko]

@dhilt На NewTerm.spec теперь точно можно посмотреть, мне кажется теперь там все так, как и должно быть - https://github.com/dhilt/dharmadict/commit/6deded3c02a10343f5c6131fb303589b484162dc.

[dyuvzhenko]

@dhilt Хотя вру, нужно еще кое-что сделать :)

[dyuvzhenko]

@dhilt Хотя нет, не врал, наверное все же сделал все что мог. Просто заметил что возможно не было смысла добавлять кучу store.dispatch'ей, ибо они судя по всему никак не меняют store. Но с другой стороны без них nock-запрос не хочет работать...

[dhilt]

@BitDen Я начал смотреть тесты, работа проделана большая... Для того, чтобы запускаться под Win, я перевел работу с окружением на cross-env, это потребует npm install перед следующим запуском. Также подготовил global.window.localStorage для работы в тестовом окружении, пока через выставление объекта в _shared, который импортируется теперь всегда первым. У меня вопрос: как выставить последовательность выполнения спек (наподобие mocha.opts)?

[dyuvzhenko]

@dhilt Вроде как всю документацию просмотрел, погуглил, но ничего не нашел на этот счет. Нашел только ключ --runInBand, позволяющий запускать тесты последовательно.

[dyuvzhenko]

@dhilt Работа может и большая, но очень возможно по части имитации асинхронного запроса на сервера с помощью nock - совершенно бесполезная.

[dhilt]

@BitDen Мне очень понравилась концепция тестирования actions, поскольку там сосредоточена бизнес-логика, и с этой позиции имитирование fetch'ей делает это полноценным: мы полностью проходим каждый отдельный action creator как независимую часть бизнес-логики приложения.

[dyuvzhenko]

@dhilt С самой концепцией тестирования все в порядке, я говорю о реализации. Есть пока что много сомнений насчет nock, правильно ли написан код, связанный с ним. Нужно наверное поподробнее эту библиотеку поизучать.

[dyuvzhenko]

@dhilt Насчет nock'а ошибался, проблемы были с созданием store.

    // test async actions
    let store = mockStore(initialState);
    store.dispatch(actions.setUserId(userId));
    store.dispatch(actions.changeAdminUserPassword({password: password}));
    store.dispatch(actions.changeAdminUserPassword({confirmPassword: confirmPassword}));

    nock('http://localhost/')
      .patch('/api/users/' + userId, {
        payload: {
          password: store.getState().admin.editUserPassword.password,
          confirmPassword: store.getState().admin.editUserPassword.confirmPassword
        }
      })
      .reply(500, expectedErrorResponse);

    return store.dispatch(actions.updateAdminUserPasswordAsync()).then(() => {
      expect(store.getActions()).toEqual(expectedErrorActions);
    });

После подобного кода ожидалось что в store отразятся новые данные после трех dispatch'ей, но там увы ничего не было. Но store.getActions() показывал все прошедшие actions. Если же в store пропихнуть уже обработанный initialState, то все получается вполне здорово:

let expectedStateBeforeRequest = JSON.parse(JSON.stringify(initialState));
    expectedStateBeforeRequest.admin.editUserPassword.id = userId;
    expectedStateBeforeRequest.admin.editUserPassword.password = password;
    expectedStateBeforeRequest.admin.editUserPassword.confirmPassword = confirmPassword;
    let store = mockStore(expectedStateBeforeRequest);

    nock('http://localhost')
      .patch('/api/users/' + userId, {
        payload: {
          password: store.getState().admin.editUserPassword.password,
          confirmPassword: store.getState().admin.editUserPassword.confirmPassword
        }
      })
      .reply(200, expectedSuccessResponse);

    return store.dispatch(actions.updateAdminUserPasswordAsync()).then(() => {
      const successAction = store.getActions().find(elem => elem.type === types.UPDATE_ADMIN_USER_PASSWORD_END);
      let successNotification = store.getActions().find(elem => elem.type === types.CREATE_NOTIFICATION);
      delete successNotification.notification.timer;
      expect(successAction).toEqual(expectedSuccessActions[1]);
      expect(successNotification).toEqual(expectedSuccessActions[2]);
    });

[dyuvzhenko]

@dhilt Это всё в общем означает что по части асинхронных actions все тесты какие есть написаны неверно и бессмысленно. Буду думать как бы иначе store инициализировать, чтобы после dispatch'ей все данные отражались в state.

[dyuvzhenko]

@dhilt Судя по всему библиотека redux-mock-store и не подразумевает изменений в state: https://github.com/arnaudbenard/redux-mock-store/issues/71

[dyuvzhenko]

@dhilt Значит у меня остается только один вопрос насчет тестов. Следующий код будет считаться подходящим?

  it('should work correctly: function updateAdminUserPasswordAsync', () => {
    const userId = translators[0].id;
    const password = 'new_password';
    const confirmPassword = 'new_password';
    const expectedSuccessResponse = {
      success: true,
      user: translators[0]
    };
    const expectedSuccessActions = [
      { type: types.UPDATE_ADMIN_USER_PASSWORD_START },
      {
        type: types.UPDATE_ADMIN_USER_PASSWORD_END,
        error: null,
        result: true
      },
      {
        type: types.CREATE_NOTIFICATION,
        idLast: 1,
        notification: {
          id: 1,
          text: 'EditUserPassword.new_password_success',
          ttl: 3000,
          type: 'success',
          values: {}
        }
      }
    ];

    // test reducers
    let expectedState = JSON.parse(JSON.stringify(initialState));
    let changedInitialState = JSON.parse(JSON.stringify(initialState));
    changedInitialState.admin.editUserPassword.password = password;
    changedInitialState.admin.editUserPassword.confirmPassword = confirmPassword;
    expectedState.admin.editUserPassword.error = null;
    expectedState.admin.editUserPassword.pending = false;
    expectedState.admin.editUserPassword.password = '';
    expectedState.admin.editUserPassword.confirmPassword = '';
    expect(reducer(changedInitialState, expectedSuccessActions[1])).toEqual(expectedState);

    // test async actions
    let expectedStateBeforeRequest = JSON.parse(JSON.stringify(initialState));
    expectedStateBeforeRequest.admin.editUserPassword.id = userId;
    expectedStateBeforeRequest.admin.editUserPassword.password = password;
    expectedStateBeforeRequest.admin.editUserPassword.confirmPassword = confirmPassword;
    let store = mockStore(expectedStateBeforeRequest);

    nock('http://localhost')
      .patch('/api/users/' + userId, {
        payload: {
          password: store.getState().admin.editUserPassword.password,
          confirmPassword: store.getState().admin.editUserPassword.confirmPassword
        }
      })
      .reply(200, expectedSuccessResponse);

    return store.dispatch(actions.updateAdminUserPasswordAsync()).then(() => {
      const successAction = store.getActions().find(elem => elem.type === types.UPDATE_ADMIN_USER_PASSWORD_END);
      let successNotification = store.getActions().find(elem => elem.type === types.CREATE_NOTIFICATION);
      delete successNotification.notification.timer;
      expect(successAction).toEqual(expectedSuccessActions[1]);
      expect(successNotification).toEqual(expectedSuccessActions[2]);
    });
  });

[dyuvzhenko]

@dhilt Написал changeUser.spec.js - https://github.com/dhilt/dharmadict/commit/952502c5eac8e66fa447cac8299286e3243e4216. Наверное в схожем стиле перепишу остальные спеки, точнее немного подправлю.

[dyuvzhenko]

@dhilt Закончил писать тесты для actions/admin. И пока корректировал код для actions/newTerm заметил что по логике action'а saveTermAsync вполне допустима ситуация, чтобы запрос на сервер улетел с пустыми данными, wylie или отсутствующим sanskrit. У нас конечно есть защита на сервере от таких нехороших ситуаций, но тем не менее есть ли какой-нибудь смысл писать защиту от подобных ситуаций внутри actions или нет?

[dhilt]

@BitDen Отвечу на последний вопрос: на UI у нас есть защита от дурака через то, что мы блокируем кнопку до тех пор, пока все значения не будут введены. И по-хорошему должно хватить теста для компонента, что он не дает нажать на кнопку. Конечно, можно было бы и action creator защитить, но мне кажется, это будет overhead. Давай не будем отделять redux от компонента и опустим этот момент. Но мысль твоя верная и такие вещи надо иметь в виду и продвигать.

[dhilt]

@BitDen Я попробую дать комментарии по коду прямо в PR. Здесь это называется review.

[dhilt]

@BitDen Итак, см. https://github.com/dhilt/dharmadict/pull/33. Отвечать не надо, твои изменения, твои коммиты сами перекроют эти места. Надо было только тебе открывать PR, потому что сейчас я как бы сам себе review делаю в виде комментариев.

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

[dyuvzhenko]

@dhilt С нотификациями решил обойтись несколько иначе. Мне кажется неправильным локально внутри тестов создавать функцию, создающую уведомления, наверное так тестирование всегда будет успешным. В общем, импортирую notifier с ./app/helpers/notifier.js, и уже в _shared.js создаю уведомления, основываясь исключительно на коде происходящем из папки ./app. Коммит - https://github.com/dhilt/dharmadict/commit/9174fe535f424b33e2ea9ee9ba40f4c4e3ad0e23.

[dyuvzhenko]

@dhilt И код с initialState, как мне кажется наилучшим образом будет выглядеть так - https://github.com/dhilt/dharmadict/commit/13e0a63f8d06b33ba3da394b25ecb7f43efa5ed3. Во всех спеках нам нужно одно из двух: просто initialState(который не будет изменяться ввиду выясненных особенностей библиотеки redux-mock-store), либо же клон initialState (который при использовании с пустым параметром по default'у возьмет initialState). По мне так такой вариант кода, связанного с созданием initialState - просто сказка. Просто импортируется:

const {initialState, cloneInitialState} = require('../../_shared.js');

Просто используется:

let _initialState = cloneInitialState();
let specificState = cloneInitialState(_specifisState);
...
expect(reducer(initialState, expectedAction)).toEqual(expectedState);

Код выше не несет никакого смысла, просто как пример написал. Ну, я очень надеюсь вы согласитесь все же с таким вариантом, как в коммите.

[dhilt]

@BitDen Из этого видно, что с нотификациями мы не дошли до оптимального решения, слишком запутано выглядит, хотя использование в action creators уже вполне пригодное. Но на данном этапе я вполне согласен с результатом. И с клоном состояния я согласен, но в виде с параметром он должен называться просто cloneState:

const cloneState = (state = initialState) => JSON.parse(JSON.stringify(state));

[dyuvzhenko]

@dhilt Здорово! Переименовал - https://github.com/dhilt/dharmadict/commit/339488c8751f5995696a6828cb6f657b55905e7f.

[dyuvzhenko]

@dhilt Все моменты с review выполнил. Если еще есть какие-то предложения что можно изменить - пишите, но из спек смотрите только на changeUser.spec и changeUserPassword. Остальные спеки пока что в творческом беспорядке. Но вообще понимаю что громоздко получилось и стоит локальных функций подобавлять, и то что написано дважды - написать один раз в этих функциях. Примерно как в последнем коммите - https://github.com/dhilt/dharmadict/commit/1612532e82c6ab8f5c79266ab06599758b510d54. В спеке changeUserPassword только создание initialState c pending'ами равными true написано дважды, и первый тест функции changeAdminUserPassword возможно получится разбить на одну функцию.

[dyuvzhenko]

@dhilt Совсем отходя от темы тестов, недавно подумал, что у сервера нет защиты на приходящие данные. Допустим, при создании пользователя мы смотрим все ли 6 полей (name, descripition...) содержит пришедший с клиента объект user, но пропускаем мимо момент что пришедших полей у объекта может быть сколько угодно больше 6 и все эти посторонние поля могут содержать любые собственно данные. Возможно, стоит подумать о защите от таких дел?

[dhilt]

@BitDen Нет, защищаться от лишних полей мы не будем, если это конкретно не понадобится бизнес-модели. У нас на сервере фактически white list. Все данные, которые идут в БД, пропускаются через валидатор, все остальное мы пропускаем мимо, оно не играет никакой роли.

[dhilt]

@BitDen Я только что провел разбиение getAdminUserDataAsync теста: https://github.com/dhilt/dharmadict/commit/aed7b614c326b4ac67fa5cd0ef092380a87b3b09. Посмотри, насколько это стало читаемее. Нужно так же разбить следующий тест (про обработку ошибок getAdminUserDataAsync) и объединить 4 получившихся теста в один новый sub-descire describe('getAdminUserDataAsync ', в котором будет

• should work (reducer)
• should work (actions)
• should handle error (reducer)
• should handle error (actions)

И применить тот же подход к остальным тестам.

[dyuvzhenko]

@dhilt Довольно масштабно изменил changeUser.spec с учетом новых пожеланий - https://github.com/dhilt/dharmadict/commit/b79e01ee1da6c7b0c49f87e8ad73d03695f1d60e. Кода меньше не стало, но теперь как по мне все выглядит очень даже исчерпывающе и более-менее понятно. Что-то вроде документации, неаккуратно составленной. Начинается один sub-describe

describe('function updateAdminUserDataAsync'

далее пишутся объекты/функции createInitialState, expectedSuccessResponse, expectedSuccessActions, expectedSuccessState - тем самым ясно показывая какой ответ ожидается с сервера в случае успеха/ошибки, какие actions должны пройти, каким должен быть state до и после. В общем, кода стало чуть больше, но в таком виде его будет гораздо проще видоизменять если приложение будет меняться или расширяться, как мне кажется.

[dyuvzhenko]

@dhilt Все тесты для admin-actions переписал, готов к review и критике, так сказать.

[dhilt]

@BitDen

Следующая итерация (только 4 теста для getAdminUserDataAsync): https://github.com/dhilt/dharmadict/commit/1d1d443a18af75cdb4bf652ee725d37b9e00e63b

Я начал с того, что эти строки неправильны:

const user = translators[0];
let changedUser = user;
changedUser.name = 'Name of user was changed...';

Не смотря на const, постоянным остается только ссылка на объект, и третья строка аффектит исходный user. Это кроме того, что в тестах на getAdminUserDataAsync не должно быть никаких изменений по данным. Мы просто запрашиваем данные у сервера и вводим их в store.

Я переписал эту часть спеки. Не смотря на то, что я не скупился на пробелы и отступы, количество кода сократилось на 20 строк.

Еще меня начало преследовать чувство, что тестирование reducer не имеет смысла. Я имею в виду expect(reducer(initialState, actionsFail[0])).toEqual(statesFail[0]); – приведя эту логику к максимально простому виду, я увидел, что мы просто повторяем код reducer. В нашем случае это бессмысленно потому, что у нас нет логики в reducer, там только присваивания, а вся логика сосредоточена в actions. И таким образом мы просто дублируем код. То есть, чтобы проверить работоспособность reducer, нам надо переписать его 1 в 1 в тестах. Я хочу еще немного подумать на эту тему и, возможно, мы откажемся от тестирования reducer.

[dhilt]

@BitDen У тебя зарождается чувство прекрасного, на следующей стадии оно начнет вытеснять удовольствие от уже написанного кода в пользу жажды более чистого/правильного варианта, вынуждать тебя разрушать уже созданное ради лучшего. Иногда надо уметь отказаться от этого и бежать дальше, но сейчас мы занимаемся искусством и можем позволить себе не торопиться.

[dyuvzhenko]

@dhilt Касательно отказа от reducers, наверное стоит заметить что задумались мы о тестах в тот момент, когда ошибка приключилась именно с reducers. С getAdminUserDataAsync возможна следующая ситуация. Админ заходит на роут /translator/MK/edit, в state заполняется translatorInfo. Далее он может прогуляться по каким-либо другим компонентам, но по возвращении на роут /translator/MK/edit должен заново проброситься getAdminUserDataAsync и затереть в state dataSource и data новым пришедшим data с сервера.

[dyuvzhenko]

@dhilt И да, я всегда за более чистый и правильный вариант решения задачи, нежели избыточный и прекрасный. Только для меня и actions, и reducers выглядят пока что как дублирование кода. Допустим, в common.spec.js при тестировании changeUserLanguage выходит просто перезаписываю весь код из этой функции, с одним лишь дополнительным моментом - проверяю сработала ли функция lang.setUserLanguage(). В остальных же спеках подобных проверок нет.

[dyuvzhenko]

@dhilt Если посмотреть на функцию getAdminUserDataAsync, там собственно и проверять нечего в теле функции, если не считать дублирование:

export function getAdminUserDataAsync(userId) {
  return (dispatch, getState) => {
    const query = 'users/' + userId
    dispatch({
      type: GET_ADMIN_USER_DATA_START
    })
    const {id, dataSource} = getState().admin.editUser
    return asyncRequest(query, 'get', false, (data, error) => {
      dispatch({
        type: GET_ADMIN_USER_DATA_END,
        error: error ? error : null,
        data: error ? dataSource : getEditableUserDataObject(data.user),
        id: error ? id : data.user.id
      })
      error && dispatch(notifier.onErrorResponse(error))
    })
  }
}

[dyuvzhenko]

@dhilt В функции saveTermAsync можно только один момент проверить без дублирований, правильно ли выполнится код для вставки id в notification:

export function saveTermAsync() {
  return (dispatch, getState) => {
    const wylie = getState().admin.newTerm.wylie
    const sanskrit = getState().admin.newTerm.sanskrit
    dispatch({
      type: ADD_TERM_START
    })
    console.log('Let\'s start a new term add request to db! The term is "' + wylie + '".')
    return asyncRequest(`terms`, 'post', {term: wylie, sanskrit}, (data, error) => {
      dispatch({
        type: ADD_TERM_END,
        error: error,
        termId: !error ? data.term.id : null
      })
      let values = {}
      if (!error) {
        values.termId = data.term.id
      }
      dispatch(notifier.onResponse('NewTerm.alert_success', error, values))
    })
  }
}

И это вроде как одна единственная функция в admin-actions, которая помимо простого перебрасывания данных из state, или с серверного ответа в state делает что-то дополнительное.

[dhilt]

@BitDen А, например, в saveTranslationAsync логики довольно много. Я причем предлагаю логику формирования состояний/действий не дублировать, а захардкодить начальное состояние и конечный набор actions. Не пытаться делать тест универсальным, но всего лишь проверить на одном конкретном наборе (на двух, если говорить о тестировании ситуаций с ошибками). И как бы мало смыла в тестировании actions мы не увидели, в тестировании reducer его значительно меньше, так как в нем вообще нет собственной логики. А тестировать то, что он умеет мержить states не нужно, поскольку это базовый функционал, оттестированный его разработчиками.

Но давай пока оставим оба момента и примем решение потом. Главное не пытаться делать тесты универсальными, описывай набор входных данных и результат без вычислений, просто как есть, константами. Если возникает краевой случай (вроде ситуации с ошибкой), то добавляй еще один тест с еще одним набором констант. Последнее обновление changeUser.spec.js, как мне кажется, приближает нас к такому видению. Функции по формированию ожиданий актуальны например для changeAdminUserData, где вариантов входных данных может быть несколько при том, что принципиально набор выходных actions не меняется. Хотя и в этом случае я бы разнес тесты, я бы сделал отдельный явный it на каждый случай, завернув лишь ожидания в функцию. Но это сейчас непринципиально.

[dyuvzhenko]

@dhilt Начал писать search.spec - https://github.com/dhilt/dharmadict/commit/4455b3c9a43d02a53e8bf7c2847929e267abd63a. Правда, неудачно. Следующие строки показывают ошибку:

return store
        .dispatch(actionsCreators.doSearchRequestAsync())
        .then(() => expect(store.getActions()).toEqual(actions));

Функция хоть и требует некий callback, но на компоненте она используется без каких-либо параметров.