From e0bbc2662301e0d6c9a0cd601c22f7f27e1f56b2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 27 Mar 2023 23:17:55 -0400 Subject: [PATCH] Improve tests that deal with microtasks (#26493) I rewrote some of our tests that deal with microtasks with the aim of making them less coupled to implementation details. This is related to an upcoming change to move update processing into a microtask. --- .../ReactInternalTestUtils.js | 24 ++++++++++ .../ReactDOMSafariMicrotaskBug-test.js | 46 ++++++++++++++----- .../__tests__/ChangeEventPlugin-test.js | 5 +- .../ReactFlushSyncNoAggregateError-test.js | 30 ++++++++++++ 4 files changed, 92 insertions(+), 13 deletions(-) diff --git a/packages/internal-test-utils/ReactInternalTestUtils.js b/packages/internal-test-utils/ReactInternalTestUtils.js index 2b7010a06ffeb..3e9a60392d0a6 100644 --- a/packages/internal-test-utils/ReactInternalTestUtils.js +++ b/packages/internal-test-utils/ReactInternalTestUtils.js @@ -218,6 +218,30 @@ ${diff(expectedLog, actualLog)} throw error; } +export async function waitForDiscrete(expectedLog) { + assertYieldsWereCleared(SchedulerMock); + + // Create the error object before doing any async work, to get a better + // stack trace. + const error = new Error(); + Error.captureStackTrace(error, waitForDiscrete); + + // Wait until end of current task/microtask. + await waitForMicrotasks(); + + const actualLog = SchedulerMock.unstable_clearLog(); + if (equals(actualLog, expectedLog)) { + return; + } + + error.message = ` +Expected sequence of events did not occur. + +${diff(expectedLog, actualLog)} +`; + throw error; +} + export function assertLog(expectedLog) { const actualLog = SchedulerMock.unstable_clearLog(); if (equals(actualLog, expectedLog)) { diff --git a/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js b/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js index f4edb90e1fb4a..17b1aed89ce72 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js @@ -13,25 +13,33 @@ let React; let ReactDOMClient; let act; +let assertLog; +let Scheduler; describe('ReactDOMSafariMicrotaskBug-test', () => { let container; - let flushMicrotasksPrematurely; + let overrideQueueMicrotask; + let flushFakeMicrotasks; beforeEach(() => { // In Safari, microtasks don't always run on clean stack. // This setup crudely approximates it. // In reality, the sync flush happens when an iframe is added to the page. // https://github.com/facebook/react/issues/22459 - let queue = []; - window.queueMicrotask = function (cb) { - queue.push(cb); + const originalQueueMicrotask = queueMicrotask; + overrideQueueMicrotask = false; + const fakeMicrotaskQueue = []; + global.queueMicrotask = cb => { + if (overrideQueueMicrotask) { + fakeMicrotaskQueue.push(cb); + } else { + originalQueueMicrotask(cb); + } }; - flushMicrotasksPrematurely = function () { - while (queue.length > 0) { - const prevQueue = queue; - queue = []; - prevQueue.forEach(cb => cb()); + flushFakeMicrotasks = () => { + while (fakeMicrotaskQueue.length > 0) { + const cb = fakeMicrotaskQueue.shift(); + cb(); } }; @@ -40,6 +48,8 @@ describe('ReactDOMSafariMicrotaskBug-test', () => { React = require('react'); ReactDOMClient = require('react-dom/client'); act = require('internal-test-utils').act; + assertLog = require('internal-test-utils').assertLog; + Scheduler = require('scheduler'); document.body.appendChild(container); }); @@ -55,10 +65,14 @@ describe('ReactDOMSafariMicrotaskBug-test', () => { return (
{ + overrideQueueMicrotask = true; if (!ran) { ran = true; setState(1); - flushMicrotasksPrematurely(); + flushFakeMicrotasks(); + Scheduler.log( + 'Content at end of ref callback: ' + container.textContent, + ); } }}> {state} @@ -69,6 +83,7 @@ describe('ReactDOMSafariMicrotaskBug-test', () => { await act(() => { root.render(); }); + assertLog(['Content at end of ref callback: 0']); expect(container.textContent).toBe('1'); }); @@ -78,8 +93,12 @@ describe('ReactDOMSafariMicrotaskBug-test', () => { return ( @@ -95,6 +114,11 @@ describe('ReactDOMSafariMicrotaskBug-test', () => { new MouseEvent('click', {bubbles: true}), ); }); + // This causes the update to flush earlier than usual. This isn't the ideal + // behavior but we use this test to document it. The bug is Safari's, not + // ours, so we just do our best to not crash even though the behavior isn't + // completely correct. + assertLog(['Content at end of click handler: 1']); expect(container.textContent).toBe('1'); }); }); diff --git a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js index ccc69785e6bea..104aed13d761f 100644 --- a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js +++ b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js @@ -16,6 +16,7 @@ let ReactFeatureFlags; let Scheduler; let act; let waitForAll; +let waitForDiscrete; let assertLog; const setUntrackedChecked = Object.getOwnPropertyDescriptor( @@ -65,6 +66,7 @@ describe('ChangeEventPlugin', () => { const InternalTestUtils = require('internal-test-utils'); waitForAll = InternalTestUtils.waitForAll; + waitForDiscrete = InternalTestUtils.waitForDiscrete; assertLog = InternalTestUtils.assertLog; container = document.createElement('div'); @@ -730,8 +732,7 @@ describe('ChangeEventPlugin', () => { ); // Flush microtask queue. - await null; - assertLog(['render: ']); + await waitForDiscrete(['render: ']); expect(input.value).toBe(''); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js index 6cef94bbd35f5..129b79f743144 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js @@ -5,6 +5,9 @@ let act; let assertLog; let waitForThrow; +let overrideQueueMicrotask; +let flushFakeMicrotasks; + // TODO: Migrate tests to React DOM instead of React Noop describe('ReactFlushSync (AggregateError not available)', () => { @@ -13,6 +16,26 @@ describe('ReactFlushSync (AggregateError not available)', () => { global.AggregateError = undefined; + // When AggregateError is not available, the errors are rethrown in a + // microtask. This is an implementation detail but we want to test it here + // so override the global one. + const originalQueueMicrotask = queueMicrotask; + overrideQueueMicrotask = false; + const fakeMicrotaskQueue = []; + global.queueMicrotask = cb => { + if (overrideQueueMicrotask) { + fakeMicrotaskQueue.push(cb); + } else { + originalQueueMicrotask(cb); + } + }; + flushFakeMicrotasks = () => { + while (fakeMicrotaskQueue.length > 0) { + const cb = fakeMicrotaskQueue.shift(); + cb(); + } + }; + React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); @@ -47,6 +70,8 @@ describe('ReactFlushSync (AggregateError not available)', () => { const aahh = new Error('AAHH!'); const nooo = new Error('Noooooooooo!'); + // Override the global queueMicrotask so we can test the behavior. + overrideQueueMicrotask = true; let error; try { ReactNoop.flushSync(() => { @@ -70,10 +95,15 @@ describe('ReactFlushSync (AggregateError not available)', () => { // AggregateError is not available, React throws the first error, then // throws the remaining errors in separate tasks. expect(error).toBe(aahh); + // TODO: Currently the remaining error is rethrown in an Immediate Scheduler // task, but this may change to a timer or microtask in the future. The // exact mechanism is an implementation detail; they just need to be logged // in the order the occurred. + + // This will start throwing if we change it to rethrow in a microtask. + flushFakeMicrotasks(); + await waitForThrow(nooo); }); });