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.Static tests #1236

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

thymusvulgaris
Copy link
Contributor

@thymusvulgaris thymusvulgaris commented Jun 26, 2024

  • Add test that asserts Plug.Static.call/2 serves files from the app directory using a :from with an application name and directory.


test "init/1 transforms an application name :from to a two-tuple :from with the application name and a directory" do
assert %{from: {:foo, "priv/static"}} = Plug.Static.init(at: "/", from: :foo)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I am not sure those tests are useful, because they are pretty much testing internals. However, I assume you wrote them to get full coverage, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, that was the goal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with José, init/1 is a callback that only makes sense together with call/2. Testing what init/1 returns is not important because anyway the important part is that then call/2 can accept what init/1 returns. So this is definitely testing internals IMO.

Copy link
Contributor Author

@thymusvulgaris thymusvulgaris Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback.

Is there a resource that could help me understand this approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have anything off the top of my head but searching for "black-box testing" for example could lead to some good resources.

Copy link
Contributor Author

@thymusvulgaris thymusvulgaris Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Does the remaining test also come under the above? I'm happy to close this PR if so :)

* Add test that asserts Plug.Static.call/2 serves files from the
  app directory using a :from with an application name and
  directory.
@josevalim josevalim merged commit cb9f1c7 into elixir-plug:main Jun 27, 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.

3 participants