From a0ba87b20209350621b44d44d1d8fd56d65e340d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 30 Apr 2020 12:05:09 -0400 Subject: [PATCH 1/5] Fix useQuery tests. --- src/react/hooks/__tests__/useQuery.test.tsx | 146 ++++++++++++-------- 1 file changed, 86 insertions(+), 60 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index b3aa0e4d37e..0123697cd4c 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -12,6 +12,7 @@ import { InMemoryCache } from '../../../cache/inmemory/inMemoryCache'; import { ApolloProvider } from '../../context/ApolloProvider'; import { useQuery } from '../useQuery'; import { requireReactLazily } from '../../react'; +import { NetworkStatus } from '../../../core/networkStatus'; const React = requireReactLazily(); const { useState, useReducer, Fragment } = React; @@ -381,28 +382,25 @@ describe('useQuery Hook', () => { it('should support polling', async () => { let renderCount = 0; const Component = () => { - let { data, loading, stopPolling } = useQuery(CAR_QUERY, { + let { data, loading, networkStatus, stopPolling } = useQuery(CAR_QUERY, { pollInterval: 10 }); - switch (renderCount) { - case 0: - expect(loading).toBeTruthy(); - break; + + switch (++renderCount) { case 1: - expect(loading).toBeFalsy(); - expect(data).toEqual(CAR_RESULT_DATA); + expect(loading).toBeTruthy(); + expect(networkStatus).toBe(NetworkStatus.loading); break; case 2: expect(loading).toBeFalsy(); expect(data).toEqual(CAR_RESULT_DATA); + expect(networkStatus).toBe(NetworkStatus.ready); stopPolling(); break; - case 3: - throw new Error('Uh oh - we should have stopped polling!'); default: - // Do nothing + throw new Error('Uh oh - we should have stopped polling!'); } - renderCount += 1; + return null; }; @@ -413,7 +411,7 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(3); + expect(renderCount).toBe(2); }); }); @@ -545,8 +543,10 @@ describe('useQuery Hook', () => { return null; }; + const mocks = [...CAR_MOCKS, ...CAR_MOCKS]; + unmount = render( - + ).unmount; @@ -610,7 +610,7 @@ describe('useQuery Hook', () => { return wait(); }); - it('should only call onError callbacks once', async () => { + itAsync('should only call onError callbacks once', (resolve, reject) => { const query = gql` query SomeQuery { stuff { @@ -662,6 +662,10 @@ describe('useQuery Hook', () => { onErrorPromise.then(() => refetch()); break; case 3: + expect(loading).toBeTruthy(); + expect(networkStatus).toBe(NetworkStatus.refetch); + break; + case 4: expect(loading).toBeFalsy(); expect(data).toEqual(resultData); break; @@ -678,11 +682,11 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(3); - }); + expect(renderCount).toBe(4); + }).then(resolve, reject); }); - it('should persist errors on re-render if they are still valid', async () => { + itAsync('should persist errors on re-render if they are still valid', (resolve, reject) => { const query = gql` query SomeQuery { stuff { @@ -705,26 +709,25 @@ describe('useQuery Hook', () => { const [_, forceUpdate] = useReducer(x => x + 1, 0); const { loading, error } = useQuery(query); - switch (renderCount) { - case 0: + switch (++renderCount) { + case 1: expect(loading).toBeTruthy(); expect(error).toBeUndefined(); break; - case 1: + case 2: expect(error).toBeDefined(); expect(error!.message).toEqual('forced error'); setTimeout(() => { forceUpdate(); }); break; - case 2: + case 3: expect(error).toBeDefined(); expect(error!.message).toEqual('forced error'); break; default: // Do nothing } - renderCount += 1; return null; } @@ -736,7 +739,7 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(3); - }); + }).then(resolve, reject); }); itAsync( @@ -806,7 +809,7 @@ describe('useQuery Hook', () => { } ); - it('should render errors (different error messages) with loading done on refetch', async () => { + itAsync('should render errors (different error messages) with loading done on refetch', (resolve, reject) => { const query = gql` query SomeQuery { stuff { @@ -851,6 +854,10 @@ describe('useQuery Hook', () => { }); break; case 3: + expect(loading).toBeTruthy(); + expect(error).toBeUndefined(); + break; + case 4: expect(loading).toBeFalsy(); expect(error).toBeDefined(); expect(error!.message).toEqual('an error 2'); @@ -868,8 +875,8 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(3); - }); + expect(renderCount).toBe(4); + }).then(resolve, reject); }); itAsync('should not re-render same error message on refetch', (resolve, reject) => { @@ -917,6 +924,15 @@ describe('useQuery Hook', () => { } }); break; + case 3: + expect(loading).toBeTruthy(); + expect(error).toBeUndefined(); + break; + case 4: + expect(loading).toBeFalsy(); + expect(error).toBeDefined(); + expect(error!.message).toEqual('same error message'); + break; default: // Do nothing } @@ -930,7 +946,7 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(2); + expect(renderCount).toBe(4); }).then(resolve, reject); }); @@ -977,6 +993,9 @@ describe('useQuery Hook', () => { }); break; case 3: + expect(loading).toBeTruthy(); + break; + case 4: expect(loading).toBeFalsy(); expect(error).toBeUndefined(); expect(data).toEqual(CAR_RESULT_DATA); @@ -985,10 +1004,10 @@ describe('useQuery Hook', () => { refetch().catch(() => {}); }); break; - case 4: + case 5: expect(loading).toBeTruthy(); break; - case 5: + case 6: expect(loading).toBeFalsy(); expect(error).toBeDefined(); expect(error!.message).toEqual('same error message'); @@ -1006,7 +1025,7 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(5); + expect(renderCount).toBe(6); }); }); }); @@ -1070,11 +1089,11 @@ describe('useQuery Hook', () => { notifyOnNetworkStatusChange: true }); - switch (renderCount) { - case 0: + switch (++renderCount) { + case 1: expect(loading).toBeTruthy(); break; - case 1: + case 2: expect(loading).toBeFalsy(); expect(data).toEqual(carResults); fetchMore({ @@ -1082,28 +1101,25 @@ describe('useQuery Hook', () => { limit: 1 }, updateQuery: (prev, { fetchMoreResult }) => ({ - cars: [...prev.cars, ...fetchMoreResult.cars] - }) + cars: [ + ...prev.cars, + ...fetchMoreResult.cars, + ], + }), }); break; - case 2: - expect(loading).toBeTruthy(); - break; case 3: expect(loading).toBeFalsy(); expect(data).toEqual({ - cars: [carResults.cars[0]] - }); - break; - case 4: - expect(data).toEqual({ - cars: [carResults.cars[0], moreCarResults.cars[0]] + cars: [ + carResults.cars[0], + moreCarResults.cars[0], + ], }); break; default: } - renderCount += 1; return null; } @@ -1114,7 +1130,7 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(5); + expect(renderCount).toBe(3); }); } ); @@ -1220,7 +1236,7 @@ describe('useQuery Hook', () => { }); describe('Refetching', () => { - it('should properly handle refetching with different variables', async () => { + itAsync('should properly handle refetching with different variables', (resolve, reject) => { const carQuery: DocumentNode = gql` query cars($id: Int) { cars(id: $id) { @@ -1275,7 +1291,8 @@ describe('useQuery Hook', () => { let renderCount = 0; function App() { const { loading, data, refetch } = useQuery(carQuery, { - variables: { id: 1 } + variables: { id: 1 }, + notifyOnNetworkStatusChange: true, }); switch (renderCount) { @@ -1317,17 +1334,17 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(6); - }); + }).then(resolve, reject); }); }); describe('Partial refetching', () => { - it( + itAsync( 'should attempt a refetch when the query result was marked as being ' + 'partial, the returned data was reset to an empty Object by the ' + 'Apollo Client QueryManager (due to a cache miss), and the ' + '`partialRefetch` prop is `true`', - async () => { + (resolve, reject) => { const query: DocumentNode = gql` query AllPeople($name: String!) { allPeople(name: $name) { @@ -1380,31 +1397,40 @@ describe('useQuery Hook', () => { let renderCount = 0; const Component = () => { - const { loading, data } = useQuery(query, { + const { loading, data, networkStatus } = useQuery(query, { variables: { someVar: 'abc123' }, - partialRefetch: true + partialRefetch: true, + notifyOnNetworkStatusChange: true, }); - switch (renderCount) { - case 0: + switch (++renderCount) { + case 1: // Initial loading render expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + expect(networkStatus).toBe(NetworkStatus.loading); break; - case 1: + case 2: // `data` is missing and `partialRetch` is true, so a refetch // is triggered and loading is set as true again expect(loading).toBeTruthy(); expect(data).toBeUndefined(); + expect(networkStatus).toBe(NetworkStatus.loading); break; - case 2: + case 3: + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + expect(networkStatus).toBe(NetworkStatus.refetch); + break; + case 4: // Refetch has completed expect(loading).toBeFalsy(); expect(data).toEqual(peopleData); + expect(networkStatus).toBe(NetworkStatus.ready); break; default: } - renderCount += 1; return null; }; @@ -1415,8 +1441,8 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(3); - }); + expect(renderCount).toBe(4); + }).then(resolve, reject); } ); }); From 6a0bcf1a6473903c25ed2fa2cc122a36f0e8caac Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 30 Apr 2020 12:05:11 -0400 Subject: [PATCH 2/5] Finish converting all async useQuery tests to itAsync style. --- src/react/hooks/__tests__/useQuery.test.tsx | 92 +++++++++++---------- 1 file changed, 48 insertions(+), 44 deletions(-) diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 0123697cd4c..1227684daa1 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -51,7 +51,7 @@ describe('useQuery Hook', () => { afterEach(cleanup); describe('General use', () => { - it('should handle a simple query properly', async () => { + itAsync('should handle a simple query properly', (resolve, reject) => { const Component = () => { const { data, loading } = useQuery(CAR_QUERY); if (!loading) { @@ -66,10 +66,10 @@ describe('useQuery Hook', () => { ); - return wait(); + return wait().then(resolve, reject); }); - it('should keep data as undefined until data is actually returned', async () => { + itAsync('should keep data as undefined until data is actually returned', (resolve, reject) => { const Component = () => { const { data, loading } = useQuery(CAR_QUERY); if (loading) { @@ -86,10 +86,10 @@ describe('useQuery Hook', () => { ); - return wait(); + return wait().then(resolve, reject); }); - it('should return a result upon first call, if data is available', async () => { + itAsync('should return a result upon first call, if data is available', async (resolve, reject) => { // This test verifies that the `useQuery` hook returns a result upon its first // invocation if the data is available in the cache. This is essential for SSR // to work properly, since effects are not run during SSR. @@ -120,10 +120,10 @@ describe('useQuery Hook', () => { ); - return wait(); + return wait().then(resolve, reject); }); - it('should ensure ObservableQuery fields have a stable identity', async () => { + itAsync('should ensure ObservableQuery fields have a stable identity', (resolve, reject) => { let refetchFn: any; let fetchMoreFn: any; let updateQueryFn: any; @@ -164,10 +164,10 @@ describe('useQuery Hook', () => { ); - return wait(); + return wait().then(resolve, reject); }); - it('should update result when query result change', async () => { + itAsync('should update result when query result change', async (resolve, reject) => { const CAR_QUERY_BY_ID = gql` query($id: Int) { car(id: $id) { @@ -220,13 +220,13 @@ describe('useQuery Hook', () => { ); - await wait(() => + await wait(() => { expect(hookResponse).toHaveBeenLastCalledWith({ data: CAR_DATA_A4, loading: false, error: undefined, }) - ); + }); rerender( @@ -234,16 +234,18 @@ describe('useQuery Hook', () => { ); - await wait(() => + await wait(() => { expect(hookResponse).toHaveBeenLastCalledWith({ data: CAR_DATA_RS8, loading: false, error: undefined, - }) - ); + }); + }); + + resolve(); }); - it('should return result when result is equivalent', async () => { + itAsync('should return result when result is equivalent', async (resolve, reject) => { const CAR_QUERY_BY_ID = gql` query($id: Int) { car(id: $id) { @@ -289,13 +291,13 @@ describe('useQuery Hook', () => { ); - await wait(() => + await wait(() => { expect(hookResponse).toHaveBeenLastCalledWith({ data: CAR_DATA_A4, loading: false, error: undefined, }) - ); + }); rerender( @@ -313,16 +315,18 @@ describe('useQuery Hook', () => { ); - await wait(() => + await wait(() => { expect(hookResponse).toHaveBeenLastCalledWith({ data: CAR_DATA_A4, loading: false, error: undefined, }) - ); + }); + + resolve(); }); - it('should not error when forcing an update with React >= 16.13.0', async () => { + itAsync('should not error when forcing an update with React >= 16.13.0', (resolve, reject) => { let wasUpdateErrorLogged = false; const consoleError = console.error; console.error = (msg: string) => { @@ -346,10 +350,10 @@ describe('useQuery Hook', () => { fetchPolicy: 'network-only', variables: { something } }); + renderCount += 1; if (loading) return null; expect(wasUpdateErrorLogged).toBeFalsy(); expect(data).toEqual(CAR_RESULT_DATA); - renderCount += 1; return null; }; @@ -361,7 +365,7 @@ describe('useQuery Hook', () => { } render( - + @@ -370,16 +374,16 @@ describe('useQuery Hook', () => { ); - await wait(() => { + wait(() => { expect(renderCount).toBe(3); }).finally(() => { console.error = consoleError; - }); + }).then(resolve, reject); }); }); describe('Polling', () => { - it('should support polling', async () => { + itAsync('should support polling', (resolve, reject) => { let renderCount = 0; const Component = () => { let { data, loading, networkStatus, stopPolling } = useQuery(CAR_QUERY, { @@ -412,7 +416,7 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(2); - }); + }).then(resolve, reject); }); itAsync('should stop polling when skip is true', (resolve, reject) => { @@ -514,11 +518,11 @@ describe('useQuery Hook', () => { }).then(resolve, reject); }); - it( + itAsync( 'should not throw an error if `stopPolling` is called manually after ' + 'a component has unmounted (even though polling has already been ' + 'stopped automatically)', - async () => { + (resolve, reject) => { let unmount: any; let renderCount = 0; const Component = () => { @@ -553,7 +557,7 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(2); - }); + }).then(resolve, reject); } ); @@ -574,7 +578,7 @@ describe('useQuery Hook', () => { }); describe('Error handling', () => { - it("should render GraphQLError's", async () => { + itAsync("should render GraphQLError's", (resolve, reject) => { const query = gql` query TestQuery { rates(currency: "USD") { @@ -607,7 +611,7 @@ describe('useQuery Hook', () => { ); - return wait(); + return wait().then(resolve, reject); }); itAsync('should only call onError callbacks once', (resolve, reject) => { @@ -950,7 +954,7 @@ describe('useQuery Hook', () => { }).then(resolve, reject); }); - it('should render both success and errors (same error messages) with loading done on refetch', async () => { + itAsync('should render both success and errors (same error messages) with loading done on refetch', (resolve, reject) => { const mocks = [ { request: { query: CAR_QUERY }, @@ -1026,15 +1030,15 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(6); - }); + }).then(resolve, reject); }); }); describe('Pagination', () => { - it( + itAsync( 'should render `fetchMore.updateQuery` updated results with proper ' + 'loading status, when `notifyOnNetworkStatusChange` is true', - async () => { + (resolve, reject) => { const carQuery: DocumentNode = gql` query cars($limit: Int) { cars(limit: $limit) { @@ -1131,14 +1135,14 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(3); - }); + }).then(resolve, reject); } ); - it( + itAsync( 'should render `fetchMore.updateQuery` updated results with no ' + 'loading status, when `notifyOnNetworkStatusChange` is false', - async () => { + (resolve, reject) => { const carQuery: DocumentNode = gql` query cars($limit: Int) { cars(limit: $limit) { @@ -1230,7 +1234,7 @@ describe('useQuery Hook', () => { return wait(() => { expect(renderCount).toBe(3); - }); + }).then(resolve, reject); } ); }); @@ -1448,10 +1452,10 @@ describe('useQuery Hook', () => { }); describe('Callbacks', () => { - it( + itAsync( 'should pass loaded data to onCompleted when using the cache-only ' + 'fetch policy', - async () => { + (resolve, reject) => { const cache = new InMemoryCache(); const client = new ApolloClient({ cache, @@ -1486,11 +1490,11 @@ describe('useQuery Hook', () => { return wait(() => { expect(onCompletedCalled).toBeTruthy(); - }); + }).then(resolve, reject); } ); - it('should only call onCompleted once per query run', async () => { + itAsync('should only call onCompleted once per query run', (resolve, reject) => { const cache = new InMemoryCache(); const client = new ApolloClient({ cache, @@ -1524,7 +1528,7 @@ describe('useQuery Hook', () => { return wait(() => { expect(onCompletedCount).toBe(1); - }); + }).then(resolve, reject); }); }); }); From 92cfcb9b421457096ce51ff14bbd536ad48742f2 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 30 Apr 2020 12:05:13 -0400 Subject: [PATCH 3/5] Fix ObservableQuery tests. --- src/core/__tests__/ObservableQuery.ts | 406 ++++++++++---------------- 1 file changed, 147 insertions(+), 259 deletions(-) diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 5d4cbd2a4a8..bf35a596b04 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -182,7 +182,7 @@ describe('ObservableQuery', () => { const data2 = { allPeople: { people: [{ name: 'Leia Skywalker' }] } }; const variables2 = { first: 1 }; - const observable: ObservableQuery = mockWatchQuery( + const queryManager = mockQueryManager( reject, { request: { @@ -200,15 +200,23 @@ describe('ObservableQuery', () => { }, ); + const observable = queryManager.watchQuery({ + query: queryWithVars, + variables: variables1, + notifyOnNetworkStatusChange: true, + }); + subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(data); + expect(result.data).toEqual(data); + expect(result.loading).toBe(false); return observable.refetch(variables2); } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual(data); expect(result.loading).toBe(true); + expect(result.networkStatus).toBe(NetworkStatus.setVariables); } else if (handleCount === 3) { - expect(stripSymbols(result.data)).toEqual(data2); + expect(result.loading).toBe(false); + expect(result.data).toEqual(data2); resolve(); } }); @@ -231,7 +239,7 @@ describe('ObservableQuery', () => { const data2 = { allPeople: { people: [{ name: 'Leia Skywalker' }] } }; - const observable: ObservableQuery = mockWatchQuery( + const queryManager = mockQueryManager( reject, { request: { @@ -249,12 +257,23 @@ describe('ObservableQuery', () => { }, ); + const observable = queryManager.watchQuery({ + query, + variables, + notifyOnNetworkStatusChange: true, + }); + subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(data); + expect(result.loading).toEqual(false); + expect(result.data).toEqual(data); return observable.refetch(); } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual(data2); + expect(result.loading).toEqual(true); + expect(result.networkStatus).toEqual(NetworkStatus.refetch); + } else if (handleCount === 3) { + expect(result.loading).toEqual(false); + expect(result.data).toEqual(data2); resolve(); } }); @@ -298,16 +317,21 @@ describe('ObservableQuery', () => { subscribeAndCount(reject, observable, async (handleCount, result) => { if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(data); - await observable.setOptions({ variables: variables2 }); + expect(result.data).toEqual(data); + expect(result.loading).toBe(false); + await observable.setOptions({ + variables: variables2, + notifyOnNetworkStatusChange: true, + }); } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual(data); expect(result.loading).toBe(true); + expect(result.networkStatus).toBe(NetworkStatus.setVariables); } else if (handleCount === 3) { - expect(stripSymbols(result.data)).toEqual(data2); + expect(result.loading).toBe(false); + expect(result.data).toEqual(data2); // go back to first set of variables - const current = await observable.setOptions({ variables }); - expect(stripSymbols(current.data)).toEqual(data); + const current = await observable.reobserve({ variables }); + expect(current.data).toEqual(data); resolve(); } }); @@ -367,18 +391,18 @@ describe('ObservableQuery', () => { subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(dataOne); + expect(result.loading).toBe(false); + expect(result.data).toEqual(dataOne); return observable.setOptions({ fetchPolicy: 'network-only' }); } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual(dataTwo); + expect(result.loading).toBe(false); + expect(result.data).toEqual(dataTwo); resolve(); } }); }); itAsync('does a network request if fetchPolicy is cache-only then store is reset then fetchPolicy becomes not cache-only', (resolve, reject) => { - let queryManager: QueryManager; - let observable: ObservableQuery; const testQuery = gql` query { author { @@ -396,45 +420,37 @@ describe('ObservableQuery', () => { let timesFired = 0; const link: ApolloLink = ApolloLink.from([ - () => { - return new Observable(observer => { - timesFired += 1; - observer.next({ data }); - observer.complete(); - return; - }); - }, + () => new Observable(observer => { + timesFired += 1; + observer.next({ data }); + observer.complete(); + }), ]); - queryManager = createQueryManager({ link }); + + const queryManager = createQueryManager({ link }); // fetch first data from server - observable = queryManager.watchQuery({ query: testQuery }); + const observable = queryManager.watchQuery({ + query: testQuery, + }); subscribeAndCount(reject, observable, async (handleCount, result) => { - try { - if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(data); - expect(timesFired).toBe(1); - // set policy to be cache-only but data is found - await observable.setOptions({ fetchPolicy: 'cache-only' }); - await queryManager.resetStore(); - } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual({}); - expect(timesFired).toBe(1); - await observable.setOptions({ fetchPolicy: 'cache-first' }); - } else if (handleCount === 3) { - expect(stripSymbols(result.data)).toEqual(data); - expect(timesFired).toBe(2); - resolve(); - } - } catch (e) { - reject(e); + if (handleCount === 1) { + expect(result.data).toEqual(data); + expect(timesFired).toBe(1); + // set policy to be cache-only but data is found + await observable.setOptions({ fetchPolicy: 'cache-only' }); + await queryManager.resetStore(); + } else if (handleCount === 2) { + expect(result.data).toEqual({}); + expect(result.loading).toBe(false); + expect(result.networkStatus).toBe(NetworkStatus.ready); + expect(timesFired).toBe(1); + resolve(); } }); }); itAsync('does a network request if fetchPolicy changes from cache-only', (resolve, reject) => { - let queryManager: QueryManager; - let observable: ObservableQuery; const testQuery = gql` query { author { @@ -457,24 +473,27 @@ describe('ObservableQuery', () => { timesFired += 1; observer.next({ data }); observer.complete(); - return; }); }, ]); - queryManager = createQueryManager({ link }); - observable = queryManager.watchQuery({ + + const queryManager = createQueryManager({ link }); + + const observable = queryManager.watchQuery({ query: testQuery, fetchPolicy: 'cache-only', notifyOnNetworkStatusChange: false, }); - subscribeAndCount(reject, observable, async (handleCount, result) => { + subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual({}); + expect(result.loading).toBe(false); + expect(result.data).toEqual({}); expect(timesFired).toBe(0); - await observable.setOptions({ fetchPolicy: 'cache-first' }); + observable.setOptions({ fetchPolicy: 'cache-first' }); } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual(data); + expect(result.loading).toBe(false); + expect(result.data).toEqual(data); expect(timesFired).toBe(1); resolve(); } @@ -597,24 +616,22 @@ describe('ObservableQuery', () => { }, ); - subscribeAndCount(reject, observable, handleCount => { - if (handleCount !== 1) { - return; + subscribeAndCount(reject, observable, (handleCount, result) => { + if (handleCount === 1) { + expect(result.data).toEqual(dataOne); + observable.setOptions({ + fetchPolicy: 'cache-and-network', + }).then(res => { + expect(res.data).toEqual(dataTwo); + }).then(resolve, reject); } - observable - .setOptions({ fetchPolicy: 'cache-and-network' }) - .then(res => { - // returns dataOne from cache - expect(stripSymbols(res.data)).toEqual(dataOne); - resolve(); - }); }); }); }); describe('setVariables', () => { itAsync('reruns query if the variables change', (resolve, reject) => { - const observable: ObservableQuery = mockWatchQuery( + const queryManager = mockQueryManager( reject, { request: { query, variables }, @@ -626,13 +643,20 @@ describe('ObservableQuery', () => { }, ); + const observable = queryManager.watchQuery({ + query, + variables, + notifyOnNetworkStatusChange: true, + }); + subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { + expect(result.loading).toBe(false); expect(stripSymbols(result.data)).toEqual(dataOne); return observable.setVariables(differentVariables); } else if (handleCount === 2) { expect(result.loading).toBe(true); - expect(stripSymbols(result.data)).toEqual(dataOne); + expect(result.networkStatus).toBe(NetworkStatus.setVariables); } else if (handleCount === 3) { expect(result.loading).toBe(false); expect(stripSymbols(result.data)).toEqual(dataTwo); @@ -749,15 +773,13 @@ describe('ObservableQuery', () => { errorPolicy: 'all', }); - subscribeAndCount(reject, observable, async (handleCount, result) => { + subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { expect(result.errors).toEqual([error]); expect(observable.getCurrentResult().errors).toEqual([error]); observable.setVariables(differentVariables); expect(observable.getCurrentResult().errors).toEqual([error]); - } - // after loading is done and new results are returned - if (handleCount === 3) { + } else if (handleCount === 2) { expect(stripSymbols(result.data)).toEqual(dataTwo); expect(stripSymbols(observable.getCurrentResult().data)).toEqual( dataTwo, @@ -798,13 +820,13 @@ describe('ObservableQuery', () => { subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { + expect(result.loading).toBe(false); expect(stripSymbols(result.data)).toEqual(dataOne); expect(result.networkStatus).toBe(NetworkStatus.ready); observable.setVariables(differentVariables); } else if (handleCount === 2) { expect(result.loading).toBe(true); expect(result.networkStatus).toBe(NetworkStatus.setVariables); - expect(stripSymbols(result.data)).toEqual(dataOne); } else if (handleCount === 3) { expect(result.loading).toBe(false); expect(result.networkStatus).toBe(NetworkStatus.ready); @@ -836,13 +858,13 @@ describe('ObservableQuery', () => { subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { + expect(result.loading).toBe(false); expect(stripSymbols(result.data)).toEqual(dataOne); expect(result.networkStatus).toBe(NetworkStatus.ready); observable.refetch(differentVariables); } else if (handleCount === 2) { expect(result.loading).toBe(true); expect(result.networkStatus).toBe(NetworkStatus.setVariables); - expect(stripSymbols(result.data)).toEqual(dataOne); } else if (handleCount === 3) { expect(result.loading).toBe(false); expect(result.networkStatus).toBe(NetworkStatus.ready); @@ -852,68 +874,6 @@ describe('ObservableQuery', () => { }); }); - itAsync('reruns observer callback if the variables change but data does not', (resolve, reject) => { - const observable: ObservableQuery = mockWatchQuery( - reject, - { - request: { query, variables }, - result: { data: dataOne }, - }, - { - request: { query, variables: differentVariables }, - result: { data: dataOne }, - }, - ); - - subscribeAndCount(reject, observable, (handleCount, result) => { - if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(dataOne); - observable.setVariables(differentVariables); - } else if (handleCount === 2) { - expect(result.loading).toBe(true); - expect(stripSymbols(result.data)).toEqual(dataOne); - } else if (handleCount === 3) { - expect(stripSymbols(result.data)).toEqual(dataOne); - resolve(); - } - }); - }); - - itAsync('does not rerun observer callback if the variables change but new data is in store', (resolve, reject) => { - const manager = mockQueryManager( - reject, - { - request: { query, variables }, - result: { data: dataOne }, - }, - { - request: { query, variables: differentVariables }, - result: { data: dataOne }, - }, - ); - - manager.query({ query, variables: differentVariables }).then(() => { - const observable: ObservableQuery = manager.watchQuery({ - query, - variables, - notifyOnNetworkStatusChange: false, - }); - - let errored = false; - subscribeAndCount(reject, observable, (handleCount, result) => { - if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(dataOne); - observable.setVariables(differentVariables); - - // Nothing should happen, so we'll wait a moment to check that - setTimeout(() => !errored && resolve(), 10); - } else if (handleCount === 2) { - throw new Error('Observable callback should not fire twice'); - } - }); - }); - }); - itAsync('does not rerun query if variables do not change', (resolve, reject) => { const observable: ObservableQuery = mockWatchQuery( reject, @@ -942,34 +902,6 @@ describe('ObservableQuery', () => { }); }); - itAsync('does not rerun query if set to not refetch', (resolve, reject) => { - const observable: ObservableQuery = mockWatchQuery( - reject, - { - request: { query, variables }, - result: { data: dataOne }, - }, - { - request: { query, variables }, - result: { data: dataTwo }, - }, - ); - - let errored = false; - subscribeAndCount(reject, observable, (handleCount, result) => { - if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(dataOne); - observable.setVariables(variables, true, false); - - // Nothing should happen, so we'll wait a moment to check that - setTimeout(() => !errored && resolve(), 10); - } else if (handleCount === 2) { - errored = true; - throw new Error('Observable callback should not fire twice'); - } - }); - }); - itAsync('handles variables changing while a query is in-flight', (resolve, reject) => { // The expected behavior is that the original variables are forgotten // and the query stays in loading state until the result for the new variables @@ -988,7 +920,7 @@ describe('ObservableQuery', () => { }, ); - setTimeout(() => observable.setVariables(differentVariables), 10); + observable.setVariables(differentVariables); subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { @@ -996,12 +928,25 @@ describe('ObservableQuery', () => { expect(result.loading).toBe(false); expect(stripSymbols(result.data)).toEqual(dataTwo); resolve(); + } else { + reject(new Error("should not deliver more than one result")); } }); }); }); describe('refetch', () => { + type TFQO = QueryManager["fetchQueryObservable"]; + function mockFetchQuery(queryManager: QueryManager) { + const origFetchQuery: TFQO = (queryManager as any).fetchQueryObservable; + return (queryManager as any).fetchQueryObservable = jest.fn< + ReturnType, + Parameters + >(function () { + return origFetchQuery.apply(queryManager, arguments); + }); + } + itAsync('calls fetchRequest with fetchPolicy `network-only` when using a non-networked fetch policy', (resolve, reject) => { const mockedResponses = [ { @@ -1022,16 +967,13 @@ describe('ObservableQuery', () => { fetchPolicy: 'cache-first', }); - const origFetchQuery = queryManager.fetchQuery; - queryManager.fetchQuery = jest.fn(() => - origFetchQuery.apply(queryManager, arguments), - ); + const mocked = mockFetchQuery(queryManager); subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { observable.refetch(differentVariables); - } else if (handleCount === 3) { - expect(queryManager.fetchQuery.mock.calls[1][1].fetchPolicy).toEqual( + } else if (handleCount === 2) { + expect(mocked.mock.calls[1][1].fetchPolicy).toEqual( 'network-only', ); resolve(); @@ -1061,17 +1003,14 @@ describe('ObservableQuery', () => { fetchPolicy: 'no-cache', }); - const origFetchQuery = queryManager.fetchQuery; - queryManager.fetchQuery = jest.fn(() => - origFetchQuery.apply(queryManager, arguments), - ); + const mocked = mockFetchQuery(queryManager); subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { observable.refetch(differentVariables); } else if (handleCount === 2) { expect( - queryManager.fetchQuery.mock.calls[1][1].fetchPolicy, + mocked.mock.calls[1][1].fetchPolicy, ).toEqual('no-cache'); resolve(); } @@ -1097,7 +1036,7 @@ describe('ObservableQuery', () => { const data2 = { allPeople: { people: [{ name: 'Leia Skywalker' }] } }; const variables2 = { first: 1 }; - const observable: ObservableQuery = mockWatchQuery( + const queryManager = mockQueryManager( reject, { request: { @@ -1122,25 +1061,30 @@ describe('ObservableQuery', () => { }, ); - observable.setOptions({ fetchPolicy: 'cache-and-network' }); + const observable = queryManager.watchQuery({ + query: queryWithVars, + variables: variables1, + fetchPolicy: 'cache-and-network', + notifyOnNetworkStatusChange: true, + }); subscribeAndCount(reject, observable, (handleCount, result) => { if (handleCount === 1) { - expect(stripSymbols(result.data)).toEqual(data); + expect(result.data).toEqual(data); expect(result.loading).toBe(false); observable.refetch(variables2); } else if (handleCount === 2) { - expect(stripSymbols(result.data)).toEqual(data); expect(result.loading).toBe(true); + expect(result.networkStatus).toBe(NetworkStatus.setVariables); } else if (handleCount === 3) { - expect(stripSymbols(result.data)).toEqual(data2); + expect(result.data).toEqual(data2); expect(result.loading).toBe(false); observable.refetch(variables1); } else if (handleCount === 4) { - expect(stripSymbols(result.data)).toEqual(data2); expect(result.loading).toBe(true); + expect(result.networkStatus).toBe(NetworkStatus.setVariables); } else if (handleCount === 5) { - expect(stripSymbols(result.data)).toEqual(data); + expect(result.data).toEqual(data); expect(result.loading).toBe(false); resolve(); } @@ -1201,12 +1145,6 @@ describe('ObservableQuery', () => { ++handleCount; if (handleCount === 1) { - expect(result).toEqual({ - data: {}, - loading: true, - networkStatus: NetworkStatus.loading, - }); - } else if (handleCount === 2) { expect(result).toEqual({ data: { counter: 1, @@ -1214,7 +1152,7 @@ describe('ObservableQuery', () => { loading: true, networkStatus: NetworkStatus.loading, }); - } else if (handleCount === 3) { + } else if (handleCount === 2) { expect(result).toEqual({ data: { counter: 2, @@ -1240,16 +1178,7 @@ describe('ObservableQuery', () => { expect(error).toBe(intentionalNetworkFailure); }, ); - } else if (handleCount === 4) { - expect(result).toEqual({ - data: { - counter: 2, - name: 'Ben', - }, - loading: true, - networkStatus: NetworkStatus.refetch, - }); - } else if (handleCount === 5) { + } else if (handleCount === 3) { expect(result).toEqual({ data: { counter: 3, @@ -1260,7 +1189,7 @@ describe('ObservableQuery', () => { }); resolve(); - } else if (handleCount > 5) { + } else if (handleCount > 4) { reject(new Error('should not get here')); } }, @@ -1349,6 +1278,7 @@ describe('ObservableQuery', () => { cache: new InMemoryCache({ possibleTypes: { Creature: ['Pet'], + Pet: ['Dog', 'Cat'], }, }), }); @@ -1361,23 +1291,24 @@ describe('ObservableQuery', () => { subscribeAndCount(reject, observable, (count, result) => { const { data, loading, networkStatus } = observable.getCurrentResult(); - try { - expect(result).toEqual({ - data, - loading, - networkStatus, - }); - } catch (e) { - reject(e); - } + expect(result.loading).toEqual(loading); + expect(result.networkStatus).toEqual(networkStatus); + expect(result.data).toEqual(data); if (count === 1) { + expect(result.loading).toBe(false); + expect(result.networkStatus).toEqual(NetworkStatus.ready); + expect(result.data).toEqual(dataOneWithTypename); observable.refetch(); - } - if (count === 3) { + } else if (count === 2) { + expect(result.loading).toBe(true); + expect(result.networkStatus).toEqual(NetworkStatus.refetch); + } else if (count === 3) { + expect(result.loading).toBe(false); + expect(result.networkStatus).toEqual(NetworkStatus.ready); + expect(result.data).toEqual(dataTwoWithTypename); setTimeout(resolve, 5); - } - if (count > 3) { + } else { reject(new Error('Observable.next called too many times')); } }); @@ -1580,11 +1511,10 @@ describe('ObservableQuery', () => { // we can use this to trigger the query subscribeAndCount(reject, observable, (handleCount, subResult) => { const { data, loading, networkStatus } = observable.getCurrentResult(); - expect(subResult).toEqual({ - data, - loading, - networkStatus, - }); + + expect(subResult.data).toEqual(data); + expect(subResult.loading).toEqual(loading); + expect(subResult.networkStatus).toEqual(networkStatus); if (handleCount === 1) { expect(subResult).toEqual({ @@ -1867,48 +1797,6 @@ describe('ObservableQuery', () => { }); }); - describe('stopPolling', () => { - itAsync('does not restart polling after stopping and resubscribing', (resolve, reject) => { - const observable = mockWatchQuery( - reject, - { - request: { query, variables }, - result: { data: dataOne }, - }, - { - request: { query, variables }, - result: { data: dataTwo }, - }, - ); - - observable.startPolling(50); - observable.stopPolling(); - - let startedPolling = false; - subscribeAndCount(reject, observable, handleCount => { - if (handleCount === 1) { - // first call to subscribe is the immediate result when - // subscribing. later calls to this callback indicate that - // we will be polling. - - // Wait a bit to see if the subscription's `next` was called - // again, indicating that we are polling for data. - setTimeout(() => { - if (!startedPolling) { - // if we're not polling for data, it means this test - // is ok - resolve(); - } - }, 60); - } else if (handleCount === 2) { - // oops! we are polling for data, this should not happen. - startedPolling = true; - reject(new Error('should not start polling, already stopped')); - } - }); - }); - }); - describe('resetQueryStoreErrors', () => { itAsync("should remove any GraphQLError's stored in the query store", (resolve, reject) => { const graphQLError = new GraphQLError('oh no!'); From 972da4b5f737eedf5c2107507f2b3d263324a62d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 1 May 2020 13:47:06 -0400 Subject: [PATCH 4/5] Improve racy subscribeToMore tests. The use of setTimeout in tests is often an invitation to race conditions and timing-sensitive outcomes. This raciness became particularly problematic thanks to changes (between Node 10 and Node 12) in the timing of Promise callbacks with respect to setTimeout callbacks. These changes also leave all tests passing when cherry-picked onto master, so I'm confident I am *not* changing the tests just to fix my PR #6221, at the expense of backwards compatibility. --- src/__tests__/subscribeToMore.ts | 76 +++++++++++++++++--------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/src/__tests__/subscribeToMore.ts b/src/__tests__/subscribeToMore.ts index ae290f51fa5..69c1c0e0056 100644 --- a/src/__tests__/subscribeToMore.ts +++ b/src/__tests__/subscribeToMore.ts @@ -72,14 +72,23 @@ describe('subscribeToMore', () => { link, }); - const obsHandle = client.watchQuery({ + type TData = typeof req1['result']['data']; + const obsHandle = client.watchQuery({ query, }); const sub = obsHandle.subscribe({ - next(queryResult) { + next(queryResult: TData) { latestResult = queryResult; - counter++; + if (++counter === 3) { + sub.unsubscribe(); + expect(latestResult).toEqual({ + data: { entry: { value: 'Amanda Liu' } }, + loading: false, + networkStatus: 7, + }); + resolve(); + } }, }); @@ -94,20 +103,15 @@ describe('subscribeToMore', () => { }, }); - setTimeout(() => { - sub.unsubscribe(); - expect(counter).toBe(3); - expect(stripSymbols(latestResult)).toEqual({ - data: { entry: { value: 'Amanda Liu' } }, - loading: false, - networkStatus: 7, - }); - resolve(); - }, 15); - - for (let i = 0; i < 2; i++) { - wSLink.simulateResult(results[i]); + let i = 0; + function simulate() { + const result = results[i++]; + if (result) { + wSLink.simulateResult(result); + setTimeout(simulate, 10); + } } + simulate(); }); itAsync('calls error callback on error', (resolve, reject) => { @@ -351,18 +355,25 @@ describe('subscribeToMore', () => { link, }); - const obsHandle = client.watchQuery< - typeof typedReq['result']['data'], - typeof typedReq['request']['variables'] - >({ + type TData = typeof typedReq['result']['data']; + type TVars = typeof typedReq['request']['variables']; + const obsHandle = client.watchQuery({ query, variables: { someNumber: 1 }, }); const sub = obsHandle.subscribe({ - next(queryResult) { + next(queryResult: TData) { latestResult = queryResult; - counter++; + if (++counter === 3) { + sub.unsubscribe(); + expect(latestResult).toEqual({ + data: { entry: { value: 'Amanda Liu' } }, + loading: false, + networkStatus: 7, + }); + resolve(); + } }, }); @@ -380,19 +391,14 @@ describe('subscribeToMore', () => { }, }); - setTimeout(() => { - sub.unsubscribe(); - expect(counter).toBe(3); - expect(stripSymbols(latestResult)).toEqual({ - data: { entry: { value: 'Amanda Liu' } }, - loading: false, - networkStatus: 7, - }); - resolve(); - }, 15); - - for (let i = 0; i < 2; i++) { - wSLink.simulateResult(results[i]); + let i = 0; + function simulate() { + const result = results[i++]; + if (result) { + wSLink.simulateResult(result); + setTimeout(simulate, 10); + } } + simulate(); }); }); From dd86a7174998af45c6e7f04e44b8dfc2d08ac9de Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 1 May 2020 14:16:27 -0400 Subject: [PATCH 5/5] Prevent stray invariant failures in MockLink#request. --- src/utilities/testing/mocking/mockLink.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/utilities/testing/mocking/mockLink.ts b/src/utilities/testing/mocking/mockLink.ts index 2609a626818..8d82dbbfd09 100644 --- a/src/utilities/testing/mocking/mockLink.ts +++ b/src/utilities/testing/mocking/mockLink.ts @@ -90,10 +90,9 @@ export class MockLink extends ApolloLink { operation.query )}, variables: ${JSON.stringify(operation.variables)}` )); + return null; } - invariant(response, "mocked response is required"); - this.mockedResponsesByKey[key].splice(responseIndex, 1); const { newData } = response!;