Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests for citgm #4221

Merged
merged 5 commits into from
Jan 22, 2021
Merged

Fix tests for citgm #4221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4bf907
Fix missing lsof on node v14 for citgm tests
devinivy Jan 19, 2021
7211b86
Attempt to fix flaky tests on macos in CI. Should also address AIX fo…
devinivy Jan 19, 2021
54aefda
Skip tests without lsof rather than allow them to pass
devinivy Jan 20, 2021
689adf9
Tighten-up flaky tests for citgm based on review
devinivy Jan 20, 2021
b1a649c
More test tightening from review of fixes for citgm.
devinivy Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions test/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
10 changes: 2 additions & 8 deletions test/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 = {};

Expand Down Expand Up @@ -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);
Expand All @@ -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, { errno: 'ENOENT' });
});

cmd.stdout.on('data', (buffer) => {

lsof += buffer.toString();
Expand Down
115 changes: 69 additions & 46 deletions test/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -275,45 +276,57 @@ 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) => {

let testComplete = false;

const onCleanup = [];
flags.onCleanup = async () => {

testComplete = true;

for (const cleanup of onCleanup) {
await cleanup();
}
};

const server = Hapi.server();
const team = new Teamwork.Team();
const team2 = new Teamwork.Team();
const leaveHandlerTeam = new Teamwork.Team();

let rounds = 0;
kanongil marked this conversation as resolved.
Show resolved Hide resolved
server.route({
method: 'GET',
path: '/',
options: {
handler: async (request, h) => {

team2.attend();
for (let i = 0; i < 100; ++i) {
++rounds;
await Hoek.wait(10);
req.abort();

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(() => 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 team2.work;
req.abort();
await server.stop();

await team.work;
expect(rounds).to.be.below(10);
const note = await leaveHandlerTeam.work;

expect(note).to.equal({
active: false,
testComplete: false
});
});
});

Expand Down Expand Up @@ -847,10 +860,10 @@ describe('Request', () => {

describe('_postCycle()', () => {

it('skips onPreResponse when validation terminates request', async () => {
it('skips onPreResponse when validation terminates request', { retry: true }, async (flags) => {
kanongil marked this conversation as resolved.
Show resolved Hide resolved

const server = Hapi.server();
const team = new Teamwork.Team();
const abortedReqTeam = new Teamwork.Team();

let called = false;
server.ext('onPreResponse', (request, h) => {
Expand All @@ -863,28 +876,38 @@ 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();
devinivy marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
});

await server.start();
flags.onCleanup = () => 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 server.stop();
await abortedReqTeam.work;

await server.events.once('response');

expect(called).to.be.false();
});
Expand Down Expand Up @@ -2279,41 +2302,41 @@ 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(() => server.stop());

const timer = new Hoek.Bench();
const options = {
hostname: '127.0.0.1',
hostname: 'localhost',
port: server.info.port,
path: '/timeout',
method: 'POST'
};

await new Promise(async (resolve) => {
const req = Http.request(options);
onCleanup.unshift(() => req.destroy());

const req = Http.request(options, (res) => {
req.write('\n');

expect([503, 408]).to.contain(res.statusCode);
expect(timer.elapsed()).to.be.at.least(80);
resolve();
});
const [res] = await Events.once(req, 'response');

req.on('error', (err) => {
expect([503, 408]).to.contain(res.statusCode);
expect(timer.elapsed()).to.be.at.least(80);

expect(err).to.not.exist();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some (flaky) cases this error was being triggered late and causing the next test to fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, lab does not bind the expect to a specific test, so it can only assume that errors are caused by the current test.

});

req.write('\n');
await Hoek.wait(200);
req.end();
});

await server.stop({ timeout: 1 });
await Events.once(req, 'close'); // Ensures that req closes without error
});
});

Expand Down
18 changes: 3 additions & 15 deletions test/transmit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 = {};

Expand Down Expand Up @@ -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);
Expand All @@ -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, { errno: 'ENOENT' });
});

cmd.stdout.on('data', (buffer) => {

lsof += buffer.toString();
Expand All @@ -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') });
Expand All @@ -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, { errno: 'ENOENT' });
});

cmd.stdout.on('data', (buffer) => {

lsof += buffer.toString();
Expand Down