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

Dynamic response #1

Merged
merged 8 commits into from
Jun 17, 2015
Merged

Dynamic response #1

merged 8 commits into from
Jun 17, 2015

Conversation

alexjeffburke
Copy link
Member

Can I get an initial review for this?

This pull request makes it it possible to supply a function as the response property which will generate a dynamic response based upon the request object.

This version allows an generating the response to be an asynchronous operation. It comes with tests of the success and failure cases.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.66% when pulling 5641801 on alexjeffburke:dynamic-response into 9aa8bd4 on unexpectedjs:master.

@papandreou
Copy link
Member

This is a really powerful feature and definitely a missing piece.

I have a feeling that it would be more flexible to expose it by letting the function generating the response take req and res as the first two parameters so that any existing node.js servers would be pluggable to some extent. It looks like you're doing that work in the mock CouchDB adapter here, so it would probably simplify that use case as well: https://github.com/alexjeffburke/unexpected-mock-couchdb/blob/integrate-mock-couch/lib/unexpectedMockCouchdb.js

Since mitm already provides the low-level mocked out req and res objects, it should mostly be a matter of populating a messy.HttpResponse instance based on whatever the user-supplied function does to the res object. Still a bit more work, but at least we'd only need to implement it once.

@alexjeffburke
Copy link
Member Author

With your kind explanation and that link the penny has dropped - I see what you were getting at.

Leave this with me :)

@papandreou
Copy link
Member

Great, looking forward to see what you'll come up with! And to using it.

@alexjeffburke
Copy link
Member Author

Right, I've just force pushed this branch again. This time it passes down req/res objects, intercepts the responses, arranges for the request assertion to run and then completes the underlying response and asserts that. Am very curious to see what you think :)

request: {
method: 'GET',
url: '/404'
},
Copy link
Member

Choose a reason for hiding this comment

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

Shorthand: request: 'GET /404' or even request: '/404'

@alexjeffburke
Copy link
Member Author

Ahh yes, cool. I think they may read like that now as a result of being fiddled with during some quality node-inspector time. Will look at another cleanup pass.

@papandreou
Copy link
Member

This looks really great! I'll be happy to merge it in its current state.

I have a couple of follow-up feature requests:

Capturing the low-level response

I recently changed unexpected-express to capture the response generated by the express app by looking at what's written to the socket rather than into the http.ServerResponse instance: https://github.com/unexpectedjs/unexpected-express/blob/d82330e5ab5e4a3dee00a694583f5e95e47a26ea/lib/unexpectedExpress.js#L269-L274 ... and use messy's low-level parser to parse both the headers and the body: https://github.com/unexpectedjs/unexpected-express/blob/d82330e5ab5e4a3dee00a694583f5e95e47a26ea/lib/unexpectedExpress.js#L344-L346

That enables us to assert things about the chunked encoding. The original usecase was a one we ran into at the ol' company where an express handler had to write bogus into the socket in order to force a browser downloading a big file to consider the download to have failed. Once a certain amount of data had been streamed, Chrome simply refused to realize that the download had failed unless we did that, otherwise it would leave a truncated file on disc and allow the user to open it. To test this piece of code we needed to assert that the raw response body (Transfer-Encoding: chunked) ended with XXX.

Also, node.js (0.12+ I think) plays some tricks when you provide non-ASCII chars in a header. If the chars can be represented in iso-8859-1, it will output them like that. This happens right before the bytes are written into the socket, so you need to listen the raw socket output in order to know what actually happened, neither res._header nor res.headers will reflect that.

Longer conversations with a dynamic response

Could there be some kind of shorthand for saying that a series of requests is to be processed by the same app?

@papandreou
Copy link
Member

I've invited you to be a collaborator on the project here on github and added you as an owner of the npm package. You're welcome to merge the PR yourself and release a new version.

@alexjeffburke
Copy link
Member Author

Thanks very much @papandreou, will gladly accept :)
Will do on the PR (exciting!). Two quick things, do you have any conventions about marking the merges reviewed and should it be a major or minor?

Re your feature requests:

  • Capturing the low-level
    This makes a lot of sense to me. Where I previously tried fabricating a ServerResponse I got in the interesting position of getting the headers in the stream I tried to buffer too. I'll sit down with those links and have a good read around.
  • Longer conversations
    That's an interesting thought - the way it is now, you'd have to store a reference and pass it into each response block. I wonder if that's going to be a new assertion and definitely worth throwing some language around to see what sticks.

@papandreou
Copy link
Member

I don't have any marking conventions, except for the conversation here :). Let's make this a minor release unless we suspect it'll break backwards compatibility.

alexjeffburke added a commit that referenced this pull request Jun 17, 2015
@alexjeffburke alexjeffburke merged commit 36a32ff into unexpectedjs:master Jun 17, 2015
@alexjeffburke alexjeffburke deleted the dynamic-response branch October 12, 2015 22:32
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.

None yet

3 participants