Skip to content

Test renderer flushAll method verifies an array of expected yields #13174

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

Merged
bvaughn merged 1 commit into from
Jul 9, 2018
Merged

Test renderer flushAll method verifies an array of expected yields #13174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 39 additions & 36 deletions packages/react-test-renderer/src/ReactTestRendererScheduling.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {Deadline} from 'react-reconciler/src/ReactFiberScheduler';
// Current virtual time
export let nowImplementation = () => 0;
export let scheduledCallback: ((deadline: Deadline) => mixed) | null = null;
export let yieldedValues: Array<mixed> | null = null;
export let yieldedValues: Array<mixed> = [];
Copy link
Copy Markdown
Contributor Author

@bvaughn bvaughn Jul 8, 2018

Choose a reason for hiding this comment

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

The transient null value didn't seem to be buying us anything, and it complicated the logic below– so I ditched it.


export function scheduleDeferredCallback(
callback: (deadline: Deadline) => mixed,
Expand All @@ -31,8 +31,39 @@ export function setNowImplementation(implementation: () => number): void {
nowImplementation = implementation;
}

export function flushAll(): Array<mixed> {
yieldedValues = null;
function verifyExpectedValues(expectedValues: Array<mixed>): void {
for (let i = 0; i < expectedValues.length; i++) {
const expectedValue = `"${(expectedValues[i]: any)}"`;
const yieldedValue =
i < yieldedValues.length ? `"${(yieldedValues[i]: any)}"` : 'nothing';
if (yieldedValue !== expectedValue) {
const error = new Error(
`Flush expected to yield ${(expectedValue: any)}, but ${(yieldedValue: any)} was yielded`,
);
// Attach expected and yielded arrays,
// So the caller could pretty print the diff (if desired).
(error: any).expectedValues = expectedValues;
(error: any).actualValues = yieldedValues;
throw error;
}
}

if (expectedValues.length !== yieldedValues.length) {
Copy link
Copy Markdown
Contributor Author

@bvaughn bvaughn Jul 8, 2018

Choose a reason for hiding this comment

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

Normally I would have put this check before the prior one, but that would change the error message in a lot of cases and I suspect the current message might be more helpful. Thoughts?

Copy link
Copy Markdown
Contributor

@flarnie flarnie Jul 9, 2018

Choose a reason for hiding this comment

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

Agreed, I like throwing the more specific error first.

const error = new Error(
`Flush expected to yield ${expectedValues.length} values, but yielded ${
yieldedValues.length
}`,
);
// Attach expected and yielded arrays,
// So the caller could pretty print the diff (if desired).
Copy link
Copy Markdown
Collaborator

@acdlite acdlite Jul 10, 2018

Choose a reason for hiding this comment

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

Can we please go ahead and implement this for the React repo? @cyan33 You want to do this?

Copy link
Copy Markdown
Contributor

@cyan33 cyan33 Jul 10, 2018

Choose a reason for hiding this comment

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

Of Course 👍

(error: any).expectedValues = expectedValues;
(error: any).actualValues = yieldedValues;
throw error;
}
}

export function flushAll(expectedValues: Array<mixed>): Array<mixed> {
yieldedValues = [];
while (scheduledCallback !== null) {
const cb = scheduledCallback;
scheduledCallback = null;
Expand All @@ -47,25 +78,19 @@ export function flushAll(): Array<mixed> {
didTimeout: false,
});
}
if (yieldedValues === null) {
// Always return an array.
return [];
}
verifyExpectedValues(expectedValues);
return yieldedValues;
}

export function flushThrough(expectedValues: Array<mixed>): Array<mixed> {
let didStop = false;
yieldedValues = null;
yieldedValues = [];
while (scheduledCallback !== null && !didStop) {
const cb = scheduledCallback;
scheduledCallback = null;
cb({
timeRemaining() {
if (
yieldedValues !== null &&
yieldedValues.length >= expectedValues.length
) {
if (yieldedValues.length >= expectedValues.length) {
// We at least as many values as expected. Stop rendering.
didStop = true;
return 0;
Expand All @@ -79,34 +104,12 @@ export function flushThrough(expectedValues: Array<mixed>): Array<mixed> {
didTimeout: false,
});
}
if (yieldedValues === null) {
// Always return an array.
yieldedValues = [];
}
for (let i = 0; i < expectedValues.length; i++) {
const expectedValue = `"${(expectedValues[i]: any)}"`;
const yieldedValue =
i < yieldedValues.length ? `"${(yieldedValues[i]: any)}"` : 'nothing';
if (yieldedValue !== expectedValue) {
const error = new Error(
`flushThrough expected to yield ${(expectedValue: any)}, but ${(yieldedValue: any)} was yielded`,
);
// Attach expected and yielded arrays,
// So the caller could pretty print the diff (if desired).
(error: any).expectedValues = expectedValues;
(error: any).actualValues = yieldedValues;
throw error;
}
}
verifyExpectedValues(expectedValues);
return yieldedValues;
}

export function yieldValue(value: mixed): void {
if (yieldedValues === null) {
yieldedValues = [value];
} else {
yieldedValues.push(value);
}
yieldedValues.push(value);
}

export function withCleanYields(fn: Function) {
Expand Down
50 changes: 42 additions & 8 deletions packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ describe('ReactTestRendererAsync', () => {
expect(renderer.toJSON()).toEqual(null);

// Flush initial mount.
renderer.unstable_flushAll();
renderer.unstable_flushAll([]);
expect(renderer.toJSON()).toEqual('Hi');

// Update
renderer.update(<Foo>Bye</Foo>);
// Not yet updated.
expect(renderer.toJSON()).toEqual('Hi');
// Flush update.
renderer.unstable_flushAll();
renderer.unstable_flushAll([]);
expect(renderer.toJSON()).toEqual('Bye');
});

Expand All @@ -62,11 +62,11 @@ describe('ReactTestRendererAsync', () => {
unstable_isAsync: true,
});

expect(renderer.unstable_flushAll()).toEqual(['A:1', 'B:1', 'C:1']);
renderer.unstable_flushAll(['A:1', 'B:1', 'C:1']);
expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']);

renderer.update(<Parent step={2} />);
expect(renderer.unstable_flushAll()).toEqual(['A:2', 'B:2', 'C:2']);
renderer.unstable_flushAll(['A:2', 'B:2', 'C:2']);
expect(renderer.toJSON()).toEqual(['A:2', 'B:2', 'C:2']);
});

Expand Down Expand Up @@ -97,7 +97,7 @@ describe('ReactTestRendererAsync', () => {
expect(renderer.toJSON()).toEqual(null);

// Flush the remaining work
expect(renderer.unstable_flushAll()).toEqual(['C:1']);
renderer.unstable_flushAll(['C:1']);
expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']);
});

Expand Down Expand Up @@ -159,7 +159,41 @@ describe('ReactTestRendererAsync', () => {
);

expect(() => renderer.unstable_flushThrough(['foo', 'baz'])).toThrow(
'flushThrough expected to yield "baz", but "bar" was yielded',
'Flush expected to yield "baz", but "bar" was yielded',
);
});

it('should error if flushAll params dont match yielded values', () => {
const Yield = ({id}) => {
renderer.unstable_yield(id);
return id;
};

const renderer = ReactTestRenderer.create(
<div>
<Yield id="foo" />
<Yield id="bar" />
<Yield id="baz" />
</div>,
{
unstable_isAsync: true,
},
);

expect(() => renderer.unstable_flushAll([])).toThrow(
'Flush expected to yield 0 values, but yielded 3',
);

renderer.update(
<div>
<Yield id="foo" />
<Yield id="bar" />
<Yield id="baz" />
</div>,
);

expect(() => renderer.unstable_flushAll(['foo', 'baz'])).toThrow(
'Flush expected to yield "baz", but "bar" was yielded',
);
});

Expand All @@ -179,7 +213,7 @@ describe('ReactTestRendererAsync', () => {
);

expect(() => renderer.unstable_flushThrough(['foo', 'bar'])).toThrow(
'flushThrough expected to yield "bar", but nothing was yielded',
'Flush expected to yield "bar", but nothing was yielded',
);
});

Expand All @@ -189,7 +223,7 @@ describe('ReactTestRendererAsync', () => {
});

expect(() => renderer.unstable_flushThrough(['foo'])).toThrow(
'flushThrough expected to yield "foo", but nothing was yielded',
'Flush expected to yield "foo", but nothing was yielded',
);
});
});
19 changes: 8 additions & 11 deletions packages/react/src/__tests__/ReactProfiler-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ describe('Profiler', () => {
// Times are logged until a render is committed.
renderer.unstable_flushThrough(['first']);
expect(callback).toHaveBeenCalledTimes(0);
expect(renderer.unstable_flushAll()).toEqual(['last']);
renderer.unstable_flushAll(['last']);
expect(callback).toHaveBeenCalledTimes(1);
});

