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

One more idea for the profiler #11379

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
562c3bb
Make profile return wrapper component without taking internal component
jerelmiller Nov 22, 2023
841a9ef
Rename profile to createTestProfiler
jerelmiller Nov 22, 2023
ed0d2fc
Fix error fallback in default creation
jerelmiller Nov 22, 2023
895792c
Convert additional test to updated API
jerelmiller Nov 22, 2023
ee44633
Rename ProfiledComponent type to Profiler
jerelmiller Nov 22, 2023
2a433a4
Remove unneeded wrapper for tracking renders
jerelmiller Nov 22, 2023
e5efcc9
Don't require args to createTestProfiler
jerelmiller Nov 22, 2023
c39cba0
Fix types on profiled hook
jerelmiller Nov 22, 2023
1e64614
Track component function instead of component name for rendered compo…
jerelmiller Nov 22, 2023
02aa46c
Fix type on matchers
jerelmiller Nov 22, 2023
61443bd
Move render context into own file and add all of context to render in…
jerelmiller Nov 22, 2023
d8c129e
Copy context before passing to RenderInstance
jerelmiller Nov 22, 2023
4914048
Throw if render context is not found
jerelmiller Nov 22, 2023
87bcbb9
Rename useTrackComponentRender to useTrackRender
jerelmiller Nov 22, 2023
a683081
Go back to renderedComponents directly on RenderInstance
jerelmiller Nov 22, 2023
5c562e4
Remove eslint disable
jerelmiller Nov 22, 2023
5e8aadc
Update another test to use new pattern
jerelmiller Nov 22, 2023
79e00f5
Fix usage of profileHook with updates to profiler
jerelmiller Nov 22, 2023
659f884
Fix matchers with updates to profiler
jerelmiller Nov 22, 2023
2a5a487
Update test that checks context to use updated API
jerelmiller Nov 22, 2023
836c1c4
Update another test that checks client overriden to new API
jerelmiller Nov 22, 2023
4d27a07
Update test that checks for cache update
jerelmiller Nov 22, 2023
2b64982
Rename render context to profiler context
jerelmiller Nov 22, 2023
5df75f9
Extract helper to create default profiler for the tests
jerelmiller Nov 22, 2023
c408bed
Convert test that checks for canonical results to new API
jerelmiller Nov 22, 2023
554ecac
Update cache-and-network test to new API
jerelmiller Nov 22, 2023
891183c
Update test that checks for rendered error boundary when refetch throws
jerelmiller Nov 22, 2023
ce05f8a
Update multiple refetch test to use new API
jerelmiller Nov 22, 2023
ccde7cb
Rename createTestProfiler to createProfiler
jerelmiller Nov 27, 2023
0d141df
Recreate profile helper by using createProfiler
jerelmiller Nov 27, 2023
4b9a0ec
Use profile export in tests that previously used it
jerelmiller Nov 27, 2023
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
4 changes: 2 additions & 2 deletions src/react/components/__tests__/client/Query.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { ApolloProvider } from "../../../context";
import { itAsync, MockedProvider, mockSingleLink } from "../../../../testing";
import { Query } from "../../Query";
import { QueryResult } from "../../../types/types";
import { profile } from "../../../../testing/internal";
import { createTestProfiler } from "../../../../testing/internal";

