Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#260] fix workspace email validation message interpolation with test #278

Merged
merged 5 commits into from
Jul 15, 2024

Conversation

d1z3d
Copy link
Contributor

@d1z3d d1z3d commented Jun 15, 2024

Добрый день!

В dto при валидации email-а была ошибка в интерполяции. Внес изменения в dto и в тест.
Билд запустил, ошибок нет (приложил скрин ниже).

В других dto также неправильно указана интерполяция. В них будет выводиться "The email "Email" is not valid". Нужно внести изменения во все остальные dto?

Снимок экрана 2024-06-16 в 00 49 01

@d1z3d d1z3d mentioned this pull request Jun 15, 2024
@bazilval
Copy link
Collaborator

@d1z3d отлично, получается удачно выловили баг вашим тестом!
Давайте этим пр сразу поправим все сообщения валидации, где ошибка в интерполяции. Посмотрите сразу, возможно не только в валидации емейла такое встречается, но и в прочих

@d1z3d
Copy link
Contributor Author

d1z3d commented Jun 16, 2024

@bazilval, поправил dto по созданию и обновлению аккаунта. Добавил тесты, а также некоторые обновил.
Решил не добавлять тесты по обновлению аккаунта с некорректным email, а изменить существующие, но они были неправильные. В ответе возвращался 401 http статус.

Визуально не заметил, что есть где-то еще похожие проблемы при подстановки динамических параметров.
Есть различия в отображение валидации на фронте. Здесь нужны правки?
Снимок экрана 2024-06-16 в 21 48 20

Снимок экрана 2024-06-16 в 21 49 05

@d1z3d d1z3d marked this pull request as ready for review June 16, 2024 23:26
@bazilval
Copy link
Collaborator

bazilval commented Jun 16, 2024

@d1z3d Отлично! Наверное хорошо бы к одному виду привести, второй вариант выглядит опрятнее!

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

@d1z3d
Copy link
Contributor Author

d1z3d commented Jun 23, 2024

@bazilval, добрый день!
Привел к единому формату стили в отображении ошибок.
Подключил faker и instancio, переписал тесты в accountController.
Написал утилитарный класс для проверки ошибок валидации через resource bundle.

@bazilval, в jakarta validation используется свой resource bundle, в зависимости от используемого языка.

render - https://hexlet-correction-ac3h.onrender.com

@bazilval
Copy link
Collaborator

@d1z3d если честно, мне показалось, что с instancio тесты стали менее читаемы, а значит сложнее в поддержании. В данном случае я пока не понял насколько нам действительно нужно это, потому что в случае со старыми тестами мы сразу видим что именно проверяется и какими данными.

Можешь немного пояснить почему решил добавить?

@d1z3d
Copy link
Contributor Author

d1z3d commented Jun 26, 2024

@bazilval, здравствуйте!

  • Одна из задач, которую я видел - использование новых подходов, которые используются в проекте hexlet-components/java-spring-blog. Тестовые объекты генерируются как раз за счет instancio.
  • Если сущность зависит от другой, то достаточно будет вызвать create у определенной модели и уже её использовать. В финальном проекте в java использовал такой подход. Лично мне было удобно так описывать сам тест, потому что в самом тесте описывались шаги, которые нужно было пройти для успеха, а сама подготовка данных была описана отдельно.
  • Этот пункт вытекает из предыдущего пункта. Если меняется сущность, например, добавляется или удаляется какое-либо поле, то это не меняет поведение теста, а меняет только данные, которые подготавливаются для тестирования.

@d1z3d
Copy link
Contributor Author

d1z3d commented Jun 26, 2024

Хотя подумав над 3 пунктом снова понял, что это не так, так что он некорректный)

@bazilval
Copy link
Collaborator

@d1z3d хорошо, тогда дождёмся ревью от @Malcom1986
мб имеет смысл вынести это в отдельный PR, так как это немного меняет подход в тестировнии и возможно в каких-то других тестах его тоже следует применять

посмотрите ещё выше тут, я некоторые моменты отметил (формулировка в одном месте и вопрос по bundleSource)

@HelenOne HelenOne requested a review from Malcom1986 July 1, 2024 08:41
@Malcom1986
Copy link
Collaborator

Идея с Instancio неплохая, но я б ее на самом деле вынес в отдельный ПР. Тут бы все тесты тогда доработать. А с текущей задачей оно не связано по смыслу получается

@Malcom1986
Copy link
Collaborator

@d1z3d Андрей, поправите? Под использование instancio можно отдельный тикет создать. Но эта задача не особо критична, скорее просто улучшалка такая. Плюс еще можно поизучать, возможно есть готовые решения, чтобы маппить класс на параметры формы

@d1z3d
Copy link
Contributor Author

d1z3d commented Jul 8, 2024

@Malcom1986, Максим, добрый день! Да, внесу правки. Смогу сделать ближе к выходным.

@d1z3d
Copy link
Contributor Author

d1z3d commented Jul 14, 2024

@Malcom1986 Максим, внес правки, только в тесте оставил запрос от корректного пользователя, иначе эти тесты выполняются в холостую из-за отсутствия проверки http-кода ответа.

@Malcom1986 Malcom1986 merged commit d495bd2 into Hexlet:main Jul 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants