-
-
Notifications
You must be signed in to change notification settings - Fork 185
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 full e2e phoenix app test #797
Conversation
b1d4326
to
bfd8d21
Compare
I’m wondering if a better way to do this would be to instead have a Phoenix app in https://github.com/elixir-ecto/ecto_sql does all this setup and has been running it successfully for a while, so it could be a good source of inspiration. |
@whatyouhide yeah this makes sense, thanks for suggestion! |
4e6622d
to
f9848b3
Compare
So, what's our strategy here? Can we drop 1.13 support given that more recent Phoenix versions do not support it? |
0b45e48
to
b4c8956
Compare
09b7ab0
to
c52cb9e
Compare
I made |
c5cb3ca
to
efc209a
Compare
efc209a
to
ac1b4af
Compare
3a467d8
to
c3d78a2
Compare
@whatyouhide thank you so much for all the suggestions! I applied all of it + rebased/squashed + updated #784 (which waits for reviews too 🙏🏻) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing this locally, and noticed these issues:
-
The first time I ran
mix test.integrations
, it didn't work because I had to go into the Phoenix app directory and runmix deps.get
. I think we should shove that into the Mix alias itself. -
Can we fix this compilation warning within the Phoenix app?
==> phoenix_app
Compiling 18 files (.ex)
warning: defining a Gettext backend by calling
use Gettext, otp_app: ...
is deprecated. To define a backend, call:
use Gettext.Backend, otp_app: :my_app
Then, instead of importing your backend, call this in your module:
use Gettext, backend: MyApp.Gettext
lib/phoenix_app_web/gettext.ex:23: PhoenixAppWeb.Gettext (module)
The fix is exactly what it says it is 😄
defp test_paths(nil), do: ["test"] | ||
defp test_paths(integration), do: ["test_integrations/#{integration}/test"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this.
c3d78a2
to
3d18678
Compare
Here's a test setup that does e2e testing with a real phoenix app. This will be handy when testing more complex integrations like OTel stuff for a good start. It should also help us reduce the risk of introducing regressions in Phoenix apps (once we add more tests of course).
The only custom "trick" I had to use was defining endpoint in test/support because I made sentry depend on the phoenix_app from test/fixtures/phoenix_app and the endpoint needs access to Sentry.CapturePlug.
Another nice benefit is that we could set it up in the Sentry SDK project and it would make it simpler to test things out w/o having to create new setups manually.