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

Minor adjustments to profiler #11376

Merged
merged 15 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion src/react/components/__tests__/client/Query.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,7 @@ describe("Query component", () => {
return (
<AllPeopleQuery2 query={query} notifyOnNetworkStatusChange={true}>
{(r: any) => {
ProfiledContainer.updateSnapshot(r);
ProfiledContainer.replaceSnapshot(r);
return null;
}}
</AllPeopleQuery2>
Expand Down
2 changes: 1 addition & 1 deletion src/react/hoc/__tests__/queries/lifecycle.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe("[queries] lifecycle", () => {
})(
class extends React.Component<ChildProps<Vars, Data, Vars>> {
render() {
ProfiledApp.updateSnapshot(this.props.data!);
ProfiledApp.replaceSnapshot(this.props.data!);
return null;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/react/hoc/__tests__/queries/loading.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ describe("[queries] loading", () => {
})(
class extends React.Component<ChildProps<{}, Data>> {
render() {
ProfiledContainer.updateSnapshot(this.props.data!);
ProfiledContainer.replaceSnapshot(this.props.data!);
return null;
}
}
Expand Down
19 changes: 8 additions & 11 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ function renderVariablesIntegrationTest({
const ProfiledApp = profile<Renders, ComponentProps<typeof App>>({
Component: App,
snapshotDOM: true,
onRender: ({ updateSnapshot }) => updateSnapshot(cloneDeep(renders)),
onRender: ({ replaceSnapshot }) => replaceSnapshot(cloneDeep(renders)),
});

const { ...rest } = render(
Expand Down Expand Up @@ -434,9 +434,8 @@ function renderPaginatedIntegrationTest({
}

function SuspenseFallback() {
ProfiledApp.updateSnapshot((snapshot) => ({
...snapshot,
suspenseCount: snapshot.suspenseCount + 1,
ProfiledApp.mergeSnapshot(({ suspenseCount }) => ({
suspenseCount: suspenseCount + 1,
}));
return <div>loading</div>;
}
Expand All @@ -450,9 +449,8 @@ function renderPaginatedIntegrationTest({
}) {
const { data, error } = useReadQuery(queryRef);
// count renders in the child component
ProfiledApp.updateSnapshot((snapshot) => ({
...snapshot,
count: snapshot.count + 1,
ProfiledApp.mergeSnapshot(({ count }) => ({
count: count + 1,
}));
return (
<div>
Expand Down Expand Up @@ -504,10 +502,9 @@ function renderPaginatedIntegrationTest({
<ErrorBoundary
fallback={<div>Error</div>}
onError={(error) => {
ProfiledApp.updateSnapshot((snapshot) => ({
...snapshot,
errorCount: snapshot.errorCount + 1,
errors: snapshot.errors.concat(error),
ProfiledApp.mergeSnapshot(({ errorCount, errors }) => ({
errorCount: errorCount + 1,
errors: errors.concat(error),
}));
}}
>
Expand Down
2 changes: 1 addition & 1 deletion src/react/hooks/__tests__/useFragment.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1476,7 +1476,7 @@ describe("has the same timing as `useQuery`", () => {
from: initialItem,
});

ProfiledComponent.updateSnapshot({ queryData, fragmentData });
ProfiledComponent.replaceSnapshot({ queryData, fragmentData });

return complete ? JSON.stringify(fragmentData) : "loading";
}
Expand Down
2 changes: 1 addition & 1 deletion src/react/hooks/__tests__/useLazyQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1115,7 +1115,7 @@ describe("useLazyQuery Hook", () => {
),
});

const [execute] = ProfiledHook.getCurrentSnapshot();
const [execute] = await ProfiledHook.peekSnapshot();

{
const [, result] = await ProfiledHook.takeSnapshot();
Expand Down
2 changes: 1 addition & 1 deletion src/react/hooks/__tests__/useSuspenseQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ describe("useSuspenseQuery", () => {

const Component = () => {
const result = useSuspenseQuery(query);
ProfiledApp.updateSnapshot(result);
ProfiledApp.replaceSnapshot(result);
return <div>{result.data.greeting}</div>;
};

Expand Down
96 changes: 59 additions & 37 deletions src/testing/internal/profile/profile.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,27 @@ export interface NextRenderOptions {
export interface ProfiledComponent<Props, Snapshot>
extends React.FC<Props>,
ProfiledComponentFields<Props, Snapshot>,
ProfiledComponenOnlyFields<Props, Snapshot> {}
ProfiledComponentOnlyFields<Props, Snapshot> {}

interface UpdateSnapshot<Snapshot> {
interface ReplaceSnapshot<Snapshot> {
(newSnapshot: Snapshot): void;
(updateSnapshot: (lastSnapshot: Readonly<Snapshot>) => Snapshot): void;
}

interface ProfiledComponenOnlyFields<Props, Snapshot> {
updateSnapshot: UpdateSnapshot<Snapshot>;
interface MergeSnapshot<Snapshot> {
(partialSnapshot: Partial<Snapshot>): void;
(
updatePartialSnapshot: (
lastSnapshot: Readonly<Snapshot>
) => Partial<Snapshot>
): void;
}

interface ProfiledComponentOnlyFields<Props, Snapshot> {
// Allows for partial updating of the snapshot by shallow merging the results
mergeSnapshot: MergeSnapshot<Snapshot>;
// Performs a full replacement of the snapshot
replaceSnapshot: ReplaceSnapshot<Snapshot>;
}
interface ProfiledComponentFields<Props, Snapshot> {
/**
Expand All @@ -54,21 +66,14 @@ interface ProfiledComponentFields<Props, Snapshot> {
*/
takeRender(options?: NextRenderOptions): Promise<Render<Snapshot>>;
/**
* Returns the current render count.
* Returns the total number of renders.
*/
currentRenderCount(): number;
totalRenderCount(): number;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now the only function that doesn't rely on the iterator position in its implementation. If possible, we should keep it that way to try and maintain the intent of the original API which should force you to iterate on each render in your tests.

/**
* Returns the current render.
* @throws {Error} if no render has happened yet
*/
getCurrentRender(): Render<Snapshot>;
/**
* Iterates the renders until the render count is reached.
*/
takeUntilRenderCount(
Copy link
Member Author

Choose a reason for hiding this comment

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

This behaved in a way that was unintuitive due to the fact that it used the total render count rather than the iterator in its implementation. This isn't currently used, so I'd like us to try using the other methods first before determining if we need to reintroduce this.

If we do reintroduce, I'd propose that instead of thinking about taking each render up to a specific count, that instead this method advances the iterator a certain number of renders from "current". Doing so would prevent weird situations like this:

await Profiled.takeRender()
await Profiled.takeRender()
await Profiled.takeRender()

// This makes no sense since render #2 is now in the past
await Profiled.takeUntilRenderCount(2)

count: number,
optionsPerRender?: NextRenderOptions
): Promise<void>;
/**
* Waits for the next render to happen.
* Does not advance the render iterator.
Expand All @@ -90,18 +95,18 @@ export function profile<
onRender?: (
info: BaseRender & {
snapshot: Snapshot;
updateSnapshot: UpdateSnapshot<Snapshot>;
replaceSnapshot: ReplaceSnapshot<Snapshot>;
mergeSnapshot: MergeSnapshot<Snapshot>;
}
) => void;
snapshotDOM?: boolean;
initialSnapshot?: Snapshot;
}) {
let currentRender: Render<Snapshot> | undefined;
let nextRender: Promise<Render<Snapshot>> | undefined;
let resolveNextRender: ((render: Render<Snapshot>) => void) | undefined;
let rejectNextRender: ((error: unknown) => void) | undefined;
const snapshotRef = { current: initialSnapshot };
const updateSnapshot: UpdateSnapshot<Snapshot> = (snap) => {
const replaceSnapshot: ReplaceSnapshot<Snapshot> = (snap) => {
if (typeof snap === "function") {
if (!initialSnapshot) {
throw new Error(
Expand All @@ -118,6 +123,16 @@ export function profile<
snapshotRef.current = snap;
}
};

const mergeSnapshot: MergeSnapshot<Snapshot> = (partialSnapshot) => {
replaceSnapshot((snapshot) => ({
...snapshot,
...(typeof partialSnapshot === "function"
? partialSnapshot(snapshot)
: partialSnapshot),
}));
};

const profilerOnRender: React.ProfilerOnRenderCallback = (
id,
phase,
Expand Down Expand Up @@ -145,7 +160,8 @@ export function profile<
*/
onRender?.({
...baseRender,
updateSnapshot,
replaceSnapshot,
mergeSnapshot,
snapshot: snapshotRef.current!,
});

Expand All @@ -154,8 +170,6 @@ export function profile<
? window.document.body.innerHTML
: undefined;
const render = new RenderInstance(baseRender, snapshot, domSnapshot);
// eslint-disable-next-line testing-library/render-result-naming-convention
currentRender = render;
Profiled.renders.push(render);
resolveNextRender?.(render);
} catch (error) {
Expand All @@ -178,29 +192,31 @@ export function profile<
</React.Profiler>
),
{
updateSnapshot,
} satisfies ProfiledComponenOnlyFields<Props, Snapshot>,
replaceSnapshot,
mergeSnapshot,
} satisfies ProfiledComponentOnlyFields<Props, Snapshot>,
{
renders: new Array<
| Render<Snapshot>
| { phase: "snapshotError"; count: number; error: unknown }
>(),
currentRenderCount() {
totalRenderCount() {
return Profiled.renders.length;
},
async peekRender(options: NextRenderOptions = {}) {
if (iteratorPosition < Profiled.renders.length) {
const render = Profiled.renders[iteratorPosition];

if (render.phase === "snapshotError") {
throw render.error;
}

return render;
}
const render = Profiled.waitForNextRender({
return Profiled.waitForNextRender({
[_stackTrace]: captureStackTrace(Profiled.peekRender),
...options,
});
return render;
},
async takeRender(options: NextRenderOptions = {}) {
let error: unknown = undefined;
Expand All @@ -219,18 +235,25 @@ export function profile<
}
},
getCurrentRender() {
if (!currentRender) {
throw new Error("Has not been rendered yet!");
// The "current" render should point at the same render that the most
// recent `takeRender` call returned, so we need to get the "previous"
// iterator position, otherwise `takeRender` advances the iterator
// to the next render. This means we need to call `takeRender` at least
// once before we can get a current render.
const currentPosition = iteratorPosition - 1;

if (currentPosition < 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Because I'm changing "current render" to mean the render pointing to the most recent takeRender call, I'm requiring that takeRender be called at least once. We could just point at the first render if its available, but I felt that to be weird in the following situation:

const current1 = Profiled.getCurrentRender();
const result = await Profiled.takeRender();

// Without requiring `takeRender`, this would be equal to current1
const current2 = Profiled.getCurrentRender();

On the flip side, we could make getCurrentRender be equal to the iterator position, but this means that it represents the render "after" the last takeRender call, which also feels weird. This means that calling getCurrentRender after you've exhausted the render array via takeRender would point to a render that does not exist.

throw new Error(
"No current render available. You need to call `takeRender` before you can get the current render."
);
}
return currentRender;
},
async takeUntilRenderCount(
count: number,
optionsPerRender?: NextRenderOptions
) {
while (Profiled.renders.length < count) {
await Profiled.takeRender(optionsPerRender);

const render = Profiled.renders[currentPosition];

if (render.phase === "snapshotError") {
throw render.error;
}
return render;
},
waitForNextRender({
timeout = 1000,
Expand Down Expand Up @@ -306,7 +329,7 @@ export function profileHook<ReturnValue extends ValidSnapshot, Props>(
): ProfiledHook<Props, ReturnValue> {
let returnValue: ReturnValue;
const Component = (props: Props) => {
ProfiledComponent.updateSnapshot(renderCallback(props));
ProfiledComponent.replaceSnapshot(renderCallback(props));
return null;
};
const ProfiledComponent = profile<ReturnValue, Props>({
Expand All @@ -322,7 +345,7 @@ export function profileHook<ReturnValue extends ValidSnapshot, Props>(
},
{
renders: ProfiledComponent.renders,
currentSnapshotCount: ProfiledComponent.currentRenderCount,
totalSnapshotCount: ProfiledComponent.totalRenderCount,
async peekSnapshot(options) {
return (await ProfiledComponent.peekRender(options)).snapshot;
},
Expand All @@ -332,7 +355,6 @@ export function profileHook<ReturnValue extends ValidSnapshot, Props>(
getCurrentSnapshot() {
return ProfiledComponent.getCurrentRender().snapshot;
},
takeUntilSnapshotCount: ProfiledComponent.takeUntilRenderCount,
async waitForNextSnapshot(options) {
return (await ProfiledComponent.waitForNextRender(options)).snapshot;
},
Expand Down
6 changes: 3 additions & 3 deletions src/testing/matchers/ProfiledComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ export const toRenderExactlyTimes: MatcherFunction<
const hint = this.utils.matcherHint("toRenderExactlyTimes");
let pass = true;
try {
if (profiled.currentRenderCount() > times) {
if (profiled.totalRenderCount() > times) {
throw failed;
}
try {
while (profiled.currentRenderCount() < times) {
while (profiled.totalRenderCount() < times) {
await profiled.waitForNextRender(options);
}
} catch (e) {
Expand Down Expand Up @@ -84,7 +84,7 @@ export const toRenderExactlyTimes: MatcherFunction<
return (
hint +
` Expected component to${pass ? " not" : ""} render exactly ${times}.` +
` It rendered ${profiled.currentRenderCount()} times.`
` It rendered ${profiled.totalRenderCount()} times.`
);
},
};
Expand Down
Loading