Pages to pages transition (bug)

dhilt commented 6 years ago

1) Заходим на страницу Переводчика, http://dharmadict.ru/pages/DON. 2) Нажимаем "О проекте". 3) Адрес меняется на http://dharmadict.ru/pages/about.

Ожидаем: показ страницы соотв. О проекте.

Имеем: отображается страница переводчика.

dhilt commented 6 years ago

@khajjit На счет тестов, мне нравятся тесты на поиск логически важных блоков-контейнеров, тесты на реакцию изменений состояния, но тесты на наличие атрибутов/классов мне кажутся избыточными. Кстати, можно ли написать тесты на роутинг? и покрыть, в частности, описанный в issue сценарий?

Сайт заработал. Там как и в прошлый раз забыли оплатить хостинг (из-за surge ничего не падало, он просто-напросто давал потенциальную уязвимость).

dyuvzhenko commented 6 years ago

@dhilt Да, вижу баг, попробую что-нибудь придумать на этот счет..

Тестов на присвоенный className вроде бы совсем мало, всего в 6-и компонентах (SearchInput, Term, EditControls...). И все ориентированы на динамический расчет. Допустим, пошла загрузка, выставился pending, в тестах смотрим отобразился ли нужный className. Было бы здорово, если бы указали на какую-нибудь конкретную спеку, может тогда бы мне это более ярко бросилось в глаза.

Вот касательно избыточности есть сейчас наверное более важное предложение. Рассмотрим его на примере ./test/frontend/components/TranslatorPage.spec.js. Участок кода, начиная с 122-ой строки до конца спеки. На данном участке я написал тесты на то, как должны отобразиться строки на странице посредством FormattedMessage. И да, выглядят они довольно замысловато. Но суть простая - находим в приложении по data-test-id какую-нибудь переводимую строчку (допустим ссылку на описание переводчика) и смотрим совпадает ли текст этой строчки с тем, что находится в i18n-файлах по id, определенному в arrIntlStringsId (arrIntlStringsId это массив, содержащий пары вида: '[data-test-id="changeTranslatorPassword"]' и 'TranslatorPage.button_edit_password'). И подобный код я налепил в конце каждой спеки. Ну и сейчас мне кажется, что это полнейшая бессмыслица. Вроде как сама библиотека react-intl должна гарантировать нам корректное отображение текста в приложении по заданному id в FormattedMessage и мы, как пользователи библиотеки, не должны писать тесты на этот момент. Гораздо логичнее, как мне кажется, было бы проверять все id, имеющиеся на компоненте в FormattedMessage, на то, есть ли они в i18n-файлах. Это буквально следующие строчки, которые сейчас есть (немного их видоизменил):

const arrIntlStringsId = [
    'TranslatorPage.button_edit_password',
    'TranslatorPage.translations_language',
    'TranslatorPage.link_to_desc_page',
    'TranslatorPage.loading_text',
    'TranslatorPage.button_edit'
  ];

  languages.forEach(lang => {
    const i18n = require(appPath + 'i18n/' + lang.id);

    it(`should exists all i18n-texts for the component ${lang.id}`, () =>
      arrIntlStringsId.forEach(intlId =>
        expect(i18n.hasOwnProperty(intlId)).equal(true)
      )
    );
});

Либо же поступить хитрее, и написать одну спеку с кодом, который будет заглядывать в каждый компонент на ./app, выискивать в тексте с кодом все FormattedMessage, собирать id и далее проверять их в i18n-файлах.

dyuvzhenko commented 6 years ago

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

dhilt commented 6 years ago

@khajjit Твои рассуждения справедливы, и, суммируя, я предложил бы такой подход: тесты на i18n должны

проверить отображение/компиляцию 1 лексемы, скажем, на стартовой странице;
проверить нет ли в компонентах необъявленных лексем.

Но при этом ты не можешь перечислять в тестах в явном виде лексемы, их надо распарсить из шаблона каким-то образом. В противном случае мы получим ситуацию, когда лексему добавили в шаблон, не добавили в i18n-файлы и не добавили в тест – приложение с ошибкой. Тест должен сам видеть, сколько лексем хочет шаблон и смотреть, удовлетворяют ли этим требованиям i18n-файлы. Мне кажется, что подобной сложности тест нельзя написать штатными средствами. Хотя бы по тому, что мы ожидаем рендера, а рендер в подавляющем большинстве случаев условный, то есть 1 рендер компонента почти никогда не покрывает все лексемы, использующиеся в компоненте.

В связи с этим я бы

