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

Test Methods don't seem to send body #185

Open
jeregrine opened this issue Jan 16, 2014 · 18 comments
Open

Test Methods don't seem to send body #185

jeregrine opened this issue Jan 16, 2014 · 18 comments
Labels

Comments

@jeregrine
Copy link

In real world usage the below works but in tests when I call

conn = post(conn, "/", %S/{"name":"Jason Stiebs","email":"[email protected]"}/)

And I try to parse it

{res, json} = JSON.decode(conn.req_body)

I get the exception that I need to fetch req_body. But I already am

conn.fetch([:params, :body, :headers])

If I add :req_body it blows up every non-post test.

@HashNuke
Copy link
Contributor

@jeregrine Can you post the whole test case?

I had the same problem, and I had to fix the arg I was passing to Dynamo.under_test() and setting the correct endpoint.

@jeregrine
Copy link
Author

test "creates a new user" do
    conn = post(conn, "/", %S/{"name":"Jason","email":"[email protected]"}/)

    resp = JSON.decode(conn.sent_body)
    assert Dict.get(resp, "name") == "Jason"
    assert Dict.get(resp, "email") == "[email protected]"
    assert Dict.get(resp, "id") != nil
    assert Dict.get(resp, "created_at") != nil
    assert conn.status == 201
  end

@jeregrine
Copy link
Author

Let me know if you need anything else @HashNuke the docs are not super clear so I was kind of shooting from the hip.

@devinus
Copy link
Member

devinus commented Jan 23, 2014

@jeregrine try:

conn = post("/", %S/{"name":"Jason","email":"[email protected]"}/)

@devinus
Copy link
Member

devinus commented Jan 23, 2014

test's don't start with a conn like e.g. get does, so you're calling post/3 when you actually want to CREATE a test connection.

@jeregrine
Copy link
Author

I believe I did try that first, but it failed so after looking at the source I tried that. Will try again.

@ericmj
Copy link
Contributor

ericmj commented Jan 23, 2014

@jeregrine The test you posted should fail to compile because of the undefined variable conn. Or am I missing something?

@jeregrine
Copy link
Author

@ericmj this code is basically just me flailing trying to get it working :).

Now that I've got it to rebuild I'm getting errors that it cannot fetch :session from the request. I'll need to look into this more tonight.

@HashNuke
Copy link
Contributor

@devinus, @jeregrine and others,

I'm sorry. I looked into this issue yesterday and put off posting an update on this thread and the Dynamo group.

The source of the issue is that the Test connection (Dynamo.Connection.Test) does not parse the raw request body into params[1], like the Cowboy connection module (Dynamo.Cowboy.Connection)[2]. So the tests in case_test.exs[3] pass, because it's checking for conn.sent_body but not for conn.params

I looked into making changes to parse the body, but that would involve parsing the headers to look for the www-url-encoded content type. In short, IMO, a lot of changes to the current code in the Test connection module.

I clarified with Jose about this on IRC. Plug does doesn't have this issue. The Plug Router API is complete. Maybe we should start making changes to Dynamo to run on top of Plug.

@devinus's call.

[1] - https://github.com/dynamo/dynamo/blob/master/lib/dynamo/connection/test.ex#L176-L180
[2] - https://github.com/dynamo/dynamo/blob/master/lib/dynamo/cowboy/connection.ex#L188-L193 and line-191 especially.
[3] - https://github.com/dynamo/dynamo/blob/master/test/dynamo/http/case_test.exs#L51-L54

@HashNuke
Copy link
Contributor

And for the POST request the appropriate header isn't being set in the Test connection module, creates a connection with just a host header and nothing else - https://github.com/dynamo/dynamo/blob/master/lib/dynamo/connection/test.ex#L30-L38

So that's where IMO the changes have to start from.

@jeregrine
Copy link
Author

👍 For integrating with Plug. If someone knows what needs to be done and is willing to task it out I'd be willing to help.

@HashNuke
Copy link
Contributor

Count me in too.
On Jan 24, 2014 8:57 PM, "Jason S." [email protected] wrote:

[image: 👍] For integrating with Plug. If someone knows what needs to
be done and is willing to task it out I'd be willing to help.


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-33231669
.

@nurugger07
Copy link

I'm in to help if I can

On Fri, Jan 24, 2014 at 10:35 AM, Akash Manohar [email protected]
wrote:

Count me in too.
On Jan 24, 2014 8:57 PM, "Jason S." [email protected] wrote:

[image: 👍] For integrating with Plug. If someone knows what needs to
be done and is willing to task it out I'd be willing to help.


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-33231669
.


Reply to this email directly or view it on GitHub:
#185 (comment)

@devinus
Copy link
Member

devinus commented Jan 24, 2014

@HashNuke Integration with Plug was always the plan. Plug was spawned out of discussions about extracting Dynamo's core. Integrating with Plug is going to be a huge endeavor as it means replacing a lot of components, so we need to make a plan.

@jeregrine
Copy link
Author

@devinus maybe we start a branch and a PR with a TODO list embedded? Similar to how bootstrap does their big pushes? twbs/bootstrap#6342

@patrickdet
Copy link

is the plug integration plan somewhere online or does that discussion just happen in the IRC?

@HashNuke
Copy link
Contributor

Nothing I'm aware of.

On Fri, Feb 14, 2014 at 4:08 PM, Patrick Detlefsen <[email protected]

wrote:

is the plug integration plan somewhere online or does that discussion just
happen in the IRC?

Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-35072997
.

@devinus
Copy link
Member

devinus commented Feb 14, 2014

@patrickdet Plug was designed out of discussions surrounding Dynamo. We're currently waiting for some things to land in Plug that Dynamo needs before using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants