elixir-maru / maru

Elixir RESTful Framework
https://maru.readme.io
BSD 3-Clause "New" or "Revised" License
1.32k stars 84 forks source link

`mount` doesn't work on `mix test` #42

Closed melpon closed 7 years ago

melpon commented 7 years ago

application code:

defmodule MyApp.Api.Mount do
    use Maru.Router
    get do
        json(conn, %{hello: :world})
    end
end

defmodule MyApp.Api do
    use Maru.Router
    mount MyApp.Api.Mount
end

test code:

defmodule MyAppTest do
    use ExUnit.Case
    use Maru.Test, for: MyApp.Api

    test "mount" do
        assert %{"hello" => "world"} = build_conn() |> get("/") |> json_response
    end
end

run mix test:

  1) test mount (MyAppTest)
     test/myapp_test.exs:5
     ** (Maru.Exceptions.NotFound) NotFound
     stacktrace:
       (maru) lib/maru/plugs/notfound.ex:11: Maru.Plugs.NotFound.call/2
       (gacct) lib/myapp/api.ex:8: MyApp.Api.call/2
       test/myapp_test.exs:6: (test)

It is caused by test ENV as far as I know. The issue is similar to https://github.com/falood/maru/issues/38.

I want to test a code using mount. Is there a way to do this?

falood commented 7 years ago

It's not a bug, it's a feature. You should test an endpoint with it's own test module. You should defined two test module for your case like this

defmodule MyApp.ApiTest do
    use ExUnit.Case
    use Maru.Test, for: MyApp.Api
    # write test here if there're some endpoint defined within MyApp.Api
end

defmodule MyApp.Api.MountTest do
    use ExUnit.Case
    use Maru.Test, for: MyApp.Api.Mount

    test "mount" do
        assert %{"hello" => "world"} = build_conn() |> get("/") |> json_response
    end
end
melpon commented 7 years ago

Thank you for replying.

If MyApp.Api is below code, to write test is difficult.

defmodule MyApp.Api do
    use Maru.Router

    plug Plug.Parsers,
        pass: ["*/*"],
        json_decoder: Poison,
        parsers: [:json]
    plug MyApp.Authorization
    plug :anyothermanyplugs

    namespace :foo do
        namespace :bar do
            mount MyApp.Api.Mount
        end
    end

    rescue_from MyApp.Error do
        ...
    end
end
defmodule MyAppTest do
    use ExUnit.Case
    use Maru.Test, for: MyApp.Api
    test "mount" do
        build_conn() |> get("/foo/bar") |> json_response  # failed!
    end
end
defmodule MyApp.Api.MountTest do
    use ExUnit.Case
    use Maru.Test, for: MyApp.Api.Mount

    test "mount" do
        build_conn() |> get("/") |> json_response  # this doesn't ensure specified url is into "/foo/bar"
    end
end
falood commented 7 years ago

I have got your point and I'll try to fix it. Thank you!

teodor-pripoae commented 7 years ago

Hi,

Any news about this issue ? I'm experiencing the same error (BaseRouter with complex logic) and I don't want to implement the logic in every API

falood commented 7 years ago

Hi @teodor-pripoae I'm trying, but I can't find better solution now. I'll keep going, and do you have any good idea?

teodor-pripoae commented 7 years ago

Did it work in previous releases ? I have an app using maru 0.9.0 which I was able to test it using espec. However, I was using maru behind phoenix. Now I'm using maru alone.

Example

defmodule MyApp.API do
  use Maru.Router

  plug :fetch_params

  mount MyApp.API.V1.SearchAPI
end
defmodule MyApp.API.V1.SearchAPI do
  use Maru.Router
  version "v1", using: :path

  namespace :search do
      get do
        %{} |> json
      end
   end
end
defmodule MyApp.API.SearchAPITest do
  use ESpec
  use Plug.Test

  @opts MyApp.Endpoint.init([])

  describe "/search" do
    it "returns 200" do
      conn = conn(:get, "/api/v1/search/")
            |> MyApp.Endpoint.call(@opts)
      ...
    end
  end
end
defmodule MyApp.Endpoint do
  use Phoenix.Endpoint, otp_app: :myapp

  plug Plug.Logger

  # Code reloading will only work if the :code_reloader key of
  # the :phoenix application is set to true in your config file.
  plug Phoenix.CodeReloader

  plug Plug.Parsers,
    parsers: [:urlencoded, :multipart, :json],
    pass: ["*/*"],
    json_decoder: Poison

  plug Plug.MethodOverride
  plug Plug.Head

  plug Maru.Plugs.Forword, at: "/api", to: MyApp.API

  plug :router, MyApp.Router
end
teodor-pripoae commented 7 years ago

I can try to help but I need to study the codebase first. I didn't use elixir in the past 1year and I started today to refactor an old app which I wrote 1 year ago using phoenix + maru to use only maru.

falood commented 7 years ago

May be the previous way is better, I'll try to revert back or make both works.

