Skip to content

Commit

Permalink
Improve robustness of request construction (#600)
Browse files Browse the repository at this point in the history
* Improve robustness of request construction

* Fix flakey retry tests
  • Loading branch information
sholladay committed Jun 26, 2024
1 parent b1effd9 commit 974f1e9
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 57 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"node": ">=18"
},
"scripts": {
"test": "xo && npm run build && ava --timeout=10m --serial",
"test": "xo && npm run build && ava",
"debug": "PWDEBUG=1 ava --timeout=2m",
"release": "np",
"build": "del-cli distribution && tsc --project tsconfig.dist.json",
Expand Down
11 changes: 5 additions & 6 deletions source/core/Ky.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ export class Ky {
this._options.duplex = 'half';
}

if (this._options.json !== undefined) {
this._options.body = this._options.stringifyJson?.(this._options.json) ?? JSON.stringify(this._options.json);
this._options.headers.set('content-type', this._options.headers.get('content-type') ?? 'application/json');
}

this.request = new globalThis.Request(this._input, this._options);

if (this._options.searchParams) {
Expand All @@ -201,12 +206,6 @@ export class Ky {
// The spread of `this.request` is required as otherwise it misses the `duplex` option for some reason and throws.
this.request = new globalThis.Request(new globalThis.Request(url, {...this.request}), this._options as RequestInit);
}

if (this._options.json !== undefined) {
this._options.body = this._options.stringifyJson?.(this._options.json) ?? JSON.stringify(this._options.json);
this.request.headers.set('content-type', this._options.headers.get('content-type') ?? 'application/json');
this.request = new globalThis.Request(this.request, {body: this._options.body});
}
}

protected _calculateRetryDelay(error: unknown) {
Expand Down
42 changes: 0 additions & 42 deletions test/helpers/with-performance-observer.ts

This file was deleted.

28 changes: 28 additions & 0 deletions test/helpers/with-performance.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import {performance} from 'node:perf_hooks';
import process from 'node:process';
import type {ExecutionContext} from 'ava';

type Argument = {
expectedDuration: number;
t: ExecutionContext;
test: () => Promise<void>;
};

// We allow the tests to take more time on CI than locally, to reduce flakiness
const allowedOffset = process.env.CI ? 300 : 200;

export async function withPerformance({
expectedDuration,
t,
test,
}: Argument) {
const startTime = performance.now();
await test();
const endTime = performance.now();

const duration = endTime - startTime;
t.true(
Math.abs(duration - expectedDuration) < allowedOffset,
`Duration of ${duration}ms is not close to expected duration ${expectedDuration}ms`,
);
}
14 changes: 6 additions & 8 deletions test/retry.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import test from 'ava';
import ky from '../source/index.js';
import {createHttpTestServer} from './helpers/create-http-test-server.js';
import {withPerformanceObserver} from './helpers/with-performance-observer.js';
import {withPerformance} from './helpers/with-performance.js';

const fixture = 'fixture';
const defaultRetryCount = 2;
Expand Down Expand Up @@ -442,7 +442,7 @@ test('throws when retry.statusCodes is not an array', async t => {
await server.close();
});

test('respect maximum backoff', async t => {
test('respect maximum backoffLimit', async t => {
const retryCount = 4;
let requestCount = 0;

Expand All @@ -457,9 +457,8 @@ test('respect maximum backoff', async t => {
}
});

await withPerformanceObserver({
await withPerformance({
t,
name: 'default',
expectedDuration: 300 + 600 + 1200 + 2400,
async test() {
t.is(await ky(server.url, {
Expand All @@ -469,9 +468,9 @@ test('respect maximum backoff', async t => {
});

requestCount = 0;
await withPerformanceObserver({

await withPerformance({
t,
name: 'custom',
expectedDuration: 300 + 600 + 1000 + 1000,
async test() {
t.is(await ky(server.url, {
Expand Down Expand Up @@ -501,9 +500,8 @@ test('respect custom retry.delay', async t => {
}
});

await withPerformanceObserver({
await withPerformance({
t,
name: 'linear',
expectedDuration: 200 + 300 + 400 + 500,
async test() {
t.is(await ky(server.url, {
Expand Down

0 comments on commit 974f1e9

Please sign in to comment.