оставил ровно 1 тест на отработку 1 лексемы (просто чтобы убедиться, что механизм работает);
при наличии ресурсов составил бы в отдельном порядке nodejs-тест на соответствие (это был бы парсер, который обегает ./app/components/*.js, ищет в них блоки , берет их id и проверяет на наличие в i18n-файлах). Этот вариант меня, честно говоря, пугает своей ненативностью, но я сейчас не вижу, как можно было бы решить подобную задачу в общем.

dhilt commented 6 years ago

@khajjit На счет роутинга, я уверен, должно существовать несколько подходов. Например тут описан тест на редирект, который можно применить для отработки сценария захода на защищенный роут неадмином, а также на несуществующий роут (должны увидеть компонент NotFound).

Тест, покрывающий описание настоящего issue должен, как мне видится, отрендерить /pages/DON, после чего нужно программно сменить url и посмотреть на реакцию. Не уверен, что это просто сделать, т.к. /pages/:pageUrl у нас завязаны на БД. Но так или иначе на Page component нужно слушать изменение url location и дергать getPageAsync. Чем больше я об этом думаю, тем больше мне кажется, что это тест не на роутер, а на компонент. Роутер не будет провоцировать ре-рендер, т.к. в описанном сценарии мы остаемся в пределах /pages/:pageUrl. Итого, нужно научить компонент слушать изменения url и добавить компонент-тест, покрывающий эту функциональность.

dyuvzhenko commented 6 years ago

@dhilt Баг убрал - https://github.com/dhilt/dharmadict/commit/ed849bf151d461a2d10ad02b92a02c31e47a1232.

dyuvzhenko commented 6 years ago

@dhilt Далее попробую написать парсер..

dyuvzhenko commented 6 years ago

@dhilt Так, что-то работающее написал - https://github.com/dhilt/dharmadict/commit/35584a47df6576ad6dddc97c6d1bfa6fddd28f39. Один тест на простое отображение FormattedMessage по id. Второй тест на отображение FormattedMessage по id и по values. (Для первого теста и второго пришлось два разных компонента брать. Так как удобного компонента, сочетающего в себе два случая, не нашел) И третий тест ищет в ./app/components/*.js все id. Надеюсь получилось более-менее понятно сделать этот момент. Раздробил эту задачу на четыре функции:

getFiles - возвращает массив со всеми файлами из ./app/components/*.js в текстовом виде;
getFormattedMessagesFromFile - из одного файла берем все теги вида <FormattedMessage id=... />;
getArrIntlId - из массива со строками, полученными из предыдущей функции, создает и возвращает массив с чистыми id, например 'TranslatorPage.request_error';
getAllId - объединяет в себе предыдущие три функции и в итоге создает массив со всеми id, которые были найдены в ./app/components/*.js.

dyuvzhenko commented 6 years ago

И как итог, почти 700 лишних строк кода удалось убрать - https://github.com/dhilt/dharmadict/commit/d85f752ac3054c30c89375c65589b6348199f54a. Далее напишу тесты для сервера, затем на actions и components, и перейду к роутингу.

dyuvzhenko commented 6 years ago

@dhilt Покрыл тестами момент со сменой роута на компоненте Page - https://github.com/dhilt/dharmadict/commit/2415de5c6fcc8233fcc29fc0b09cc76e817ad389. Насчет тестов на все роуты в целом пока не знаю, думаю, и это наверное может затянуться надолго.. Может стоит попробовать это делать уже в бранче test-routes?

dhilt commented 6 years ago

@khajjit Все переходы не будем покрывать, мы доверяем роутеру в целом, но отдельные частные случаи включать в тесты имеет смысл.

Я довел в новом скроллере базовую функциональность до 85% работоспособности, можно мержить бранч workflow-2 в твой бранч, покрывать тестами. Тем временем я завершу текущий спринт по Словарю.

dhilt commented 6 years ago

@khajjit Я переписал обновления роутинга, см. actions/route.js. У тебя отсылалось по два запроса на каждый переход и к этому еще было промаргивание контента. С промаргиванием разобраться было несложно, но я решил переделать это место целиком. Соответственно, тесты на Page routing в том виде, в котором они сейчас написаны, теряют смысл. И покрывать тестами надо actions/route.js. Но давай отложим этот момент, т.к. это уже 100% end-to-end история, подчисти только Page tests.

dyuvzhenko commented 6 years ago

@dhilt Page tests почистил - https://github.com/dhilt/dharmadict/commit/b1b5254a9ff3591b89eaf021c0031c482b208654. И там вы еще сделали какие-то изменения по верстке и теперь отображение страницы в FF неплохо так отличается от того, что есть в Chrome. Еще надо и на это посмотреть..

Касательно скроллера, я думаю может лучше будет до конца решить проблемы с тестами, и возникшими там асинхронностями? А как проблемы разрешатся, так сразу мержить можно.

dyuvzhenko commented 6 years ago

@dhilt И теперь есть гарантированная ошибка неполучения страницы на роуте вида localhost:5000/pages/HOP/edit, потому что идет второй запрос на несуществующий url 'HOP/edit'. И плюс к этому не выпадает dropdown menu с языками. В общем, ошибки посыпались щедро похоже, пока только не совсем понятно из-за чего именно.

dyuvzhenko commented 6 years ago

@dhilt Кажется догадался в чем ошибка. У нас в index.html ссылка на стили с bootstrap'ом вела на самые последние стили, в то время как верстка была завязана в целом на версию 3.3.7 вроде как. В общем, указал ссылку конкретно на эту версию и проблемы исчезли. Видимо у бутстрапа новая версия вышла или что-то вроде того... В любом случае если сейчас зайти на dharmadict.ru в приватном режиме браузере, ну или просто очистив кеш, можно увидеть не самую лучшую картину, вот.

dyuvzhenko commented 6 years ago

@dhilt Ну и закрыл проблему с двойным запросом на сервер на роутах /new и /edit для админа, хотя мне кажется это сомнительное решение проблемы - https://github.com/dhilt/dharmadict/commit/9e52ba758a89280769aa0fa38f2325fc8032f58d.

dhilt commented 6 years ago

@khajjit Действительно! хорошо, что ты это держишь в голове. Твое решение нормально, суть чтобы все игры с роутингом были бы сосредоточены в одном месте и проходили бы по одним правилам, и это место – actions/route.js

Со стилями я думал даже попросить Сашу Соколова глянуть, это человек, с которым мы хотели Ленту делать, он же изъявил на днях желание посмотреть на скроллер, возможно у нас будет команда... Завтра я на чистую голову посмотрю то, что ты написал по стилям и, надеюсь, выпущу релиз.

Спасибо!

dhilt commented 6 years ago

Done per PR https://github.com/dhilt/dharmadict/pull/43.

dhilt / dharmadict

Pages to pages transition (bug) #42