From 5afa89298151c0382cd83979a822f6e159469f52 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Wed, 4 Sep 2024 15:30:16 +0200 Subject: [PATCH] [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 --- .../fetch-utils/test/unit/FetchUtilsTests.js | 51 +++++++++++++++++-- .../test/unit/helpers/TestServer.js | 6 +++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/libraries/fetch-utils/test/unit/FetchUtilsTests.js b/libraries/fetch-utils/test/unit/FetchUtilsTests.js index 0c04afed19..f3aca4a412 100644 --- a/libraries/fetch-utils/test/unit/FetchUtilsTests.js +++ b/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) { - await once(req, 'close') + 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)) diff --git a/libraries/fetch-utils/test/unit/helpers/TestServer.js b/libraries/fetch-utils/test/unit/helpers/TestServer.js index 57888ea297..56dc94b566 100644 --- a/libraries/fetch-utils/test/unit/helpers/TestServer.js +++ b/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()]) } }