Expand Down Expand Up @@ -532,7 +532,7 @@ describe('Profiler', () => {
expect(callback).toHaveBeenCalledTimes(0);

// Resume render for remaining children.
expect(renderer.unstable_flushAll()).toEqual(['Yield:3']);
renderer.unstable_flushAll(['Yield:3']);

// Verify that logged times include both durations above.
expect(callback).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -576,7 +576,7 @@ describe('Profiler', () => {

// Flush the remaninig work,
// Which should take an additional 10ms of simulated time.
expect(renderer.unstable_flushAll()).toEqual(['Yield:10', 'Yield:17']);
renderer.unstable_flushAll(['Yield:10', 'Yield:17']);
expect(callback).toHaveBeenCalledTimes(2);

const [innerCall, outerCall] = callback.mock.calls;
Expand Down Expand Up @@ -647,7 +647,7 @@ describe('Profiler', () => {
callback.mockReset();

// Verify no more unexpected callbacks from low priority work
expect(renderer.unstable_flushAll()).toEqual([]);
renderer.unstable_flushAll([]);
expect(callback).toHaveBeenCalledTimes(0);
});

Expand All @@ -672,7 +672,7 @@ describe('Profiler', () => {

// Render everything initially.
// This should take 21 seconds of actual and base time.
expect(renderer.unstable_flushAll()).toEqual(['Yield:6', 'Yield:15']);
renderer.unstable_flushAll(['Yield:6', 'Yield:15']);
expect(callback).toHaveBeenCalledTimes(1);
let call = callback.mock.calls[0];
expect(call[2]).toBe(21); // actual time
Expand Down Expand Up @@ -733,7 +733,7 @@ describe('Profiler', () => {
expect(call[5]).toBe(275); // commit time

// Verify no more unexpected callbacks from low priority work
expect(renderer.unstable_flushAll()).toEqual([]);
renderer.unstable_flushAll([]);
expect(callback).toHaveBeenCalledTimes(1);
});

Expand Down Expand Up @@ -780,7 +780,7 @@ describe('Profiler', () => {
// Render everything initially.
// This simulates a total of 14ms of actual render time.
// The base render time is also 14ms for the initial render.
expect(renderer.unstable_flushAll()).toEqual([
renderer.unstable_flushAll([
'FirstComponent:1',
'Yield:4',
'SecondComponent:2',
Expand Down Expand Up @@ -836,10 +836,7 @@ describe('Profiler', () => {
// The tree contains 42ms of base render time at this point,
// Reflecting the most recent (longer) render durations.
// TODO: This actual time should decrease by 10ms once the scheduler supports resuming.
expect(renderer.unstable_flushAll()).toEqual([
'FirstComponent:10',
'Yield:4',
]);
renderer.unstable_flushAll(['FirstComponent:10', 'Yield:4']);
expect(callback).toHaveBeenCalledTimes(1);
call = callback.mock.calls[0];
expect(call[2]).toBe(14); // actual time
Expand Down