Skip to content

Commit

Permalink
Get rid of 500ms timeout
Browse files Browse the repository at this point in the history
Replaces the hardcoded FIXME sleep(500ms) timeout in each test.

Instead, we track any pending `PerformanceObserver`s and wait for
them to disconnect before continuing.

This doesn't magically make the tests run faster, in a way it just
hides the timeout, because the implementation actually still waits
for `OBSERVER_WAIT_TIME_MS` (300ms) internally before disconnecting
the PO.

However, this makes the tests follow the actual real world behavior
and they are only as slow as the actual behavior IRL.

IMO, the timeout isn't actually necessary IF we have PO available,
and that can be removed in a future refactor. When we address that,
most of the tests will automatically run faster.
  • Loading branch information
chancancode committed Jan 15, 2025
1 parent 9f8149c commit c34f0ee
Showing 1 changed file with 113 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ import {
import * as msw from 'msw';
import { setupWorker } from 'msw/browser';

// This should match the unexported constant with the same name in fetch.ts
const OBSERVER_WAIT_TIME_MS = 300;

class DummySpanExporter implements tracing.SpanExporter {
readonly exported: tracing.ReadableSpan[][] = [];

Expand Down Expand Up @@ -91,6 +94,34 @@ const ORIGIN = location.origin;
// "localhost:9876"
const ORIGIN_HOST = new URL(ORIGIN).host;

interface Resolvers<T> {
promise: Promise<T>;
resolve: (value: T) => void;
reject: (reason: any) => void;
}

// Use Promise.withResolvers when we can
function withResolvers<T>(): Resolvers<T> {
let resolve: (value: T) => void;
let reject: (reason: any) => void;
const promise = new Promise<T>((res, rej) => {
resolve = res;
reject = rej;
});

return {
promise,
resolve: resolve!,
reject: reject!,
};
}

function waitFor(timeout: number): Promise<void> {
return new Promise(resolve => {
setTimeout(resolve, timeout);
});
}

describe('fetch', () => {
let workerStarted = false;

Expand All @@ -105,13 +136,74 @@ describe('fetch', () => {
workerStarted = true;
};

afterEach(() => {
if (workerStarted) {
worker.stop();
workerStarted = false;
let pendingObservers = 0;
let waitForPerformanceObservers = async () => {};

beforeEach(() => {
if (PerformanceObserver) {
assert.strictEqual(
pendingObservers,
0,
'Did a previous test leak a PerformanceObserver?'
);

let resolvers: Resolvers<void> | undefined;

const _observe = PerformanceObserver.prototype.observe;
const _disconnect = PerformanceObserver.prototype.disconnect;

sinon.stub(PerformanceObserver.prototype, 'observe').callsFake(function (
this: PerformanceObserver,
...args
) {
_observe.call(this, ...args);
pendingObservers++;

if (!resolvers) {
resolvers = withResolvers();
}
});

sinon
.stub(PerformanceObserver.prototype, 'disconnect')
.callsFake(function (this: PerformanceObserver, ...args) {
_disconnect.call(this, ...args);
pendingObservers--;

if (pendingObservers === 0) {
resolvers?.resolve();
resolvers = undefined;
}
});

waitForPerformanceObservers = async (): Promise<void> => {
while (resolvers) {
await resolvers.promise;
}
};
}
});

sinon.restore();
afterEach(() => {
try {
if (workerStarted) {
worker.stop();
workerStarted = false;
}

const _pendingObservers = pendingObservers;

pendingObservers = 0;
waitForPerformanceObservers = async () => {};

assert.strictEqual(
_pendingObservers,
0,
`Test leaked ${_pendingObservers} \`PerformanceObserver\`(s)!`
);
} finally {
sinon.restore();
}
});

describe('enabling/disabling', () => {
Expand Down Expand Up @@ -178,10 +270,7 @@ describe('fetch', () => {
callback
);

// FIXME!
await new Promise(resolve => {
setTimeout(resolve, 500);
});
await waitForPerformanceObservers();

if (expectExport) {
// This isn't intended to be an invariant, but in the current setup we
Expand Down Expand Up @@ -1570,6 +1659,21 @@ describe('fetch', () => {
describe('when `PerformanceObserver` is NOT available', () => {
beforeEach(async () => {
sinon.stub(window, 'PerformanceObserver').value(undefined);

// This seemingly random timeout is testing real behavior!
//
// Currently, the implementation works by waiting a hardcoded
// OBSERVER_WAIT_TIME_MS before trying to get the resource
// timing entries, and hoping that they are there by then.
//
// We will match that here plus an additional 50ms. If the
// tests still fail despite this timeout, then we may have
// found a bug that could occur in the real world, and it's
// probably time to revisit the naïve implementation.
//
// This should be updated as the implementation changes.
waitForPerformanceObservers = () =>
waitFor(OBSERVER_WAIT_TIME_MS + 50);
});

// The assertions are essentially the same as the tests from above, but
Expand Down

0 comments on commit c34f0ee

Please sign in to comment.