feat: request response tracing

[mefellows]

This should have been there since day 1. Response logging is a little more involved (it's a stream) but I'm still not sure it's really worth the effort of writing tests. I'd say the only value, is that the tests themselves are documentation for a feature (so perhaps they are).

In any case, it produces output that looks like this:

This should allow for a) much better bug reports if there are indeed issues and b) better debugging for users trying to work out what's going on.

FWIW I did look at tools like https://github.com/expressjs/morgan until I realised that the response body wasn't captured, and I needed another plugin for that. In the end, I decided that all of the hassle of an extra library wasn't worth it, and took inspiration from several sources on the best approach to stream the response body. Wrapping the http.write and http.end events (https://nodejs.org/api/http.html#http_response_write_chunk_encoding_callback) seemed to be the simplest option.

[coveralls]

Coverage decreased (-3.4%) to 90.772% when pulling ec503fd on feat/request-response-tracing into fb02dd6 on master.

[TimothyJones]

This is really great!!

Before merging, let's discuss:

1. Does this log for consumer tests too? It looks to me like it might not. If we're going to suggest using these logs in the issue template (which I think we should), I think we should do the same logging in consumer tests.

2. Since TRACE is the lowest level of logs, I wondered if we should promote this new logging to DEBUG, and demote internal pact stuff to TRACE?

Less importantly (this might just be personal preference):

3. I was hoping to one day remove lodash (it is the source of many of the vulnerability warnings). I also think the new code might be clearer if it were written natively instead of using lodash. What do you think?

[mefellows]

Does this log for consumer tests too? It looks to me like it might not. If we're going to suggest using these logs in the issue template (which I think we should), I think we should do the same logging in consumer tests.

No. I did think about it, and I'm not sure how to do it easily. Because on the provider side we actually have a proxy, it was relatively easy. On the consumer side, we'd need to hijack the HTTP runtime (perhaps I'll spike that today).

Since TRACE is the lowest level of logs, I wondered if we should promote this new logging to DEBUG, and demote internal pact stuff to TRACE?

For me, this is very much trace logging because it's passing through middleware that isn't apparent to end users that it even exists. Although I'm not ever confident in a real difference between DEBUG and TRACE - I could be convinced to abandon the use of trace altogether.

I was hoping to one day remove lodash (it is the source of many of the vulnerability warnings). I also think the new code might be clearer if it were written natively instead of using lodash. What do you think?

Interesting. I'm more of a fan of ramda these days anyway, and only brought across lodash to align to pact-node. Happy to make that change.

[TimothyJones]

No. I did think about it, and I'm not sure how to do it easily. Because on the provider side we actually have a proxy, it was relatively easy. On the consumer side, we'd need to hijack the HTTP runtime (perhaps I'll spike that today).

Ah, of course. Probably the easiest place to do that is in the mock service, and communicating the results back to pact-js. This issue is probably related: pact-foundation/pact-mock_service#123

For log levels, what do you think of:

DEBUG: Useful for users to debug their tests and find out why the results are what they are
TRACE: Useful for maintainers when debugging pact-js/node

[mefellows]

DEBUG: Useful for users to debug their tests and find out why the results are what they are
TRACE: Useful for maintainers when debugging pact-js/node

I think that's a fair delineation, but I think there will be examples where those lines cross over. But let's start with that as a guide (and perhaps add to a developer documentation).

I've just pushed up a small change that allows us to log request/responses from the consumer side. It's currently wrapped in a logging guard, so in theory should be safe to release (but we shouldn't release it yet without tests - I need to think about how to do this). It needs to be refactored out of where it is and cleaned up. But it does work.

Example:

What is really interesting about this change, is that in theory we have the tools within JS land to be able to give really good error messages about interactions not found etc. e.g. "we expected you to hit localhost:12345/some/thing but you instead hit yourserver.com/some/thing - this indicates you haven't configured your API client to hit the pact mock service

Or, perhaps even better... automatically redirect the request to the mock service (ala nock). The possibilities!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mefellows]

I think this is ready for review again Tim. I've made the suggested changes. There are ways to test this, but those ways involve more test code than actual code. I'm not sure how much value they'll add - but let me know.

[TimothyJones]

I reckon this is good to go! I won't merge it while Travis isn't working, but you can if you like 👍

I wrote to Travis to ask if they'll give us some OSS credits. If not/if that isn't enough, we'll have to move off them.