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

update deps #146 #147

Merged
merged 14 commits into from
Oct 12, 2021
Merged

update deps #146 #147

merged 14 commits into from
Oct 12, 2021

Conversation

nelsonic
Copy link
Member

@nelsonic nelsonic commented Oct 6, 2021

This is a maintenance PR that revives the project and brings it up to date with the latest Elixir & Phoenix. 🙌

@nelsonic nelsonic added the in-progress An issue or pull request that is being worked on by the assigned person label Oct 6, 2021
@nelsonic nelsonic self-assigned this Oct 6, 2021
mix.exs Outdated Show resolved Hide resolved
@nelsonic
Copy link
Member Author

nelsonic commented Oct 7, 2021

** (Mix) Could not start application auth: Auth.Application.start(:normal, []) returned an error: shutdown: failed to start child: AuthWeb.Endpoint
    ** (EXIT) an exception was raised:
        ** (UndefinedFunctionError) function Phoenix.LiveReloader.Socket.child_spec/1 is undefined (module Phoenix.LiveReloader.Socket is not available)
            Phoenix.LiveReloader.Socket.child_spec([endpoint: AuthWeb.Endpoint])
            (elixir 1.12.3) lib/enum.ex:1582: Enum."-map/2-lists^map/1-0-"/2
            (phoenix 1.6.0) lib/phoenix/endpoint/supervisor.ex:105: Phoenix.Endpoint.Supervisor.init/1
            (stdlib 3.15.2) supervisor.erl:330: :supervisor.init/1
            (stdlib 3.15.2) gen_server.erl:423: :gen_server.init_it/2
            (stdlib 3.15.2) gen_server.erl:390: :gen_server.init_it/6
            (stdlib 3.15.2) proc_lib.erl:226: :proc_lib.init_p_do_apply/3

assets/vendor/topbar.js Outdated Show resolved Hide resolved
@nelsonic

This comment has been minimized.

assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
assets/vendor/topbar.js Outdated Show resolved Hide resolved
@SimonLab
Copy link
Member

I have the same failing tests as mentioned above.
I first try to run the application with the google environment variables defined on Heroku and got the following error:
image

I'll check if the tests are using mocks or using the google application. If the later we might just need to update the application on Google.

@SimonLab
Copy link
Member

I've recreated a google oauth application and login works on localhost. However the test are still failing.

  def google_handler(conn, %{"code" => code, "state" => state}) do
    {:ok, token} = ElixirAuthGoogle.get_token(code, conn) # error here
    {:ok, profile} = ElixirAuthGoogle.get_user_profile(token.access_token)
    # save profile to people:
    app_id = get_app_id(state)
    person = Person.create_google_person(Map.merge(profile, %{app_id: app_id}))

    # render or redirect:
    handler(conn, person, state)
  end

The function get_token from the ElixirAuthGoogle library doesn't work at this point. Looking at this now

@SimonLab
Copy link
Member

SimonLab commented Oct 11, 2021

I think the issue is that we are running the test using request to Google instead of using mocks.
We first call ElixirAuthGoogle.get_token(code, conn) with a dummy code.

The get_token function is defined as:

  def get_token(code, conn) do
    body = Jason.encode!(
      %{ client_id: System.get_env("GOOGLE_CLIENT_ID") || Application.get_env(:elixir_auth_google, :client_id),
         client_secret: System.get_env("GOOGLE_CLIENT_SECRET") || Application.get_env(:elixir_auth_google, :client_secret),
         redirect_uri: generate_redirect_uri(conn),
         grant_type: "authorization_code",
         code: code
    })
    inject_poison().post(@google_token_url, body)
    |> parse_body_response()
  end

Then the inject_poison function is called

 inject_poison().post(@google_token_url, body)

