forwardemail / forwardemail/superagent

Some tests fail on Node.js v13

Open
#1,548 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
16.6k
Forks
1.3k
PR merge metrics
No merged PRs in 30d

Description

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

```shell
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](https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V13.md#2019-10-22-version-1300-current-bethgriggs) 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](https://github.com/nodejs/node/pull/27984), 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.

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the failure with Node.js 13.10.1 and `npm t` in the superagent repository. Inspect `test/node/parsers.js`, especially the linked lines around the failing `.end` callback, and compare the request-abort behavior described in the Node.js v13 changelog and linked pull request. Done means the affected tests complete reliably without the 5000ms timeout.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.