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

Add Plug.Router tests #1234

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

thymusvulgaris
Copy link
Contributor

@thymusvulgaris thymusvulgaris commented Jun 24, 2024

  • Add test that asserts an error is raised with the expected message when Plug.Router.match/3 is not given :to or :do option.

  • Add test that asserts an error is raised with the expected message when no routes are defined in a Plug module.

@josevalim
Copy link
Member

Thank you. However, I think matching on the error message would be more useful here. The exception class is not that interesting and we may be raising an unrelated error.

@thymusvulgaris
Copy link
Contributor Author

Thank you. However, I think matching on the error message would be more useful here. The exception class is not that interesting and we may be raising an unrelated error.

Thanks very much for the feedback - does the latest change work?

@thymusvulgaris
Copy link
Contributor Author

Updated PR description and squashed commits into one commit.

@thymusvulgaris
Copy link
Contributor Author

Updated test names.

@josevalim
Copy link
Member

Hrm, i thought we could use assert_raise to match only on the exception message but it is not possible. So I would use assert_raise/3 still but match on both exception and message. The message being the important one. Sorry for the wrong direction. :)

* Add test that asserts an error is raised with the expected
  message when Plug.Router.match/3 is not given :to or :do option.

* Add test that asserts an error is raised with the expected
  message when no routes are defined in a Plug module.
@thymusvulgaris
Copy link
Contributor Author

Hrm, i thought we could use assert_raise to match only on the exception message but it is not possible. So I would use assert_raise/3 still but match on both exception and message. The message being the important one. Sorry for the wrong direction. :)

No problem, thanks very much for the clarification.

@josevalim josevalim merged commit 90590ab into elixir-plug:main Jun 25, 2024
2 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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.

2 participants