[fetch-utils] Fix unit tests (#20210)

* [fetch-utils] Fix FetchUtilsTests.js tests

* Cleanup between tests
* Define `AbortError`

* [fetch-utils] Add `expectConnectionCount` showcasing that the connection stays on when the stream is destroyed

* Revert "[fetch-utils] Add `expectConnectionCount` showcasing that the connection stays on when the stream is destroyed"

This reverts commit b10da7b3fc06a7345df8fd70f27fad70a478bbb4.

* [fetch-utils] Fix `supports abort signals` test

* [fetch-utils] Add tests "aborts the request when the request body is destroyed during transfer"

* [fetch-utils] Add extra waiting step before `expectRequestAborted`

* [fetch-utils] Add `while (!req?.destroyed) await wait(10)`

Unfortunately I couldn't find a better event to await.

To try with this to test flakiness
```
for i in {1..100}; do npm run test:unit || break; echo "Run $i completed"; done
```

* [fetch-utils] Replace arbitrary `wait(10)` by `this.server.waitForEvent('request-received')`

* [fetch-utils] Improve tests per PR comments

See https://github.com/overleaf/internal/pull/20210

1. Replace `waitForEvent` with `once`
2. Replace `waitForRequestAborted` by `expectRequestAborted`
3. Add comment in `expectRequestAborted` empty catch block

Non-flakiness rechecked with `for i in {1..100}; do npm run test:unit || break; echo "Run $i completed"; done`

* [fetch-utils] Add small wait before checking `lastReq === undefined`

Per https://github.com/overleaf/internal/pull/20210#discussion_r1743329462

GitOrigin-RevId: 5fe542e0a8e6011307e03237e2f81404d9a0e674

libraries/fetch-utils/test/unit/FetchUtilsTests.js

@@ -1,5 +1,5 @@
 const { expect } = require('chai')
-const { FetchError, AbortError } = require('node-fetch')
+const { FetchError } = require('node-fetch')
 const { Readable } = require('stream')
 const { once } = require('events')
 const { TestServer } = require('./helpers/TestServer')
@@ -15,6 +15,8 @@ const {
   CustomHttpsAgent,
 } = require('../..')
 
+const AbortError = 'The user aborted a request'
+
 const HTTP_PORT = 30001
 const HTTPS_PORT = 30002
 
@@ -48,6 +50,10 @@ describe('fetch-utils', function () {
     this.httpsUrl = path => `https://example.com:${HTTPS_PORT}${path}`
   })
 
+  beforeEach(function () {
+    this.server.lastReq = undefined
+  })
+
   after(async function () {
     await this.server.stop()
   })
@@ -135,7 +141,7 @@ describe('fetch-utils', function () {
       await expectRequestAborted(this.server.lastReq)
     })
 
-    it('aborts the request when the request body is destroyed', async function () {
+    it('aborts the request when the request body is destroyed before transfer', async function () {
       const stream = Readable.from(infiniteIterator())
       const promise = fetchStream(this.url('/hang'), {
         method: 'POST',
@@ -143,6 +149,20 @@ describe('fetch-utils', function () {
       })
       stream.destroy()
       await expect(promise).to.be.rejectedWith(AbortError)
+      await wait(80)
+      expect(this.server.lastReq).to.be.undefined
+    })
+
+    it('aborts the request when the request body is destroyed during transfer', async function () {
+      const stream = Readable.from(infiniteIterator())
+      // Note: this test won't work on `/hang`
+      const promise = fetchStream(this.url('/sink'), {
+        method: 'POST',
+        body: stream,
+      })
+      await once(this.server.events, 'request-received')
+      stream.destroy()
+      await expect(promise).to.be.rejectedWith(AbortError)
       await expectRequestAborted(this.server.lastReq)
     })
 
@@ -164,6 +184,7 @@ describe('fetch-utils', function () {
       const stream = Readable.from(infiniteIterator())
       await expect(
         fetchStream(this.url('/hang'), {
+          method: 'POST',
           body: stream,
           signal: AbortSignal.timeout(10),
         })
@@ -178,7 +199,7 @@ describe('fetch-utils', function () {
       await expectRequestAborted(this.server.lastReq)
     })
 
-    it('aborts the request when the request body is destroyed', async function () {
+    it('aborts the request when the request body is destroyed before transfer', async function () {
       const stream = Readable.from(infiniteIterator())
       const promise = fetchNothing(this.url('/hang'), {
         method: 'POST',
@@ -186,6 +207,20 @@ describe('fetch-utils', function () {
       })
       stream.destroy()
       await expect(promise).to.be.rejectedWith(AbortError)
+      expect(this.server.lastReq).to.be.undefined
+    })
+
+    it('aborts the request when the request body is destroyed during transfer', async function () {
+      const stream = Readable.from(infiniteIterator())
+      // Note: this test won't work on `/hang`
+      const promise = fetchNothing(this.url('/sink'), {
+        method: 'POST',
+        body: stream,
+      })
+      await once(this.server.events, 'request-received')
+      stream.destroy()
+      await expect(promise).to.be.rejectedWith(AbortError)
+      await wait(80)
       await expectRequestAborted(this.server.lastReq)
     })
 
@@ -212,6 +247,7 @@ describe('fetch-utils', function () {
       const stream = Readable.from(infiniteIterator())
       await expect(
         fetchNothing(this.url('/hang'), {
+          method: 'POST',
           body: stream,
           signal: AbortSignal.timeout(10),
         })
@@ -331,7 +367,14 @@ async function* infiniteIterator() {
 
 async function expectRequestAborted(req) {
   if (!req.destroyed) {
+    try {
       await once(req, 'close')
+    } catch (err) {
+      // `once` throws if req emits an 'error' event
+      // For example, with `Error: aborted` when the request is aborted.
+    }
     expect(req.destroyed).to.be.true
   }
 }
+
+const wait = ms => new Promise(resolve => setTimeout(resolve, ms))

libraries/fetch-utils/test/unit/helpers/TestServer.js

@@ -1,5 +1,6 @@
 const express = require('express')
 const bodyParser = require('body-parser')
+const { EventEmitter } = require('events')
 const http = require('http')
 const https = require('https')
 const { promisify } = require('util')
@@ -7,6 +8,7 @@ const { promisify } = require('util')
 class TestServer {
   constructor() {
     this.app = express()
+    this.events = new EventEmitter()
 
     this.app.use(bodyParser.json())
     this.app.use((req, res, next) => {
@@ -38,6 +40,7 @@ class TestServer {
     })
 
     this.app.post('/sink', (req, res) => {
+      this.events.emit('request-received')
       req.on('data', () => {})
       req.on('end', () => {
         res.status(204).end()
@@ -75,6 +78,7 @@ class TestServer {
 
     // Never returns
 
+    this.app.get('/hang', (req, res) => {})
     this.app.post('/hang', (req, res) => {})
 
     // Redirect
@@ -117,6 +121,8 @@ class TestServer {
   stop() {
     const stopHttp = promisify(this.server.close).bind(this.server)
     const stopHttps = promisify(this.https_server.close).bind(this.https_server)
+    this.server.closeAllConnections()
+    this.https_server.closeAllConnections()
     return Promise.all([stopHttp(), stopHttps()])
   }
 }