falood commented 7 years ago

@melpon @teodor-pripoae Hi gays, I think I have found the solution! I uploaded an example here Add Maru.Test.start to your test_helper.ex like here. And if a router mounted by more than one router, mark the branch router like here

You should use master branch for this feature, I'll make code clear and write documents before release a new version.

teodor-pripoae commented 7 years ago

Hi @falood, just tested the fix and it works ! I still have a small problem with it, the error handling in the base API module doesn't catch errors raised in the child module.

Example:

defmodule MyApp.API do
  use Maru.Router

  plug Plug.Parsers, parsers: [Plug.Parsers.URLENCODED, Plug.Parsers.JSON, Plug.Parsers.MULTIPART],
                     pass: ["*/*"],
                     json_decoder: Poison

  rescue_from [Maru.Exceptions.InvalidFormatter], as: e do
    ...
  end

  namespace :api do
    namespace :v1 do
      mount MyApp.API.V1.SearchAPI
    end
  end
end
defmodule MyApp.API.V1.SearchAPI do
  use Maru.Router

  version "v1"

  namespace :search do
    params do
      requires :query, type: String
      optional :per_page, type: Integer
      optional :page, type: Integer
    end

    post do
    ...
    end
  end
end

In my specs, I'm testing the scenario when someone is sending a request with empty query. I want to return 400 error code each time Maru.Exceptions.InvalidFormatter is raised. It works right now if I put the error handing code in the child API module. If I put it in parent API module, errors are not handled in tests.

falood commented 7 years ago

I think it's not a bug, it's better to raise exception. We just need to know an exception raised for unit test. Maybe it will resolve your problem when use this code for unit test:

assert_raise Maru.Exceptions.InvalidFormatter, fn ->
  post(...)
end
teodor-pripoae commented 7 years ago

Yes, I can do that but it doesn't actually solve the problem.

My app is returning standard json responses for every API. For 2xx I'm returning json objects. For 4xx and 5xx I'm transforming the exceptions into json errors. For example:

# 400
{
  "query": ["is required"],
}

Similar to the aproach we are doing with Grape in ruby:

rescue_from Grape::Exceptions::ValidationErrors do |e|
  error!({ messages: e.full_messages }, 400)
end

I'm trying to transform InvalidFormatter exception into a human readable error body and I want to put it in only one place (root API). I'm writing tests to ensure this edge cases are properly handled by the API.

falood commented 7 years ago

I'm sorry, maybe I got wrong point. Are you talking about unittest or general http request ?

teodor-pripoae commented 7 years ago

I'm taking about writing integrations tests for my APIs (simulating http requests).

falood commented 7 years ago

@teodor-pripoae check here I think with_exception_handlers option is what you want.

falood commented 7 years ago

Could I close this issue now?

teodor-pripoae commented 7 years ago

Hi,

I think your fix only works with modules defined in tests. With module defined in lib files it doesn't work.

I've put a sample project here which shows my usage.

  1) test mounted with exception handlers (MarutestTest)
     test/marutest_test.exs:4
     ** (ArithmeticError) bad argument in arithmetic expression
     stacktrace:
       (marutest) lib/marutest.ex:43: MWEH.M2.endpoint/2
       (marutest) lib/marutest.ex:26: anonymous fn/1 in MWEH.M2.__error_handler__/1
       test/marutest_test.exs:27: MarutestTest.TEST2.maru_test_call/1
       test/marutest_test.exs:34: MarutestTest.TEST2.test3/0
       test/marutest_test.exs:41: (test)
falood commented 7 years ago

I have fixed your test repo and pulled a request here

  1. you should use Maru.Test.start() not Maru.Builder.MountLink.start()
  2. rescue_from is based on pattern match, so you should make rescue_from :all as the last one.
teodor-pripoae commented 7 years ago

Thank you for your help!

It still does not work in my project but I will try to assemble a new example to see if I can reproduce it in a clean repo. Btw, I'm using Espec insted of Exunit and I'm using more plugs and middlewares in my project.

falood commented 7 years ago

I'm glad to help, feel free to post error message here.

teodor-pripoae commented 7 years ago

Hi,

Now it works, I had a few bugs:

defmodule MyApp.API do
  use Maru.Router

  mount V1.SearchAPI
  # later changed to
  mount MyAPP.API.V1.SearchAPI # and it works
end

defmodule MyApp.API.V1.SearchAPI do
...
end

Should mount fail if it cannot find the module instead of silently pass ?

falood commented 7 years ago

@teodor-pripoae Thank you for point it out, I have add warning message for both cases.

falood commented 7 years ago

I released maru v0.11.0 just now. Could this issue be closed?

teodor-pripoae commented 7 years ago

Yes. Thank you !

falood commented 7 years ago

@melpon Thank you very much for opening this issue. But it seem you missed the recent discussion. You can reopen it if you have any other question about it.

melpon commented 7 years ago

I have no question. Thank you for fixing the issue! 👍