search

ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.59k stars 1.33k forks source link

Some tests fail on Node.js v13 #1548

Open jonchurch opened 4 years ago

jonchurch commented 4 years ago

Just wanted to make a note that it looks like some changes in v13 of Node.js have broken some functionality here in strange ways. I noticed this when investigating https://github.com/kwhitley/apicache/issues/189, where some of their tests are failing based on supertest timeouts (and not a stable set, some almost always do, and others are intermittent).

I opened this issue here instead of supertest because some of supertest's tests fail too with the same timeout errors from apicache's tests, and the tests that fail here.

I wasn't able to tell what the issues are exactly, just that they give the following error across all three repos' tests (including this one's):

Error: Timeout of 5000ms exceeded. 
For async tests and hooks, ensure "done()" is called; 
if returning a Promise, ensure it resolves.

Steps to Reproduce

nvm install 13.10.1
node -v # 13.0.1 or up to current 13.10.1

cd superagent

npm t

After poking around, I wasn't able to figure anything out conclusive, but it seems the callback to .end is never being called. In at least one test I was able to narrow it down to a change in v13 that I think could cause the behavior.

https://github.com/visionmedia/superagent/blob/0de12b299d5d5b5ec05cc43e18e853a95bffb25a/test/node/parsers.js#L69-L81

This test fails, and the .end callback is never called. I think that could be because in v13 aborted requests no longer emit the end event, which I think superagent is using internally.

Admittedly, I do not have a clue if that's the root cause, but if there are abort calls happening in the lib in response to other things, or abort is being called by the native HTTP in response other changes in Node 13, then it would make sense that these timeout errors pop up because end never has its callback invoked.

SpineEyE commented 4 years ago

https://github.com/sindresorhus/got/issues/1107 related?

jonchurch commented 4 years ago

@SpineEyE The change breaking the linked issue was reverted w/ Node.js v13.11 Edit: Maybe not fully fixed yet actually, looks like got's tests are still breaking currently

The timeout error I reported here still happens on 13.11, I'm assuming due to the new behavior w/ abort which isn't planned for reversion so far as I know.

And to be clear w/ the maintainers, I know that v13 is experimental and it's expected for things to be breaking. Just want to track this for anyone experiencing it 👍