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.

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

Increase coverage #542

Merged
merged 10 commits into from
Aug 3, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
6 changes: 3 additions & 3 deletions source/normalize-arguments.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,10 @@ module.exports = (url, options, defaults) => {

if (Reflect.has(error, 'headers') && Reflect.has(error.headers, 'retry-after') && retryAfterStatusCodes.has(error.statusCode)) {
let after = Number(error.headers['retry-after']);
if (is.number(after)) {
after *= 1000;
if (is.nan(after)) {
after = Date.parse(error.headers['retry-after']) - Date.now();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes a bug: at first I thought is.number won't return true for NaN, but I was wrong.

Math.max is not needed BTW.

} else {
after = Math.max(Date.parse(error.headers['retry-after']) - Date.now(), 0);
after *= 1000;
}

if (after > options.gotRetry.maxRetryAfter) {
Expand Down
10 changes: 2 additions & 8 deletions source/request-as-event-emitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const {GotError, CacheError, UnsupportedProtocolError, MaxRedirectsError, Reques
const getMethodRedirectCodes = new Set([300, 301, 302, 303, 304, 305, 307, 308]);
const allMethodRedirectCodes = new Set([300, 303, 307, 308]);

module.exports = (options = {}) => {
module.exports = options => {
const emitter = new EventEmitter();
const requestUrl = options.href || (new URLGlobal(options.path, urlLib.format(options))).toString();
const redirects = [];
Expand Down Expand Up @@ -142,13 +142,7 @@ module.exports = (options = {}) => {

if (backoff) {
retryCount++;
setTimeout(options => {
try {
get(options);
} catch (error2) {
emitter.emit('error', error2);
}
}, backoff, options);
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get() does not throw any errors directly. It uses emitter.emit('error', error);

Copy link
Owner

Choose a reason for hiding this comment

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

Good point. I think we need a try/catch here though: https://github.com/sindresorhus/got/pull/542/files#diff-62bdc57f6f22ae58f495daef16f21f8bR141 In case the options.gotRetry.retries function throws.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see I've added it here. It's no longer needed because decodeURI error is catched here. If it wasn't there then we would need to catch on setTimeout too. But there's no need as metioned :)

I'll deny my answer:

It's caught here, but not here and not here.

It was caught nowhere. I don't know how I did get that 😕

To sum up: the removal is good, I'm sure 10000% :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope my answer is no confusing :P

Copy link
Owner

@sindresorhus sindresorhus Aug 2, 2018

Choose a reason for hiding this comment

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

Your answer is not confusing, but you're not answering my question. To be clear, I think we need to try/catch this line because it could throw: const backoff = options.gotRetry.retries(++retryTries, error);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're not answering my question.

Sorry, if you're talking about the second one then it hadn't popped up when I was writing the answer. Pardon my confusion.

I think we need to try/catch this line because it could throw

Good point! Indeed. 🙌

Copy link
Owner

Choose a reason for hiding this comment

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

I think we need to try/catch this line because it could throw
Good point! Indeed. 🙌

And of course a test ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! I hope it satisfies you :)

setTimeout(get, backoff, options);
cb(true);
return;
}
Expand Down
11 changes: 11 additions & 0 deletions test/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ test.before('setup', async () => {
response.end('body');
});

s.on('/no-status-message', (request, response) => {
response.writeHead(400, '');
response.end('body');
});

s.on('/body', async (request, response) => {
const body = await getStream(request);
response.end(body);
Expand Down Expand Up @@ -102,6 +107,12 @@ test('custom status message', async t => {
t.is(error.statusMessage, 'Something Exploded');
});

test('no status message is overriden by the default one', async t => {
const error = await t.throws(got(`${s.url}/no-status-message`));
t.is(error.statusCode, 400);
t.is(error.statusMessage, http.STATUS_CODES[400]);
});

test.serial('http.request error', async t => {
const stub = sinon.stub(http, 'request').callsFake(() => {
throw new TypeError('The header content contains invalid characters');
Expand Down
40 changes: 40 additions & 0 deletions test/retry.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ let trys = 0;
let knocks = 0;
let fifth = 0;
let lastTried413access = Date.now();
let lastTried413TimestampAccess;

const retryAfterOn413 = 2;
const socketTimeout = 200;
Expand Down Expand Up @@ -54,6 +55,16 @@ test.before('setup', async () => {
response.end();
});

s.on('/413withTimestamp', (request, response) => {
const date = (new Date(Date.now() + (retryAfterOn413 * 1000))).toUTCString();

response.writeHead(413, {
'Retry-After': date
});
response.end(lastTried413TimestampAccess);
lastTried413TimestampAccess = date;
});

s.on('/413withoutRetryAfter', (request, response) => {
response.statusCode = 413;
response.end();
Expand Down Expand Up @@ -149,6 +160,15 @@ test('respect 413 Retry-After', async t => {
t.true(Number(body) >= retryAfterOn413 * 1000);
});

test('respect 413 Retry-After with RFC-1123 timestamp', async t => {
const {statusCode, body} = await got(`${s.url}/413withTimestamp`, {
throwHttpErrors: false,
retry: 1
});
t.is(statusCode, 413);
t.true(Date.now() >= Date.parse(body));
});

test('doesn\'t retry on 413 with empty statusCodes and methods', async t => {
const {statusCode, retryCount} = await got(`${s.url}/413`, {
throwHttpErrors: false,
Expand Down Expand Up @@ -207,3 +227,23 @@ test('doesn\'t retry if Retry-After header is greater than maxRetryAfter', async
});
t.is(retryCount, 0);
});

test('doesn\'t retry when set to false', async t => {
const {statusCode, retryCount} = await got(`${s.url}/413`, {
throwHttpErrors: false,
retry: false
});
t.is(statusCode, 413);
t.is(retryCount, 0);
});

test('works when defaults.options.retry is not an object', async t => {
const instance = got.extend({
retry: 2
});

const {retryCount} = await instance(`${s.url}/413`, {
throwHttpErrors: false
});
t.is(retryCount, 0);
});
38 changes: 36 additions & 2 deletions test/stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import test from 'ava';
import toReadableStream from 'to-readable-stream';
import getStream from 'get-stream';
import pEvent from 'p-event';
import delay from 'delay';
import is from '@sindresorhus/is';
import got from '../source';
import {createServer} from './helpers/server';
Expand All @@ -13,9 +14,10 @@ test.before('setup', async () => {

s.on('/', (request, response) => {
response.writeHead(200, {
unicorn: 'rainbow'
unicorn: 'rainbow',
'content-encoding': 'gzip'
});
response.end('ok');
response.end(Buffer.from('H4sIAAAAAAAA/8vPBgBH3dx5AgAAAA==', 'base64')); // 'ok'
});

s.on('/post', (request, response) => {
Expand Down Expand Up @@ -129,6 +131,38 @@ test('proxying headers works', async t => {

const {headers} = await got(server.url);
t.is(headers.unicorn, 'rainbow');
t.is(headers['content-encoding'], undefined);

await server.close();
});

test('skips proxying headers after server has sent them already', async t => {
const server = await createServer();

server.on('/', (request, response) => {
response.writeHead(200);
got.stream(s.url).pipe(response);
});

await server.listen(server.port);

const {headers} = await got(server.url);
t.is(headers.unicorn, undefined);

await server.close();
});

test('throws when trying to proxy through a closed stream', async t => {
const server = await createServer();

server.on('/', async (request, response) => {
const stream = got.stream(s.url);
await delay(1000);
t.throws(() => stream.pipe(response));
response.end();
});

await server.listen(server.port);
await got(server.url);
await server.close();
});
4 changes: 4 additions & 0 deletions test/unix-socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,8 @@ if (process.platform !== 'win32') {
const url = format('unix:%s:%s', socketPath, '/foo:bar');
t.is((await got(url)).body, 'ok');
});

test('throws on invalid URL', async t => {
await t.throws(got('unix:'));
});
}