This function is defined at compile time with:

  @httpoison Mix.env() == :test && ElixirAuthGoogle.HTTPoisonMock || HTTPoison

  @doc """
  `inject_poison/0` injects a TestDouble of HTTPoison in Test
  so that we don't have duplicate mock in consuming apps.
  see: https://github.com/dwyl/elixir-auth-google/issues/35
  """
  def inject_poison(), do: @httpoison

I think at this point the function run the real post request instead of the mock. It might be linked to dwyl/elixir-auth-google#37 and dwyl/elixir-auth-google#41

@SimonLab
Copy link
Member

SimonLab commented Oct 11, 2021

Updating manually the elixir-auth-google dependency and updating inject_poison to:

  def inject_poison() do
     Mix.env() == :test && ElixirAuthGoogle.HTTPoisonMock || HTTPoison
  end

Make the current failing tests pass (there are two more failing test link to b58 that we can debug a bit later)
However I'm not sure what is the best solution as I don't want to undo the work done with dwyl/elixir-auth-google#41
I'm trying to understand if a dependency can be compile in a test environment and so we could continue to use the current library version however I didn't have any luck at the moment. Also I think we might also need to see if we can untangle our tests as ideally our application shouldn't really use mock tests from its dependency.

edit:
see: https://elixirforum.com/t/mix-env-at-the-compile-time-always-return-prod-when-in-deps/9549
Apparently dependency are always compiled in production environment. If this is still the case we won't be able to use the mock from the dependency if we use Mix.env at compile time.
see: https://elixir-lang.org/getting-started/mix-otp/introduction-to-mix.html#environments

The environment applies only to the current project. As we will see in future chapters, any dependency you add to your project will by default run in the :prod environment.

@nelsonic
Copy link
Member Author

@SimonLab the "Test Double" (mocked version) for Google Auth was deliberate: dwyl/elixir-auth-google#35
More info on Test Doubles: https://martinfowler.com/bliki/TestDouble.html
via: https://softwareengineering.stackexchange.com/questions/408267/export-mock-versions

Thanks for diving into the the Mix.env() ... The annoying thing is that this used to "just work"! 💭

@nelsonic nelsonic marked this pull request as ready for review October 11, 2021 23:05
@nelsonic nelsonic added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Oct 11, 2021
@nelsonic nelsonic assigned SimonLab and unassigned nelsonic Oct 11, 2021
@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #147 (de63efe) into main (5d04424) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #147   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          544       545    +1     
=========================================
+ Hits           544       545    +1     
Impacted Files Coverage Δ
lib/auth_web/controllers/auth_controller.ex 100.00% <ø> (ø)
lib/auth_web/router.ex 100.00% <ø> (ø)
lib/auth/app.ex 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d04424...de63efe. Read the comment docs.

@nelsonic nelsonic removed the priority-1 Highest priority issue. This is costing us money every minute that passes. label Oct 11, 2021
@nelsonic
Copy link
Member Author

Hi @SimonLab hopefully this PR is self-explanatory and you've been following along. 😉
Please review/merge when you can. Thanks! 👍

scope "/dev" do
pipe_through :browser

forward "/mailbox", Plug.Swoosh.MailboxPreview
Copy link
Member

Choose a reason for hiding this comment

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

I'll have to check Swoosh. Not sure if you wanted to add it to this PR or later on

Copy link
Member

Choose a reason for hiding this comment

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

Just saw it's part of the Phoenix 1.6 code 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, need to investigate it. Email in general. Specifically receiving replies. 💭

Copy link
Member

@SimonLab SimonLab left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for debugging the test double 👍

@SimonLab SimonLab merged commit 820b80c into main Oct 12, 2021
@SimonLab SimonLab deleted the update-elixir-v1.12-issue-145 branch October 12, 2021 06:55
@nelsonic
Copy link
Member Author

Latest version deployed to Heroku:
https://dashboard.heroku.com/apps/dwylauth/activity/builds/e4adaad1-a7b8-4e16-a5cd-8d0289190773
image

Going to test it on my side. 👨‍💻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants