From b4bf907c502bf750d66bf767aa6bfbcbb551977f Mon Sep 17 00:00:00 2001 From: Devin Ivy Date: Mon, 18 Jan 2021 21:59:34 -0500 Subject: [PATCH 1/5] Fix missing lsof on node v14 for citgm tests --- test/core.js | 2 +- test/transmit.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/core.js b/test/core.js index fe4cc748e..aa9266b25 100755 --- a/test/core.js +++ b/test/core.js @@ -1705,7 +1705,7 @@ describe('Core', () => { cmd.on('error', (err) => { // Allow the test to pass on platforms with no lsof - Bounce.ignore(err, { errno: 'ENOENT' }); + Bounce.ignore(err, { code: 'ENOENT' }); }); cmd.stdout.on('data', (buffer) => { diff --git a/test/transmit.js b/test/transmit.js index 63b269e11..f2bfb677b 100755 --- a/test/transmit.js +++ b/test/transmit.js @@ -104,7 +104,7 @@ describe('transmission', () => { cmd.on('error', (err) => { // Allow the test to pass on platforms with no lsof - Bounce.ignore(err, { errno: 'ENOENT' }); + Bounce.ignore(err, { code: 'ENOENT' }); }); cmd.stdout.on('data', (buffer) => { @@ -145,7 +145,7 @@ describe('transmission', () => { cmd.on('error', (err) => { // Allow the test to pass on platforms with no lsof - Bounce.ignore(err, { errno: 'ENOENT' }); + Bounce.ignore(err, { code: 'ENOENT' }); }); cmd.stdout.on('data', (buffer) => { From 7211b86e3f3a73d5e4cf6604c918bf64587e1383 Mon Sep 17 00:00:00 2001 From: Devin Ivy Date: Mon, 18 Jan 2021 23:47:51 -0500 Subject: [PATCH 2/5] Attempt to fix flaky tests on macos in CI. Should also address AIX for citgm. --- test/request.js | 118 +++++++++++++++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 42 deletions(-) diff --git a/test/request.js b/test/request.js index adf748a48..8deaf7ff3 100755 --- a/test/request.js +++ b/test/request.js @@ -4,6 +4,7 @@ const Http = require('http'); const Net = require('net'); const Stream = require('stream'); const Url = require('url'); +const Events = require('events'); const Boom = require('@hapi/boom'); const Code = require('@hapi/code'); @@ -275,45 +276,64 @@ describe('Request', () => { describe('active()', () => { - it('exits handler early when request is no longer active', async () => { + it('exits handler early when request is no longer active', { retry: true }, async (flags) => { + + const onCleanup = []; + flags.onCleanup = async () => { + + for (const cleanup of onCleanup) { + await cleanup(); + } + }; + + let testComplete = false; + onCleanup.unshift(() => { + + testComplete = true; + }); const server = Hapi.server(); - const team = new Teamwork.Team(); - const team2 = new Teamwork.Team(); + const enterHandlerTeam = new Teamwork.Team(); + const leaveHandlerTeam = new Teamwork.Team(); - let rounds = 0; server.route({ method: 'GET', path: '/', options: { handler: async (request, h) => { - team2.attend(); - for (let i = 0; i < 100; ++i) { - ++rounds; - await Hoek.wait(10); + enterHandlerTeam.attend(); - if (!request.active()) { - break; - } + while (request.active() && !testComplete) { + await Hoek.wait(10); } - team.attend(); + leaveHandlerTeam.attend({ + active: request.active(), + testComplete + }); + return null; } } }); await server.start(); + onCleanup.unshift(async () => await server.stop()); + + const req = Http.get(server.info.uri, Hoek.ignore); - const req = Http.get(server.info.uri, (res) => { }); req.on('error', Hoek.ignore); - await team2.work; + onCleanup.unshift(() => req.off('error', Hoek.ignore)); + + await enterHandlerTeam.work; req.abort(); - await server.stop(); + const note = await leaveHandlerTeam.work; - await team.work; - expect(rounds).to.be.below(10); + expect(note).to.equal({ + active: false, + testComplete: false + }); }); }); @@ -847,10 +867,10 @@ describe('Request', () => { describe('_postCycle()', () => { - it('skips onPreResponse when validation terminates request', async () => { + it('skips onPreResponse when validation terminates request', { retry: true }, async (flags) => { const server = Hapi.server(); - const team = new Teamwork.Team(); + const abortedReqTeam = new Teamwork.Team(); let called = false; server.ext('onPreResponse', (request, h) => { @@ -863,14 +883,23 @@ describe('Request', () => { method: 'GET', path: '/', options: { - handler: () => null, + handler: (request) => { + + // Stash raw so that we can access it on response validation + Object.assign(request.app, request.raw); + + return null; + }, response: { status: { - 200: async () => { + 200: async (_, { context }) => { req.abort(); - await Hoek.wait(10); - team.attend(); + + const raw = context.app.request; + await Events.once(raw.req, 'aborted'); + + abortedReqTeam.attend(); } } } @@ -878,14 +907,19 @@ describe('Request', () => { }); await server.start(); + flags.onCleanup = async () => await server.stop(); - const req = Http.get(server.info.uri, (res) => { }); + const req = Http.get(server.info.uri, Hoek.ignore); req.on('error', Hoek.ignore); - await team.work; - await Hoek.wait(100); + await abortedReqTeam.work; + + // Drain pending requests await server.stop(); + // In case server is still running lifecycle after abort and drain + await Hoek.wait(100); + expect(called).to.be.false(); }); @@ -2279,12 +2313,21 @@ describe('Request', () => { expect(res.statusCode).to.equal(200); }); - it('handles race condition between equal client and server timeouts', async () => { + it('handles race condition between equal client and server timeouts', async (flags) => { + + const onCleanup = []; + flags.onCleanup = async () => { + + for (const cleanup of onCleanup) { + await cleanup(); + } + }; const server = Hapi.server({ routes: { timeout: { server: 100 }, payload: { timeout: 100 } } }); server.route({ method: 'POST', path: '/timeout', options: { handler: Hoek.block } }); await server.start(); + onCleanup.unshift(async () => await server.stop({ timeout: 1 })); const timer = new Hoek.Bench(); const options = { @@ -2294,26 +2337,17 @@ describe('Request', () => { method: 'POST' }; - await new Promise(async (resolve) => { - - const req = Http.request(options, (res) => { - - expect([503, 408]).to.contain(res.statusCode); - expect(timer.elapsed()).to.be.at.least(80); - resolve(); - }); + const res = await new Promise((resolve, reject) => { - req.on('error', (err) => { - - expect(err).to.not.exist(); - }); + const req = Http.request(options, resolve); + req.once('error', reject); req.write('\n'); - await Hoek.wait(200); - req.end(); + onCleanup.unshift(() => req.end()); }); - await server.stop({ timeout: 1 }); + expect([503, 408]).to.contain(res.statusCode); + expect(timer.elapsed()).to.be.at.least(80); }); }); From 54aefda8bfa8e50d39f04c8dd7ed528084fb359c Mon Sep 17 00:00:00 2001 From: Devin Ivy Date: Tue, 19 Jan 2021 22:47:56 -0500 Subject: [PATCH 3/5] Skip tests without lsof rather than allow them to pass --- test/common.js | 19 +++++++++++++++++++ test/core.js | 10 ++-------- test/transmit.js | 18 +++--------------- 3 files changed, 24 insertions(+), 23 deletions(-) create mode 100644 test/common.js diff --git a/test/common.js b/test/common.js new file mode 100644 index 000000000..067c3022d --- /dev/null +++ b/test/common.js @@ -0,0 +1,19 @@ +'use strict'; + +const ChildProcess = require('child_process'); + +const internals = {}; + +internals.hasLsof = () => { + + try { + ChildProcess.execSync(`lsof -p ${process.pid}`, { stdio: 'ignore' }); + } + catch (err) { + return false; + } + + return true; +}; + +exports.hasLsof = internals.hasLsof(); diff --git a/test/core.js b/test/core.js index aa9266b25..195fb515e 100755 --- a/test/core.js +++ b/test/core.js @@ -11,7 +11,6 @@ const Stream = require('stream'); const TLS = require('tls'); const Boom = require('@hapi/boom'); -const Bounce = require('@hapi/bounce'); const CatboxMemory = require('@hapi/catbox-memory'); const Code = require('@hapi/code'); const Handlebars = require('handlebars'); @@ -22,6 +21,7 @@ const Lab = require('@hapi/lab'); const Vision = require('@hapi/vision'); const Wreck = require('@hapi/wreck'); +const Common = require('./common'); const internals = {}; @@ -1679,7 +1679,7 @@ describe('Core', () => { expect(res.result.isBoom).to.equal(true); }); - it('cleans unused file stream when response is overridden', { skip: process.platform === 'win32' }, async () => { + it('cleans unused file stream when response is overridden', { skip: !Common.hasLsof }, async () => { const server = Hapi.server(); await server.register(Inert); @@ -1702,12 +1702,6 @@ describe('Core', () => { const cmd = ChildProcess.spawn('lsof', ['-p', process.pid]); let lsof = ''; - cmd.on('error', (err) => { - - // Allow the test to pass on platforms with no lsof - Bounce.ignore(err, { code: 'ENOENT' }); - }); - cmd.stdout.on('data', (buffer) => { lsof += buffer.toString(); diff --git a/test/transmit.js b/test/transmit.js index f2bfb677b..d77cc691e 100755 --- a/test/transmit.js +++ b/test/transmit.js @@ -8,7 +8,6 @@ const Stream = require('stream'); const Zlib = require('zlib'); const Boom = require('@hapi/boom'); -const Bounce = require('@hapi/bounce'); const Code = require('@hapi/code'); const Hapi = require('..'); const Hoek = require('@hapi/hoek'); @@ -17,6 +16,7 @@ const Lab = require('@hapi/lab'); const Teamwork = require('@hapi/teamwork'); const Wreck = require('@hapi/wreck'); +const Common = require('./common'); const internals = {}; @@ -86,7 +86,7 @@ describe('transmission', () => { expect(res.statusCode).to.equal(200); }); - it('closes file handlers when not reading file stream', { skip: process.platform === 'win32' }, async () => { + it('closes file handlers when not reading file stream', { skip: !Common.hasLsof }, async () => { const server = Hapi.server(); await server.register(Inert); @@ -101,12 +101,6 @@ describe('transmission', () => { const cmd = ChildProcess.spawn('lsof', ['-p', process.pid]); let lsof = ''; - cmd.on('error', (err) => { - - // Allow the test to pass on platforms with no lsof - Bounce.ignore(err, { code: 'ENOENT' }); - }); - cmd.stdout.on('data', (buffer) => { lsof += buffer.toString(); @@ -128,7 +122,7 @@ describe('transmission', () => { }); }); - it('closes file handlers when not using a manually open file stream', { skip: process.platform === 'win32' }, async () => { + it('closes file handlers when not using a manually open file stream', { skip: !Common.hasLsof }, async () => { const server = Hapi.server(); server.route({ method: 'GET', path: '/file', handler: (request, h) => h.response(Fs.createReadStream(__dirname + '/../package.json')).header('etag', 'abc') }); @@ -142,12 +136,6 @@ describe('transmission', () => { const cmd = ChildProcess.spawn('lsof', ['-p', process.pid]); let lsof = ''; - cmd.on('error', (err) => { - - // Allow the test to pass on platforms with no lsof - Bounce.ignore(err, { code: 'ENOENT' }); - }); - cmd.stdout.on('data', (buffer) => { lsof += buffer.toString(); From 689adf92cd5985962ff0c269948e5306b441d506 Mon Sep 17 00:00:00 2001 From: Devin Ivy Date: Wed, 20 Jan 2021 00:30:32 -0500 Subject: [PATCH 4/5] Tighten-up flaky tests for citgm based on review --- test/request.js | 49 ++++++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/test/request.js b/test/request.js index 8deaf7ff3..b7f27ebc0 100755 --- a/test/request.js +++ b/test/request.js @@ -278,22 +278,19 @@ describe('Request', () => { it('exits handler early when request is no longer active', { retry: true }, async (flags) => { + let testComplete = false; + const onCleanup = []; flags.onCleanup = async () => { + testComplete = true; + for (const cleanup of onCleanup) { await cleanup(); } }; - let testComplete = false; - onCleanup.unshift(() => { - - testComplete = true; - }); - const server = Hapi.server(); - const enterHandlerTeam = new Teamwork.Team(); const leaveHandlerTeam = new Teamwork.Team(); server.route({ @@ -302,7 +299,7 @@ describe('Request', () => { options: { handler: async (request, h) => { - enterHandlerTeam.attend(); + req.abort(); while (request.active() && !testComplete) { await Hoek.wait(10); @@ -319,15 +316,11 @@ describe('Request', () => { }); await server.start(); - onCleanup.unshift(async () => await server.stop()); + onCleanup.unshift(() => server.stop()); const req = Http.get(server.info.uri, Hoek.ignore); - req.on('error', Hoek.ignore); - onCleanup.unshift(() => req.off('error', Hoek.ignore)); - await enterHandlerTeam.work; - req.abort(); const note = await leaveHandlerTeam.work; expect(note).to.equal({ @@ -870,7 +863,6 @@ describe('Request', () => { it('skips onPreResponse when validation terminates request', { retry: true }, async (flags) => { const server = Hapi.server(); - const abortedReqTeam = new Teamwork.Team(); let called = false; server.ext('onPreResponse', (request, h) => { @@ -898,8 +890,6 @@ describe('Request', () => { const raw = context.app.request; await Events.once(raw.req, 'aborted'); - - abortedReqTeam.attend(); } } } @@ -907,18 +897,12 @@ describe('Request', () => { }); await server.start(); - flags.onCleanup = async () => await server.stop(); + flags.onCleanup = () => server.stop(); const req = Http.get(server.info.uri, Hoek.ignore); req.on('error', Hoek.ignore); - await abortedReqTeam.work; - - // Drain pending requests - await server.stop(); - - // In case server is still running lifecycle after abort and drain - await Hoek.wait(100); + await server.events.once('response'); expect(called).to.be.false(); }); @@ -2327,25 +2311,28 @@ describe('Request', () => { server.route({ method: 'POST', path: '/timeout', options: { handler: Hoek.block } }); await server.start(); - onCleanup.unshift(async () => await server.stop({ timeout: 1 })); + onCleanup.unshift(() => server.stop()); const timer = new Hoek.Bench(); const options = { - hostname: '127.0.0.1', + hostname: 'localhost', port: server.info.port, path: '/timeout', method: 'POST' }; - const res = await new Promise((resolve, reject) => { + const req = Http.request(options); + onCleanup.unshift(() => req.destroy()); - const req = Http.request(options, resolve); - req.once('error', reject); + req.on('error', (err) => { - req.write('\n'); - onCleanup.unshift(() => req.end()); + expect(err).to.not.exist(); }); + req.write('\n'); + + const [res] = await Events.once(req, 'response'); + expect([503, 408]).to.contain(res.statusCode); expect(timer.elapsed()).to.be.at.least(80); }); From b1a649c342c37df0a2bb0b5c8b0d8a54d7133846 Mon Sep 17 00:00:00 2001 From: Devin Ivy Date: Thu, 21 Jan 2021 23:53:10 -0500 Subject: [PATCH 5/5] More test tightening from review of fixes for citgm. --- test/request.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/request.js b/test/request.js index b7f27ebc0..5e1e61c85 100755 --- a/test/request.js +++ b/test/request.js @@ -863,6 +863,7 @@ describe('Request', () => { it('skips onPreResponse when validation terminates request', { retry: true }, async (flags) => { const server = Hapi.server(); + const abortedReqTeam = new Teamwork.Team(); let called = false; server.ext('onPreResponse', (request, h) => { @@ -890,6 +891,8 @@ describe('Request', () => { const raw = context.app.request; await Events.once(raw.req, 'aborted'); + + abortedReqTeam.attend(); } } } @@ -902,6 +905,8 @@ describe('Request', () => { const req = Http.get(server.info.uri, Hoek.ignore); req.on('error', Hoek.ignore); + await abortedReqTeam.work; + await server.events.once('response'); expect(called).to.be.false(); @@ -2324,17 +2329,14 @@ describe('Request', () => { const req = Http.request(options); onCleanup.unshift(() => req.destroy()); - req.on('error', (err) => { - - expect(err).to.not.exist(); - }); - req.write('\n'); const [res] = await Events.once(req, 'response'); expect([503, 408]).to.contain(res.statusCode); expect(timer.elapsed()).to.be.at.least(80); + + await Events.once(req, 'close'); // Ensures that req closes without error }); });