const allPeopleQuery: DocumentNode = gql`
query people {
Expand Down Expand Up @@ -1498,7 +1498,7 @@ describe("Query component", () => {
);
}

const ProfiledContainer = profile<QueryResult>({
const ProfiledContainer = createTestProfiler<QueryResult>({
Component: Container,
});

Expand Down
4 changes: 2 additions & 2 deletions src/react/hoc/__tests__/queries/lifecycle.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { mockSingleLink } from "../../../../testing";
import { Query as QueryComponent } from "../../../components";
import { graphql } from "../../graphql";
import { ChildProps, DataValue } from "../../types";
import { profile } from "../../../../testing/internal";
import { createTestProfiler } from "../../../../testing/internal";

describe("[queries] lifecycle", () => {
// lifecycle
Expand Down Expand Up @@ -58,7 +58,7 @@ describe("[queries] lifecycle", () => {
}
);

const ProfiledApp = profile<DataValue<Data, Vars>, Vars>({
const ProfiledApp = createTestProfiler<DataValue<Data, Vars>, Vars>({
Component: Container,
});

Expand Down
4 changes: 2 additions & 2 deletions src/react/hoc/__tests__/queries/loading.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { InMemoryCache as Cache } from "../../../../cache";
import { itAsync, mockSingleLink } from "../../../../testing";
import { graphql } from "../../graphql";
import { ChildProps, DataValue } from "../../types";
import { profile } from "../../../../testing/internal";
import { createTestProfiler } from "../../../../testing/internal";

describe("[queries] loading", () => {
// networkStatus / loading
Expand Down Expand Up @@ -413,7 +413,7 @@ describe("[queries] loading", () => {
}
);

const ProfiledContainer = profile<
const ProfiledContainer = createTestProfiler<
DataValue<{
allPeople: {
people: {
Expand Down
10 changes: 5 additions & 5 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import {
import equal from "@wry/equality";
import { RefetchWritePolicy } from "../../../core/watchQueryOptions";
import { skipToken } from "../constants";
import { profile, spyOnConsole } from "../../../testing/internal";
import { createTestProfiler, spyOnConsole } from "../../../testing/internal";

function renderIntegrationTest({
client,
Expand Down Expand Up @@ -332,7 +332,7 @@ function renderVariablesIntegrationTest({
);
}

const ProfiledApp = profile<Renders, ComponentProps<typeof App>>({
const ProfiledApp = createTestProfiler<Renders, ComponentProps<typeof App>>({
Component: App,
snapshotDOM: true,
onRender: ({ replaceSnapshot }) => replaceSnapshot(cloneDeep(renders)),
Expand Down Expand Up @@ -516,7 +516,7 @@ function renderPaginatedIntegrationTest({
);
}

const ProfiledApp = profile({
const ProfiledApp = createTestProfiler({
Component: App,
snapshotDOM: true,
initialSnapshot: {
Expand Down Expand Up @@ -3895,7 +3895,7 @@ describe("useBackgroundQuery", () => {
);
}

const ProfiledApp = profile({ Component: App, snapshotDOM: true });
const ProfiledApp = createTestProfiler({ Component: App, snapshotDOM: true });

render(<ProfiledApp />);

Expand Down Expand Up @@ -4193,7 +4193,7 @@ describe("useBackgroundQuery", () => {
);
}

const ProfiledApp = profile({ Component: App, snapshotDOM: true });
const ProfiledApp = createTestProfiler({ Component: App, snapshotDOM: true });
render(<ProfiledApp />);

{
Expand Down
8 changes: 4 additions & 4 deletions src/react/hooks/__tests__/useFragment.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { concatPagination } from "../../../utilities";
import assert from "assert";
import { expectTypeOf } from "expect-type";
import { SubscriptionObserver } from "zen-observable-ts";
import { profile, spyOnConsole } from "../../../testing/internal";
import { createTestProfiler, spyOnConsole } from "../../../testing/internal";

describe("useFragment", () => {
it("is importable and callable", () => {
Expand Down Expand Up @@ -1481,7 +1481,7 @@ describe("has the same timing as `useQuery`", () => {
return complete ? JSON.stringify(fragmentData) : "loading";
}

const ProfiledComponent = profile({
const ProfiledComponent = createTestProfiler({
Component,
initialSnapshot: {
queryData: undefined as any,
Expand Down Expand Up @@ -1569,7 +1569,7 @@ describe("has the same timing as `useQuery`", () => {
return <>{JSON.stringify({ item: data })}</>;
}

const ProfiledParent = profile({
const ProfiledParent = createTestProfiler({
Component: Parent,
snapshotDOM: true,
onRender() {
Expand Down Expand Up @@ -1664,7 +1664,7 @@ describe("has the same timing as `useQuery`", () => {
return <>{JSON.stringify(data)}</>;
}

const ProfiledParent = profile({
const ProfiledParent = createTestProfiler({
Component: Parent,
onRender() {
const parent = screen.getByTestId("parent");
Expand Down
187 changes: 110 additions & 77 deletions src/react/hooks/__tests__/useLoadableQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,12 @@ import { LoadableQueryHookFetchPolicy } from "../../types/types";
import { QueryReference } from "../../../react";
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't worry about looking through all of these. I tried to convert a variety of use cases to ensure the updates to the API held up in each case.

import { FetchMoreFunction, RefetchFunction } from "../useSuspenseQuery";
import invariant, { InvariantError } from "ts-invariant";
import { profile, profileHook, spyOnConsole } from "../../../testing/internal";
import {
Profiler,
createTestProfiler,
spyOnConsole,
useTrackComponentRender,
} from "../../../testing/internal";

interface SimpleQueryData {
greeting: string;
Expand Down Expand Up @@ -149,28 +154,34 @@ function usePaginatedQueryCase() {
return { query, link, client };
}

function createDefaultProfiledComponents<TData = unknown>() {
const SuspenseFallback = profile({
Component: function SuspenseFallback() {
return <p>Loading</p>;
},
});
function createDefaultProfiledComponents<
Snapshot extends {
result: UseReadQueryResult<any> | null;
error?: Error | null;
},
TData = Snapshot["result"] extends UseReadQueryResult<infer TData> | null
? TData
: unknown,
>(profiler: Profiler<Snapshot>) {
function SuspenseFallback() {
useTrackComponentRender();
return <p>Loading</p>;
}

const ReadQueryHook = profileHook<
UseReadQueryResult<TData>,
{ queryRef: QueryReference<TData> }
>(({ queryRef }) => useReadQuery(queryRef), { displayName: "UseReadQuery" });
function ReadQueryHook({ queryRef }: { queryRef: QueryReference<TData> }) {
useTrackComponentRender();
profiler.mergeSnapshot({
result: useReadQuery(queryRef),
} as Partial<Snapshot>);

const ErrorFallback = profile<{ error: Error | null }, { error: Error }>({
Component: function Fallback({ error }) {
ErrorFallback.replaceSnapshot({ error });
return null;
}

return <div>Oops</div>;
},
initialSnapshot: {
error: null,
},
});
function ErrorFallback({ error }: { error: Error }) {
profiler.mergeSnapshot({ error } as Partial<Snapshot>);

return <div>Oops</div>;
}

function ErrorBoundary({ children }: { children: React.ReactNode }) {
return (
Expand Down Expand Up @@ -219,98 +230,120 @@ function renderWithClient(
it("loads a query and suspends when the load query function is called", async () => {
const { query, mocks } = useSimpleQueryCase();

const Profiler = createTestProfiler({
initialSnapshot: {
result: null as UseReadQueryResult<SimpleQueryData> | null,
},
});

const { SuspenseFallback, ReadQueryHook } =
createDefaultProfiledComponents<SimpleQueryData>();
createDefaultProfiledComponents(Profiler);

const App = profile({
Component: () => {
const [loadQuery, queryRef] = useLoadableQuery(query);
function App() {
useTrackComponentRender();
const [loadQuery, queryRef] = useLoadableQuery(query);

return (
<>
<button onClick={() => loadQuery()}>Load query</button>
<Suspense fallback={<SuspenseFallback />}>
{queryRef && <ReadQueryHook queryRef={queryRef} />}
</Suspense>
</>
);
},
});
return (
<>
<button onClick={() => loadQuery()}>Load query</button>
<Suspense fallback={<SuspenseFallback />}>
{queryRef && <ReadQueryHook queryRef={queryRef} />}
</Suspense>
</>
);
}

const { user } = renderWithMocks(<App />, { mocks });
const { user } = renderWithMocks(
<Profiler>
<App />
</Profiler>,
{ mocks }
);

expect(SuspenseFallback).not.toHaveRendered();
{
const { renderedComponents } = await Profiler.takeRender();

expect(renderedComponents).toStrictEqual([App]);
}

await act(() => user.click(screen.getByText("Load query")));

expect(SuspenseFallback).toHaveRendered();
expect(ReadQueryHook).not.toHaveRendered();
expect(App).toHaveRenderedTimes(2);
{
const { renderedComponents } = await Profiler.takeRender();

const snapshot = await ReadQueryHook.takeSnapshot();
expect(renderedComponents).toStrictEqual([App, SuspenseFallback]);
}

expect(snapshot).toEqual({
data: { greeting: "Hello" },
error: undefined,
networkStatus: NetworkStatus.ready,
});
{
const { snapshot, renderedComponents } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: { greeting: "Hello" },
error: undefined,
networkStatus: NetworkStatus.ready,
});

expect(SuspenseFallback).toHaveRenderedTimes(1);
expect(ReadQueryHook).toHaveRenderedTimes(1);
expect(App).toHaveRenderedTimes(3);
expect(renderedComponents).toStrictEqual([ReadQueryHook]);
}
});

it("loads a query with variables and suspends by passing variables to the loadQuery function", async () => {
const { query, mocks } = useVariablesQueryCase();

const { SuspenseFallback, ReadQueryHook } =
createDefaultProfiledComponents<VariablesCaseData>();

const App = profile({
Component: function App() {
const [loadQuery, queryRef] = useLoadableQuery(query);

return (
<>
<button onClick={() => loadQuery({ id: "1" })}>Load query</button>
<Suspense fallback={<SuspenseFallback />}>
{queryRef && <ReadQueryHook queryRef={queryRef} />}
</Suspense>
</>
);
const Profiler = createTestProfiler({
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 pattern makes it easier for me to extract this into a helper if need-be. In fact, 90% of the tests in this suite operate on the same snapshot shape, which means I could extract this to a reusable function to avoid some repetition in tests. For example:

function createLoadableQueryProfiler<TData>() {
  return createTestProfiler({
    initialSnapshot: {
      lastError: null as Error | null,
      result: null as UseReadQueryResult<TData> | null
    }
  })
}

then in my tests:

it('checks something about useLoadableQuery', async () => {
  const Profiler = createLoadableQueryProfiler();

  // snapshot is now of shape { lastError, result }
  const { snapshot } = await Profiler.takeRender()
})

initialSnapshot: {
result: null as UseReadQueryResult<VariablesCaseData> | null,
},
});

const { user } = renderWithMocks(<App />, { mocks });
const { SuspenseFallback, ReadQueryHook } =
createDefaultProfiledComponents(Profiler);

{
const { renderedComponents } = await App.takeRender();
expect(renderedComponents).toStrictEqual(["App"]);
function App() {
useTrackComponentRender();
const [loadQuery, queryRef] = useLoadableQuery(query);

return (
<>
<button onClick={() => loadQuery({ id: "1" })}>Load query</button>
<Suspense fallback={<SuspenseFallback />}>
{queryRef && <ReadQueryHook queryRef={queryRef} />}
</Suspense>
</>
);
}

await act(() => user.click(screen.getByText("Load query")));
const { user } = renderWithMocks(
<Profiler>
<App />
</Profiler>,
{ mocks }
);

{
const { renderedComponents } = await App.takeRender();
expect(renderedComponents).toStrictEqual(["App", "SuspenseFallback"]);
const { renderedComponents } = await Profiler.takeRender();
expect(renderedComponents).toStrictEqual([App]);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we enforce a single profiler, it makes it more difficult to avoid stepping through each render like I originally did with the nested profiler approach.

}

await act(() => user.click(screen.getByText("Load query")));

{
const { renderedComponents } = await App.takeRender();
expect(renderedComponents).toStrictEqual(["UseReadQuery"]);
const { renderedComponents } = await Profiler.takeRender();
expect(renderedComponents).toStrictEqual([App, SuspenseFallback]);
}

{
const snapshot = await ReadQueryHook.takeSnapshot();
const { snapshot, renderedComponents } = await Profiler.takeRender();

expect(snapshot).toEqual({
expect(renderedComponents).toStrictEqual([ReadQueryHook]);
expect(snapshot.result).toEqual({
data: { character: { id: "1", name: "Spider-Man" } },
networkStatus: NetworkStatus.ready,
error: undefined,
});
}

await expect(App).not.toRerender();
await expect(Profiler).not.toRerender();
});

it("changes variables on a query and resuspends when passing new variables to the loadQuery function", async () => {
Expand All @@ -319,7 +352,7 @@ it("changes variables on a query and resuspends when passing new variables to th
const { SuspenseFallback, ReadQueryHook } =
createDefaultProfiledComponents<VariablesCaseData>();

const App = profile({
const App = createTestProfiler({
Component: () => {
const [loadQuery, queryRef] = useLoadableQuery(query);

Expand Down Expand Up @@ -660,7 +693,7 @@ it("returns initial cache data followed by network data when the fetch policy is
hello: string;
}>();

const App = profile({
const App = createTestProfiler({
Component: () => {
const [loadQuery, queryRef] = useLoadableQuery(query, {
fetchPolicy: "cache-and-network",
Expand Down
Loading
Loading