-
-
Notifications
You must be signed in to change notification settings - Fork 736
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
feat: remember the request history (nock#2171) #2750
base: main
Are you sure you want to change the base?
Conversation
I'm interested in doing some analysis on HTTP requests made by a service of mine. In your opinion--or in the opinion of any maintainer--how suitable would it be for this project to capture all requests (not just intercepted requests)? Edit: specifically, I'm looking to make assertions about my request bodies for real requests. This is not a hard requirement of mine, just considering my options. |
@PersonTheCat You probably want the Recording mode |
I'm really looking forward to this. This would make it so much more pleasant for me to work with nock.
having this similar in nock would feel natural and intuitive. |
tests/test_request_history.js
Outdated
}) | ||
|
||
it('remembers requests', async () => { | ||
const interceptor1 = nock('http://example.test').persist().get(/\/dsad1.*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that this may lead to OOM errors and/or memory leak-like behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say unit-tests typically don't send enormous payloads, or make thousands of requests.
Anyhow: remembering 2000 get-requests getting "foo" as answer: https://gist.github.com/sezanzeb/d5b89e10bdebf2cfb6192983743aa9eb shows 101MiB of memory usage in my system monitor. Without any requests 48MiB are needed.
Also, requests should be cleaned up after every test, if the interceptor isn't created in beforeEach callbacks. I just realized this still needs to be implemented, like https://jestjs.io/docs/mock-function-api#mockfnmockclear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, but users may use Nock in unexpected ways. WDYT about saving only the last ten requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a warning being written to the console when the maximum number of requests is stored, and if it is configurable, it would be an acceptable compromise I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative:
nock('http://example.test')
.persist()
.get(/\/dsad2.*/)
.remember(10) // the argument is null by default: remembers all requests
without .remember()
, no requests are saved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is nice and fits well into nock, and is optional, so existing tests won't remember requests. Therefore the limit can be infinite by default. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding features is easy. Maintaining them is something else :)
I know that using persist is a known pattern with nock
, but can you please share with me why you use it instead of regular interceptors?
Don't get me wrong—I'm in favor of inserting this feature to nock,
but I think we need to be careful how we implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also do
it('remembers a request', async () => {
const interceptor = nock('http://example.test')
.get('/foo')
.remember()
interceptor.reply(202, '1')
await sendRequest('example.test', '/foo')
expect(interceptor.requests).to.have.lengthOf(1);
interceptor.reply(202, '1')
await sendRequest('example.test', '/foo')
expect(interceptor.requests).to.have.lengthOf(2);
})
without persist
.
I have commited that new test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the request object should be remembered in the scope as well:
const scope = nock('http://example.test')
.get('/foo')
.remember()
.reply(202, '1')
await sendRequest('example.test', '/foo')
console.log(scope.request)
Though I'm somewhat confused with scopes and interceptors. A scope has a list of this.interceptors
. But doing a single .reply(202, '1')
on an interceptor creates a scope for a single request, which suggests that a scope belongs to a single interceptor.
In general, I like this idea because it let us to move the assertions to the end of the test file, which is more natural: it('should..', () => {
// Arrange
const scope = nock(..).post(..).reply(200);
// Act
await fetch(...);
// Assert
expect(scope.requests[0].body).toBe('Hello World');
}); vs today: it('should..', () => {
let body;
// Arrange
const scope = nock(..).post(..).reply(200, (uri, requestBody) => { body = requestBody;});
// Act
await fetch(...);
// Assert
expect(body).toBe('Hello World');
}); |
I think we need to work on the |
lib/intercepted_request_router.js
Outdated
@@ -305,6 +305,8 @@ class InterceptedRequestRouter { | |||
) | |||
|
|||
if (matchedInterceptor) { | |||
matchedInterceptor.requests.push(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClientRequest
does not have a friendly API (think about getting the body). Maybe we should transform it to a Request
instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how?
7dcdc7a
to
3a6ffdd
Compare
Fixes #2171 properly
also contains updated tests and documentation.