From 4ce92da98178e5dc3792121fc76781da232bfb15 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 23 Jun 2020 15:22:20 -0700 Subject: [PATCH 01/15] libdefs: Prepare to edit `redux-mock-store`. In an upcoming commit, we'll change the Dispatch type in this libdef to play nicely with our async redux-thunk actions. I don't see an easy way to give the libdef the flexibility to give different types depending on what middleware is used, so, we hard-code it for ourselves, since we know we'll be using redux-thunk for the foreseeable future. This means we probably can't reasonably make a PR to FlowTyped, even though this is (surprisingly) one of the few libraries we use that does have a libdef hosted by FlowTyped. Ah, well. Here, we just move the file, delete the metadata at the top, and run our auto-formatting. [cherry-picked from #4171] --- flow-typed/npm/redux-mock-store_v1.2.x.js | 35 ----------------------- flow-typed/redux-mock-store_v1.2.x.js | 32 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 35 deletions(-) delete mode 100644 flow-typed/npm/redux-mock-store_v1.2.x.js create mode 100644 flow-typed/redux-mock-store_v1.2.x.js diff --git a/flow-typed/npm/redux-mock-store_v1.2.x.js b/flow-typed/npm/redux-mock-store_v1.2.x.js deleted file mode 100644 index 96e1ddb14c3..00000000000 --- a/flow-typed/npm/redux-mock-store_v1.2.x.js +++ /dev/null @@ -1,35 +0,0 @@ -// flow-typed signature: 6e958768eab6cb706d8c4ff13fd4d5ab -// flow-typed version: c6154227d1/redux-mock-store_v1.2.x/flow_>=v0.25.x <=v0.103.x - -declare module "redux-mock-store" { - /* - S = State - A = Action - */ - - declare type mockStore = { - (state: S): mockStoreWithoutMiddleware - }; - declare type DispatchAPI = (action: A) => A; - declare type Dispatch = DispatchAPI; - declare type mockStoreWithoutMiddleware = { - getState(): S, - getActions(): Array, - dispatch: Dispatch, - clearActions(): void, - subscribe(callback: () => void): () => void, - replaceReducer(nextReducer: Function): void - }; - - declare type Middleware = any => any => any; - - declare module.exports: (middlewares: ?Array) => mockStore; -} - -// Filename aliases -declare module "redux-mock-store/src/index" { - declare module.exports: $Exports<"redux-mock-store">; -} -declare module "redux-mock-store/src/index.js" { - declare module.exports: $Exports<"redux-mock-store">; -} diff --git a/flow-typed/redux-mock-store_v1.2.x.js b/flow-typed/redux-mock-store_v1.2.x.js new file mode 100644 index 00000000000..2d44c368f31 --- /dev/null +++ b/flow-typed/redux-mock-store_v1.2.x.js @@ -0,0 +1,32 @@ +declare module 'redux-mock-store' { + /* + S = State + A = Action + */ + + declare type mockStore = { + (state: S): mockStoreWithoutMiddleware, + }; + declare type DispatchAPI = (action: A) => A; + declare type Dispatch = DispatchAPI; + declare type mockStoreWithoutMiddleware = { + getState(): S, + getActions(): Array, + dispatch: Dispatch, + clearActions(): void, + subscribe(callback: () => void): () => void, + replaceReducer(nextReducer: Function): void, + }; + + declare type Middleware = (any) => any => any; + + declare module.exports: (middlewares: ?Array) => mockStore; +} + +// Filename aliases +declare module 'redux-mock-store/src/index' { + declare module.exports: $Exports<'redux-mock-store'>; +} +declare module 'redux-mock-store/src/index.js' { + declare module.exports: $Exports<'redux-mock-store'>; +} From 967ddc7daa8a78bd5113dbfb3bccafb79b01a955 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 23 Jun 2020 15:30:43 -0700 Subject: [PATCH 02/15] libdefs: Copy new version of redux-mock-store libdef. This version [1] is marked for a higher version of Flow than we're using now (v0.104 and above), but there aren't any errors on our Flow version (v0.98). So, take it. This gets us a particular change to the Dispatch type [2] that goes in the right direction...until we clobber it with our own Dispatch that accounts for async redux-thunk actions, I suppose. Ah well, there are a few minor changes that we might like to have, and this is a later version, so, might as well. [1]: https://github.com/flow-typed/flow-typed/blob/c4f47bdda/definitions/npm/redux-mock-store_v1.2.x/flow_v0.104.x-/redux-mock-store_v1.2.x.js [2]: https://github.com/flow-typed/flow-typed/pull/3803 [cherry-picked from #4171] --- flow-typed/redux-mock-store_v1.2.x.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/flow-typed/redux-mock-store_v1.2.x.js b/flow-typed/redux-mock-store_v1.2.x.js index 2d44c368f31..5d8bd01d042 100644 --- a/flow-typed/redux-mock-store_v1.2.x.js +++ b/flow-typed/redux-mock-store_v1.2.x.js @@ -4,11 +4,9 @@ declare module 'redux-mock-store' { A = Action */ - declare type mockStore = { - (state: S): mockStoreWithoutMiddleware, - }; + declare type mockStore = { (state: S): mockStoreWithoutMiddleware, ... }; declare type DispatchAPI = (action: A) => A; - declare type Dispatch = DispatchAPI; + declare type Dispatch> = DispatchAPI; declare type mockStoreWithoutMiddleware = { getState(): S, getActions(): Array, @@ -16,6 +14,7 @@ declare module 'redux-mock-store' { clearActions(): void, subscribe(callback: () => void): () => void, replaceReducer(nextReducer: Function): void, + ... }; declare type Middleware = (any) => any => any; From 660de30b0630788cb15b3254a4fd0eb3f28a658e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 23 Jun 2020 15:42:19 -0700 Subject: [PATCH 03/15] libdefs: Hard-code redux-thunk support into redux-mock-store. As foreshadowed in the preceding commits, we can't really expect a FlowTyped-hosted libdef to have the flexibility to give us the right types for Dispatch on the condition that we pass `thunk` as a piece of middleware. We're decidedly using `thunk`, so, hard-code support for it here by allowing dispatch to be called with a redux-thunk async action. We're pretty consistent with having correct types for such an action, so not much is lost by typing its arguments loosely as `Function, Function`, and from some attempts, it seems much easier than nailing down something exactly right. [cherry-picked from #4171] --- flow-typed/redux-mock-store_v1.2.x.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/flow-typed/redux-mock-store_v1.2.x.js b/flow-typed/redux-mock-store_v1.2.x.js index 5d8bd01d042..50eba86f8c5 100644 --- a/flow-typed/redux-mock-store_v1.2.x.js +++ b/flow-typed/redux-mock-store_v1.2.x.js @@ -5,12 +5,15 @@ declare module 'redux-mock-store' { */ declare type mockStore = { (state: S): mockStoreWithoutMiddleware, ... }; - declare type DispatchAPI = (action: A) => A; - declare type Dispatch> = DispatchAPI; + declare interface Dispatch { + (action: A): A; + ((Function, Function) => T): T; + } + declare type mockStoreWithoutMiddleware = { getState(): S, getActions(): Array, - dispatch: Dispatch, + dispatch: Dispatch, clearActions(): void, subscribe(callback: () => void): () => void, replaceReducer(nextReducer: Function): void, From 0e5533948286f81d94908053c4a772c88eba27be Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 23 Jun 2020 15:53:45 -0700 Subject: [PATCH 04/15] mocks: Remove __mocks__/redux-mock-store.js. It looks like this was added with the idea that we'd use it in more places than we actually have. It adds a slight bit of convenience; with this mock, we can avoid having to write `configureStore([thunk])` a bunch of times. But that's a very small amount of boilerplate. The mock doesn't seem to have any other purpose that might be part of a unit-testing strategy. Removing it makes it possible to use the libdef without contorting it more drastically than we already have. [cherry-picked from #4171] --- __mocks__/redux-mock-store.js | 6 ------ src/message/__tests__/fetchActions-test.js | 5 ++++- src/message/__tests__/messageActions-test.js | 5 ++++- 3 files changed, 8 insertions(+), 8 deletions(-) delete mode 100644 __mocks__/redux-mock-store.js diff --git a/__mocks__/redux-mock-store.js b/__mocks__/redux-mock-store.js deleted file mode 100644 index 6282d915afd..00000000000 --- a/__mocks__/redux-mock-store.js +++ /dev/null @@ -1,6 +0,0 @@ -import configureMockStore from 'redux-mock-store'; // eslint-disable-line -import thunk from 'redux-thunk'; - -const mockStore = configureMockStore([thunk]); - -export default mockStore; diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 4bc5901497d..2c3ab150460 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -1,9 +1,12 @@ -import mockStore from 'redux-mock-store'; // eslint-disable-line +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; import { fetchMessages, fetchOlder, fetchNewer } from '../fetchActions'; import { streamNarrow, HOME_NARROW, HOME_NARROW_STR } from '../../utils/narrow'; import { navStateWithNarrow } from '../../utils/testHelpers'; +const mockStore = configureStore([thunk]); + const narrow = streamNarrow('some stream'); const streamNarrowStr = JSON.stringify(narrow); diff --git a/src/message/__tests__/messageActions-test.js b/src/message/__tests__/messageActions-test.js index 1e2d58e13ca..b8a7934acf1 100644 --- a/src/message/__tests__/messageActions-test.js +++ b/src/message/__tests__/messageActions-test.js @@ -1,9 +1,12 @@ -import mockStore from 'redux-mock-store'; // eslint-disable-line +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; import { doNarrow } from '../messagesActions'; import { streamNarrow, HOME_NARROW, privateNarrow } from '../../utils/narrow'; import { navStateWithNarrow } from '../../utils/testHelpers'; +const mockStore = configureStore([thunk]); + const streamNarrowObj = streamNarrow('some stream'); const streamNarrowStr = JSON.stringify(streamNarrowObj); From bf11c245452a75b424e3b04494e71632000bb051 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 23 Jun 2020 16:01:03 -0700 Subject: [PATCH 05/15] test data: Well-type `navStateWithNarrow`; make more representative. The added fields were gleaned from a real-life example observed with `redux-logger`. [cherry-picked from #4171] --- src/utils/testHelpers.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/utils/testHelpers.js b/src/utils/testHelpers.js index 57f7e0a7a65..e913b89283a 100644 --- a/src/utils/testHelpers.js +++ b/src/utils/testHelpers.js @@ -1,11 +1,20 @@ /* @flow strict-local */ import type { Narrow } from '../types'; +import type { NavigationState } from '../reduxTypes'; -export const navStateWithNarrow = (narrow: Narrow): {| nav: mixed |} => ({ +export const navStateWithNarrow = (narrow: Narrow): {| nav: NavigationState |} => ({ nav: { - index: 0, + key: 'StackRouterRoot', + index: 1, + isTransitioning: false, routes: [ { + key: 'id-1592948746166-1', + params: {}, + routeName: 'main', + }, + { + key: 'id-1592948746166-2', routeName: 'chat', params: { narrow, From 03637c10b2a4b9b259bf02a35b97bce9f40755e5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Jul 2020 10:39:57 -0700 Subject: [PATCH 06/15] messageActions tests: Start type-checking, use example data. --- src/message/__tests__/messageActions-test.js | 184 +++++++++---------- 1 file changed, 91 insertions(+), 93 deletions(-) diff --git a/src/message/__tests__/messageActions-test.js b/src/message/__tests__/messageActions-test.js index b8a7934acf1..999ba875293 100644 --- a/src/message/__tests__/messageActions-test.js +++ b/src/message/__tests__/messageActions-test.js @@ -1,9 +1,13 @@ +/* @flow strict-local */ import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import { doNarrow } from '../messagesActions'; import { streamNarrow, HOME_NARROW, privateNarrow } from '../../utils/narrow'; import { navStateWithNarrow } from '../../utils/testHelpers'; +import * as eg from '../../__tests__/lib/exampleData'; +import type { GlobalState } from '../../reduxTypes'; +import type { Action } from '../../actionTypes'; const mockStore = configureStore([thunk]); @@ -13,54 +17,51 @@ const streamNarrowStr = JSON.stringify(streamNarrowObj); describe('messageActions', () => { describe('doNarrow', () => { test('when no messages in new narrow and caughtUp is false, actions to fetch messages and switch narrow are dispatched', () => { - const store = mockStore({ - realm: {}, - session: { isHydrated: true }, - caughtUp: { - [streamNarrowStr]: { - newer: false, - older: true, + const store = mockStore( + eg.reduxState({ + session: { ...eg.baseReduxState.session, isHydrated: true }, + caughtUp: { + [streamNarrowStr]: { + newer: false, + older: true, + }, }, - }, - ...navStateWithNarrow(HOME_NARROW), - narrows: {}, - streams: [ - { - name: 'some stream', - }, - ], - }); + ...navStateWithNarrow(HOME_NARROW), + narrows: {}, + streams: [eg.makeStream({ name: 'some stream' })], + }), + ); store.dispatch(doNarrow(streamNarrowObj)); const actions = store.getActions(); expect(actions).toHaveLength(3); - expect(actions[0].type).toBe('DO_NARROW'); - expect(actions[1].type).toBe('MESSAGE_FETCH_START'); - expect(actions[1].narrow).toEqual(streamNarrowObj); - expect(actions[2].type).toBe('Navigation/PUSH'); + const [action0, action1, action2] = actions; + expect(action0.type).toBe('DO_NARROW'); + expect(action1.type).toBe('MESSAGE_FETCH_START'); + if (action1.type === 'MESSAGE_FETCH_START') { + expect(action1.narrow).toEqual(streamNarrowObj); + } + expect(action2.type).toBe('Navigation/PUSH'); }); test('when no messages in new narrow but caught up, only switch to narrow, do not fetch', () => { - const store = mockStore({ - realm: {}, - session: { isHydrated: true }, - caughtUp: { - [streamNarrowStr]: { - newer: true, - older: true, + const store = mockStore( + eg.reduxState({ + session: { ...eg.baseReduxState.session, isHydrated: true }, + caughtUp: { + [streamNarrowStr]: { + newer: true, + older: true, + }, }, - }, - ...navStateWithNarrow(HOME_NARROW), - narrows: { - [streamNarrowStr]: [], - }, - streams: [ - { - name: 'some stream', + ...navStateWithNarrow(HOME_NARROW), + narrows: { + [streamNarrowStr]: [], }, - ], - }); + streams: [eg.makeStream({ name: 'some stream' })], + }), + ); store.dispatch(doNarrow(streamNarrowObj)); const actions = store.getActions(); @@ -76,52 +77,49 @@ describe('messageActions', () => { }); test('when messages in new narrow are too few and not caught up, fetch more messages', () => { - const store = mockStore({ - realm: {}, - session: { isHydrated: true }, - caughtUp: {}, - ...navStateWithNarrow(HOME_NARROW), - narrows: { - [streamNarrowStr]: [1], - }, - messages: { - 1: {}, - }, - streams: [ - { - name: 'some stream', + const store = mockStore( + eg.reduxState({ + session: { ...eg.baseReduxState.session, isHydrated: true }, + caughtUp: {}, + ...navStateWithNarrow(HOME_NARROW), + narrows: { + [streamNarrowStr]: [1], + }, + messages: { + [1]: eg.streamMessage() /* eslint-disable-line no-useless-computed-key */, }, - ], - }); + streams: [eg.makeStream({ name: 'some stream' })], + }), + ); store.dispatch(doNarrow(streamNarrowObj)); const actions = store.getActions(); expect(actions).toHaveLength(3); - expect(actions[0].type).toBe('DO_NARROW'); - expect(actions[1].type).toBe('MESSAGE_FETCH_START'); - expect(actions[1].narrow).toEqual(streamNarrowObj); - expect(actions[2].type).toBe('Navigation/PUSH'); + const [action0, action1, action2] = actions; + expect(action0.type).toBe('DO_NARROW'); + expect(action1.type).toBe('MESSAGE_FETCH_START'); + if (action1.type === 'MESSAGE_FETCH_START') { + expect(action1.narrow).toEqual(streamNarrowObj); + } + expect(action2.type).toBe('Navigation/PUSH'); }); test('if new narrow stream is not valid, do nothing', () => { - const store = mockStore({ - realm: {}, - session: { isHydrated: true }, - caughtUp: {}, - ...navStateWithNarrow(HOME_NARROW), - narrows: { - [streamNarrowStr]: [1], - }, - messages: { - 1: {}, - }, - streams: [ - { - name: 'some updated stream', + const store = mockStore( + eg.reduxState({ + session: { ...eg.baseReduxState.session, isHydrated: true }, + caughtUp: {}, + ...navStateWithNarrow(HOME_NARROW), + narrows: { + [streamNarrowStr]: [1], }, - ], - }); + messages: { + [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, + }, + streams: [eg.makeStream({ name: 'some updated stream' })], + }), + ); store.dispatch(doNarrow(streamNarrowObj)); const actions = store.getActions(); @@ -130,28 +128,28 @@ describe('messageActions', () => { }); test('if new narrow user is deactivated, do nothing', () => { - const store = mockStore({ - realm: { - crossRealmBots: [], - nonActiveUsers: [], - }, - session: { isHydrated: true }, - caughtUp: {}, - ...navStateWithNarrow(HOME_NARROW), - narrows: { - [streamNarrowStr]: [1], - }, - messages: { - 1: {}, - }, - users: [ - { - email: 'ab@a.com', + const inactiveUser = eg.makeUser({ name: 'inactive', is_active: false }); + + const store = mockStore( + eg.reduxState({ + realm: eg.realmState({ + crossRealmBots: [], + nonActiveUsers: [], // TODO: Shouldn't `inactiveUser` go here? + }), + session: { ...eg.baseReduxState.session, isHydrated: true }, + caughtUp: {}, + ...navStateWithNarrow(HOME_NARROW), + narrows: { + [streamNarrowStr]: [1], + }, + messages: { + [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, }, - ], - }); + users: [eg.makeUser({ name: 'bob', is_active: true })], + }), + ); - store.dispatch(doNarrow(privateNarrow('a@a.com'))); + store.dispatch(doNarrow(privateNarrow(inactiveUser.email))); const actions = store.getActions(); expect(actions).toHaveLength(0); From 27e20aa5676c65e36752259c8fe88a6c5d5c6a51 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Jul 2020 18:04:56 -0700 Subject: [PATCH 07/15] redux types: Make `MessagesState` inexact, for now. --- src/reduxTypes.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/reduxTypes.js b/src/reduxTypes.js index a7533442acb..d1466a3d086 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -153,9 +153,11 @@ export type FlagName = $Keys; * See also `NarrowsState`, which is an index on this data that identifies * messages belonging to a given narrow. */ -export type MessagesState = {| +export type MessagesState = { + // MessagesState should be exact; we're waiting for Flow v0.111.0. See note + // in src/utils/jsonable.js. [id: number]: $Exact, -|}; +}; export type MigrationsState = {| version?: string, From 55d6461922bbdf464e08c253bbbbf49e5156e2c2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Jul 2020 11:33:46 -0700 Subject: [PATCH 08/15] narrowsReducer tests: Remove a test that won't type-check. Flow won't allow these inputs (a GlobalState of `undefined`, or an action of `{}`). --- src/chat/__tests__/narrowsReducer-test.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 5484b5c451e..d82ff1dbacd 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -25,11 +25,6 @@ describe('narrowsReducer', () => { const streamNarrowStr = JSON.stringify(streamNarrow('some stream')); const topicNarrowStr = JSON.stringify(topicNarrow('some stream', 'some topic')); - test('handles unknown action and no previous state by returning initial state', () => { - const newState = narrowsReducer(undefined, {}); - expect(newState).toBeDefined(); - }); - describe('EVENT_NEW_MESSAGE', () => { test('if not caught up in narrow, do not add message in home narrow', () => { const initialState = deepFreeze({ From 4077e8880658cb5bd94876c7325580eee7f486f2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Jul 2020 16:07:29 -0700 Subject: [PATCH 09/15] narrowsReducer tests: Start typechecking, use example data. --- src/chat/__tests__/narrowsReducer-test.js | 187 +++++++++++++++------- 1 file changed, 126 insertions(+), 61 deletions(-) diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index d82ff1dbacd..45c344434a4 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -1,3 +1,4 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; import narrowsReducer from '../narrowsReducer'; @@ -31,9 +32,17 @@ describe('narrowsReducer', () => { [HOME_NARROW_STR]: [1, 2], }); + const message = eg.streamMessage({ + id: 3, + display_recipient: 'some stream', + subject: 'some topic', + flags: [], + }); + const action = deepFreeze({ + ...eg.eventNewMessageActionBase, type: EVENT_NEW_MESSAGE, - message: { id: 3, display_recipient: 'some stream', subject: 'some topic', flags: [] }, + message, caughtUp: {}, }); @@ -51,9 +60,11 @@ describe('narrowsReducer', () => { [HOME_NARROW_STR]: [1, 2], }); + const message = eg.streamMessage({ id: 3, flags: [] }); + const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { id: 3, flags: [] }, + ...eg.eventNewMessageActionBase, + message, caughtUp: { [HOME_NARROW_STR]: { older: false, @@ -74,25 +85,26 @@ describe('narrowsReducer', () => { test('if new message key does not exist do not create it', () => { const initialState = deepFreeze({ - [JSON.stringify(topicNarrow('some stream', 'some topic'))]: [1, 2], + [topicNarrowStr]: [1, 2], + }); + + const message = eg.streamMessage({ + id: 3, + flags: [], + display_recipient: 'stream', + subject: 'topic', }); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 3, - type: 'stream', - display_recipient: 'stream', - subject: 'topic', - flags: [], - }, + ...eg.eventNewMessageActionBase, + message, caughtUp: {}, }); const newState = narrowsReducer(initialState, action); const expectedState = { - [JSON.stringify(topicNarrow('some stream', 'some topic'))]: [1, 2], + [topicNarrowStr]: [1, 2], }; expect(newState).toEqual(expectedState); }); @@ -102,14 +114,13 @@ describe('narrowsReducer', () => { const initialState = deepFreeze({ [ALL_PRIVATE_NARROW_STR]: [], }); - const message = { + const message = eg.pmMessage({ id: 1, - type: 'private', display_recipient: [{ email: 'me@example.com' }, { email: 'a@a.com' }, { email: 'b@b.com' }], flags: [], - }; + }); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, + ...eg.eventNewMessageActionBase, message, ownEmail: 'me@example.com', caughtUp: { @@ -130,15 +141,16 @@ describe('narrowsReducer', () => { [HOME_NARROW_STR]: [1, 2], [topicNarrowStr]: [2], }); - const message = { + + const message = eg.streamMessage({ id: 3, - type: 'stream', + flags: [], display_recipient: 'some stream', subject: 'some topic', - flags: [], - }; + }); + const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, + ...eg.eventNewMessageActionBase, message, caughtUp: { [HOME_NARROW_STR]: { @@ -153,7 +165,7 @@ describe('narrowsReducer', () => { }); const expectedState = { [HOME_NARROW_STR]: [1, 2], - [topicNarrowStr]: [2, 3], + [topicNarrowStr]: [2, message.id], }; const newState = narrowsReducer(initialState, action); @@ -168,13 +180,14 @@ describe('narrowsReducer', () => { [narrowWithSelfStr]: [], }); - const message = { + const message = eg.pmMessage({ id: 1, display_recipient: [{ email: 'me@example.com' }], flags: [], - }; + }); + const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, + ...eg.eventNewMessageActionBase, ownEmail: 'me@example.com', message, caughtUp: { @@ -184,8 +197,8 @@ describe('narrowsReducer', () => { }); const expectedState = { - [HOME_NARROW_STR]: [1], - [narrowWithSelfStr]: [1], + [HOME_NARROW_STR]: [message.id], + [narrowWithSelfStr]: [message.id], }; const newState = narrowsReducer(initialState, action); @@ -204,15 +217,15 @@ describe('narrowsReducer', () => { [groupNarrowStr]: [2, 4], }); - const message = { + const message = eg.streamMessage({ id: 5, - type: 'stream', + flags: [], display_recipient: 'some stream', subject: 'some topic', - flags: [], - }; + }); + const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, + ...eg.eventNewMessageActionBase, message, caughtUp: { [HOME_NARROW_STR]: { older: false, newer: true }, @@ -222,10 +235,10 @@ describe('narrowsReducer', () => { }); const expectedState = { - [HOME_NARROW_STR]: [1, 2, 5], + [HOME_NARROW_STR]: [1, 2, message.id], [ALL_PRIVATE_NARROW_STR]: [1, 2], - [streamNarrowStr]: [2, 3, 5], - [topicNarrowStr]: [2, 3, 5], + [streamNarrowStr]: [2, 3, message.id], + [topicNarrowStr]: [2, 3, message.id], [privateNarrowStr]: [2, 4], [groupNarrowStr]: [2, 4], }; @@ -241,15 +254,10 @@ describe('narrowsReducer', () => { [HOME_NARROW_STR]: [1], }); - const message = { - id: 3, - type: 'stream', - display_recipient: 'stream name', - subject: 'some topic', - flags: [], - }; + const message = eg.streamMessage({ id: 3, flags: [] }); + const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, + ...eg.eventNewMessageActionBase, message, caughtUp: { [HOME_NARROW_STR]: { older: false, newer: true }, @@ -257,7 +265,7 @@ describe('narrowsReducer', () => { }); const expectedState = { - [HOME_NARROW_STR]: [1, 3], + [HOME_NARROW_STR]: [1, message.id], }; const newState = narrowsReducer(initialState, action); @@ -276,17 +284,11 @@ describe('narrowsReducer', () => { [groupNarrowStr]: [2, 4], }); - const message = { - id: 5, - type: 'private', - sender_email: 'someone@example.com', - display_recipient: [{ email: 'me@example.com' }, { email: 'mark@example.com' }], - flags: [], - }; + const message = eg.pmMessage({ id: 5, flags: [] }); + const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, + ...eg.eventNewMessageActionBase, message, - ownEmail: 'me@example.com', caughtUp: { [HOME_NARROW_STR]: { older: false, newer: true }, [ALL_PRIVATE_NARROW_STR]: { older: false, newer: true }, @@ -295,11 +297,11 @@ describe('narrowsReducer', () => { }); const expectedState = { - [HOME_NARROW_STR]: [1, 2, 5], - [ALL_PRIVATE_NARROW_STR]: [1, 2, 5], + [HOME_NARROW_STR]: [1, 2, message.id], + [ALL_PRIVATE_NARROW_STR]: [1, 2, message.id], [streamNarrowStr]: [2, 3], [topicNarrowStr]: [2, 3], - [privateNarrowStr]: [2, 4, 5], + [privateNarrowStr]: [2, 4, message.id], [groupNarrowStr]: [2, 4], }; @@ -370,11 +372,18 @@ describe('narrowsReducer', () => { const initialState = deepFreeze({ [HOME_NARROW_STR]: [1, 2, 3], }); + const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, + anchor: 2, narrow: privateNarrow('mark@example.com'), messages: [], + numBefore: 100, + numAfter: 100, + foundNewest: false, + foundOldest: false, }); + const expectedState = { [HOME_NARROW_STR]: [1, 2, 3], [JSON.stringify(privateNarrow('mark@example.com'))]: [], @@ -392,8 +401,17 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, + anchor: 2, narrow: [], - messages: [{ id: 2 }, { id: 3 }, { id: 4 }], + messages: [ + eg.streamMessage({ id: 2 }), + eg.streamMessage({ id: 3 }), + eg.streamMessage({ id: 4 }), + ], + numBefore: 100, + numAfter: 100, + foundNewest: false, + foundOldest: false, }); const expectedState = { @@ -413,8 +431,16 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, + anchor: 2, narrow: [], - messages: [{ id: 4, timestamp: 2 }, { id: 3, timestamp: 1 }], + messages: [ + eg.streamMessage({ id: 3, timestamp: 2 }), + eg.streamMessage({ id: 4, timestamp: 1 }), + ], + numBefore: 100, + numAfter: 100, + foundNewest: false, + foundOldest: false, }); const expectedState = { @@ -436,7 +462,14 @@ describe('narrowsReducer', () => { anchor: FIRST_UNREAD_ANCHOR, type: MESSAGE_FETCH_COMPLETE, narrow: [], - messages: [{ id: 3, timestamp: 2 }, { id: 4, timestamp: 1 }], + messages: [ + eg.streamMessage({ id: 3, timestamp: 2 }), + eg.streamMessage({ id: 4, timestamp: 1 }), + ], + numBefore: 100, + numAfter: 100, + foundNewest: false, + foundOldest: false, }); const expectedState = { @@ -457,7 +490,14 @@ describe('narrowsReducer', () => { anchor: LAST_MESSAGE_ANCHOR, type: MESSAGE_FETCH_COMPLETE, narrow: [], - messages: [{ id: 3, timestamp: 2 }, { id: 4, timestamp: 1 }], + messages: [ + eg.streamMessage({ id: 3, timestamp: 2 }), + eg.streamMessage({ id: 4, timestamp: 1 }), + ], + numBefore: 100, + numAfter: 0, + foundNewest: true, + foundOldest: false, }); const expectedState = { @@ -486,6 +526,14 @@ describe('narrowsReducer', () => { }); describe('EVENT_UPDATE_MESSAGE_FLAGS', () => { + const allMessages = { + // Flow doesn't like number literals as keys...but it also wants + // them to be numbers. See https://github.com/facebook/flow/issues/380. + [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, + [2]: eg.streamMessage({ id: 2 }) /* eslint-disable-line no-useless-computed-key */, + [4]: eg.streamMessage({ id: 4 }) /* eslint-disable-line no-useless-computed-key */, + }; + test('Do nothing if the is:starred narrow has not been fetched', () => { const initialState = deepFreeze({ [HOME_NARROW_STR]: [1, 2], @@ -493,6 +541,12 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, + id: 1, + allMessages, + all: false, + flag: 'starred', + operation: 'add', + messages: [4, 2], }); const newState = narrowsReducer(initialState, action); @@ -507,6 +561,11 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, + id: 1, + all: false, + allMessages, + operation: 'add', + messages: [1], flag: 'read', }); @@ -525,6 +584,9 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, + id: 1, + allMessages, + all: false, flag: 'starred', operation: 'add', messages: [4, 2], @@ -550,6 +612,9 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, + id: 1, + allMessages, + all: false, flag: 'starred', operation: 'remove', messages: [4, 2], From 5a99b210e8064d51e958eb37ae8f9815a2b84800 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Jul 2020 17:38:10 -0700 Subject: [PATCH 10/15] narrowsSelectors tests [nfc]: Remove confusing `expectedState` variables. `expectedState` makes sense in tests for reducers, but not really for selectors. The *input* of a selector is the GlobalState, but the output is...whatever we want to do with that input. Sometimes we just pick things directly from the state; sometimes we do that and then nudge it around until it's in a shape we want. Also rename `anchor` to the more general `result`; I'm not really sure what "anchor" is meant to refer to in each of these instances. --- src/chat/__tests__/narrowsSelectors-test.js | 40 +++++++++------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 775c953c00b..6ae6e45b819 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -30,11 +30,9 @@ describe('getMessagesForNarrow', () => { outbox: [], }); - const expectedState = deepFreeze([state.messages[123]]); + const result = getMessagesForNarrow(state, HOME_NARROW); - const anchor = getMessagesForNarrow(state, HOME_NARROW); - - expect(anchor).toEqual(expectedState); + expect(result).toEqual([state.messages[123]]); }); test('combine messages and outbox in same narrow', () => { @@ -59,9 +57,9 @@ describe('getMessagesForNarrow', () => { }, }); - const anchor = getMessagesForNarrow(state, HOME_NARROW); + const result = getMessagesForNarrow(state, HOME_NARROW); - const expectedState = deepFreeze([ + expect(result).toEqual([ { id: 123 }, { email: 'donald@zulip.com', @@ -71,8 +69,6 @@ describe('getMessagesForNarrow', () => { timestamp: 12, }, ]); - - expect(anchor).toEqual(expectedState); }); test('do not combine messages and outbox if not caught up', () => { @@ -94,11 +90,9 @@ describe('getMessagesForNarrow', () => { ], }); - const expectedState = deepFreeze([state.messages[123]]); + const result = getMessagesForNarrow(state, HOME_NARROW); - const anchor = getMessagesForNarrow(state, HOME_NARROW); - - expect(anchor).toEqual(expectedState); + expect(result).toEqual([state.messages[123]]); }); test('do not combine messages and outbox in different narrow', () => { @@ -120,11 +114,9 @@ describe('getMessagesForNarrow', () => { ], }); - const anchor = getMessagesForNarrow(state, privateNarrow('john@example.com')); - - const expectedState = deepFreeze([{ id: 123 }]); + const result = getMessagesForNarrow(state, privateNarrow('john@example.com')); - expect(anchor).toEqual(expectedState); + expect(result).toEqual([{ id: 123 }]); }); }); @@ -137,9 +129,9 @@ describe('getFirstMessageId', () => { outbox: [], }); - const anchor = getFirstMessageId(state, HOME_NARROW); + const result = getFirstMessageId(state, HOME_NARROW); - expect(anchor).toEqual(undefined); + expect(result).toEqual(undefined); }); test('returns first message id', () => { @@ -155,9 +147,9 @@ describe('getFirstMessageId', () => { outbox: [], }); - const anchor = getFirstMessageId(state, HOME_NARROW); + const result = getFirstMessageId(state, HOME_NARROW); - expect(anchor).toEqual(1); + expect(result).toEqual(1); }); }); @@ -171,9 +163,9 @@ describe('getLastMessageId', () => { outbox: [], }); - const anchor = getLastMessageId(state, HOME_NARROW); + const result = getLastMessageId(state, HOME_NARROW); - expect(anchor).toEqual(undefined); + expect(result).toEqual(undefined); }); test('returns last message id', () => { @@ -189,9 +181,9 @@ describe('getLastMessageId', () => { outbox: [], }); - const anchor = getLastMessageId(state, HOME_NARROW); + const result = getLastMessageId(state, HOME_NARROW); - expect(anchor).toEqual(3); + expect(result).toEqual(3); }); }); From 3ecf4456ce60853f2eb3ab314ef39357d9737c8a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Jul 2020 09:37:45 -0700 Subject: [PATCH 11/15] narrowsSelectors tests: Remove a test that won't type-check. --- src/chat/__tests__/narrowsSelectors-test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 6ae6e45b819..a1f7f68189e 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -215,7 +215,6 @@ describe('getStreamInNarrow', () => { }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { - expect(getStreamInNarrow(state, undefined)).toEqual(NULL_SUBSCRIPTION); expect(getStreamInNarrow(state, privateNarrow('abc@zulip.com'))).toEqual(NULL_SUBSCRIPTION); expect(getStreamInNarrow(state, topicNarrow('stream4', 'topic'))).toEqual(NULL_SUBSCRIPTION); }); From cc837fb7271a9a19510083440e3bfeaf41a5bb64 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Jul 2020 17:33:53 -0700 Subject: [PATCH 12/15] narrowsSelectors tests: Start typechecking, use example data. --- src/chat/__tests__/narrowsSelectors-test.js | 215 +++++++++----------- 1 file changed, 100 insertions(+), 115 deletions(-) diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index a1f7f68189e..6c3e0db1744 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -1,5 +1,4 @@ -import deepFreeze from 'deep-freeze'; - +/* @flow strict-local */ import { getFirstMessageId, getLastMessageId, @@ -17,16 +16,25 @@ import { groupNarrow, } from '../../utils/narrow'; import { NULL_SUBSCRIPTION } from '../../nullObjects'; +import * as eg from '../../__tests__/lib/exampleData'; describe('getMessagesForNarrow', () => { + const message = eg.streamMessage({ id: 123 }); + const messages = { + // Flow doesn't like number literals as keys...but it also wants + // them to be numbers. + [123]: message /* eslint-disable-line no-useless-computed-key */, + }; + const outboxMessage = eg.makeOutboxMessage({ + narrow: HOME_NARROW, + }); + test('if no outbox messages returns messages with no change', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { '[]': [123], }, - messages: { - 123: { id: 123 }, - }, + messages, outbox: [], }); @@ -36,22 +44,12 @@ describe('getMessagesForNarrow', () => { }); test('combine messages and outbox in same narrow', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { '[]': [123], }, - messages: { - 123: { id: 123 }, - }, - outbox: [ - { - email: 'donald@zulip.com', - narrow: HOME_NARROW, - parsedContent: '

Hello

', - sender_full_name: 'donald', - timestamp: 12, - }, - ], + messages, + outbox: [outboxMessage], caughtUp: { [HOME_NARROW_STR]: { older: false, newer: true }, }, @@ -59,35 +57,16 @@ describe('getMessagesForNarrow', () => { const result = getMessagesForNarrow(state, HOME_NARROW); - expect(result).toEqual([ - { id: 123 }, - { - email: 'donald@zulip.com', - narrow: [], - parsedContent: '

Hello

', - sender_full_name: 'donald', - timestamp: 12, - }, - ]); + expect(result).toEqual([message, outboxMessage]); }); test('do not combine messages and outbox if not caught up', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { [HOME_NARROW_STR]: [123], }, - messages: { - 123: { id: 123 }, - }, - outbox: [ - { - email: 'donald@zulip.com', - narrow: HOME_NARROW, - parsedContent: '

Hello

', - sender_full_name: 'donald', - timestamp: 12, - }, - ], + messages, + outbox: [outboxMessage], }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -96,33 +75,23 @@ describe('getMessagesForNarrow', () => { }); test('do not combine messages and outbox in different narrow', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { [JSON.stringify(privateNarrow('john@example.com'))]: [123], }, - messages: { - 123: { id: 123 }, - }, - outbox: [ - { - email: 'donald@zulip.com', - narrow: streamNarrow('denmark', 'denmark'), - parsedContent: '

Hello

', - sender_full_name: 'donald', - timestamp: 12, - }, - ], + messages, + outbox: [{ ...outboxMessage, narrow: streamNarrow('denmark') }], }); const result = getMessagesForNarrow(state, privateNarrow('john@example.com')); - expect(result).toEqual([{ id: 123 }]); + expect(result).toEqual([message]); }); }); describe('getFirstMessageId', () => { test('return undefined when there are no messages', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { '[]': [], }, @@ -135,14 +104,14 @@ describe('getFirstMessageId', () => { }); test('returns first message id', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { '[]': [1, 2, 3], }, messages: { - 1: { id: 1 }, - 2: { id: 2 }, - 3: { id: 3 }, + [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, + [2]: eg.streamMessage({ id: 2 }) /* eslint-disable-line no-useless-computed-key */, + [3]: eg.streamMessage({ id: 3 }) /* eslint-disable-line no-useless-computed-key */, }, outbox: [], }); @@ -155,7 +124,7 @@ describe('getFirstMessageId', () => { describe('getLastMessageId', () => { test('return undefined when there are no messages', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { '[]': [], }, @@ -169,14 +138,14 @@ describe('getLastMessageId', () => { }); test('returns last message id', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: { '[]': [1, 2, 3], }, messages: { - 1: { id: 1 }, - 2: { id: 2 }, - 3: { id: 3 }, + [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, + [2]: eg.streamMessage({ id: 2 }) /* eslint-disable-line no-useless-computed-key */, + [3]: eg.streamMessage({ id: 3 }) /* eslint-disable-line no-useless-computed-key */, }, outbox: [], }); @@ -188,43 +157,45 @@ describe('getLastMessageId', () => { }); describe('getStreamInNarrow', () => { - const state = deepFreeze({ - streams: [{ name: 'stream' }, { name: 'steam2' }, { name: 'stream3' }], - subscriptions: [ - { name: 'stream', in_home_view: false }, - { name: 'stream2', in_home_view: true }, - ], + const stream1 = eg.makeStream({ name: 'stream' }); + const stream2 = eg.makeStream({ name: 'stream2' }); + const stream3 = eg.makeStream({ name: 'stream3' }); + const stream4 = eg.makeStream({ name: 'stream4' }); + const sub1 = { ...eg.makeSubscription({ stream: stream1 }), in_home_view: false }; + const sub2 = { ...eg.makeSubscription({ stream: stream2 }), in_home_view: true }; + + const state = eg.reduxState({ + streams: [stream1, stream2, stream3], + subscriptions: [sub1, sub2], }); test('return subscription if stream in narrow is subscribed', () => { - const narrow = streamNarrow('stream'); + const narrow = streamNarrow(stream1.name); - expect(getStreamInNarrow(state, narrow)).toEqual({ name: 'stream', in_home_view: false }); + expect(getStreamInNarrow(state, narrow)).toEqual(sub1); }); test('return stream if stream in narrow is not subscribed', () => { - const narrow = streamNarrow('stream3'); + const narrow = streamNarrow(stream3.name); - expect(getStreamInNarrow(state, narrow)).toEqual({ name: 'stream3', in_home_view: true }); + expect(getStreamInNarrow(state, narrow)).toEqual({ ...stream3, in_home_view: true }); }); test('return NULL_SUBSCRIPTION if stream in narrow is not valid', () => { - const narrow = streamNarrow('stream4'); + const narrow = streamNarrow(stream4.name); expect(getStreamInNarrow(state, narrow)).toEqual(NULL_SUBSCRIPTION); }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { expect(getStreamInNarrow(state, privateNarrow('abc@zulip.com'))).toEqual(NULL_SUBSCRIPTION); - expect(getStreamInNarrow(state, topicNarrow('stream4', 'topic'))).toEqual(NULL_SUBSCRIPTION); + expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION); }); }); describe('isNarrowValid', () => { test('narrowing to a special narrow is always valid', () => { - const state = { - realm: {}, - }; + const state = eg.reduxState(); const narrow = STARRED_NARROW; const result = isNarrowValid(state, narrow); @@ -233,11 +204,12 @@ describe('isNarrowValid', () => { }); test('narrowing to an existing stream is valid', () => { - const state = { - realm: {}, - streams: [{ name: 'some stream' }], - }; - const narrow = streamNarrow('some stream'); + const stream = eg.makeStream({ name: 'some stream' }); + + const state = eg.reduxState({ + streams: [stream], + }); + const narrow = streamNarrow(stream.name); const result = isNarrowValid(state, narrow); @@ -245,10 +217,9 @@ describe('isNarrowValid', () => { }); test('narrowing to a non-existing stream is invalid', () => { - const state = { - realm: {}, + const state = eg.reduxState({ streams: [], - }; + }); const narrow = streamNarrow('nonexisting'); const result = isNarrowValid(state, narrow); @@ -257,11 +228,12 @@ describe('isNarrowValid', () => { }); test('narrowing to an existing stream is valid regardless of topic', () => { - const state = { - realm: {}, - streams: [{ name: 'some stream' }], - }; - const narrow = topicNarrow('some stream', 'topic does not matter'); + const stream = eg.makeStream({ name: 'some stream' }); + + const state = eg.reduxState({ + streams: [stream], + }); + const narrow = topicNarrow(stream.name, 'topic does not matter'); const result = isNarrowValid(state, narrow); @@ -269,15 +241,17 @@ describe('isNarrowValid', () => { }); test('narrowing to a PM with existing user is valid', () => { - const state = { + const user = eg.makeUser({ name: 'bob' }); + const state = eg.reduxState({ realm: { + ...eg.realmState(), crossRealmBots: [], nonActiveUsers: [], }, streams: [], - users: [{ email: 'bob@example.com' }], - }; - const narrow = privateNarrow('bob@example.com'); + users: [user], + }); + const narrow = privateNarrow(user.email); const result = isNarrowValid(state, narrow); @@ -285,15 +259,17 @@ describe('isNarrowValid', () => { }); test('narrowing to a PM with non-existing user is not valid', () => { - const state = { + const user = eg.makeUser({ name: 'bob' }); + const state = eg.reduxState({ realm: { + ...eg.realmState(), crossRealmBots: [], nonActiveUsers: [], }, streams: [], users: [], - }; - const narrow = privateNarrow('bob@example.com'); + }); + const narrow = privateNarrow(user.email); const result = isNarrowValid(state, narrow); @@ -301,15 +277,19 @@ describe('isNarrowValid', () => { }); test('narrowing to a group chat with non-existing user is not valid', () => { - const state = { + const john = eg.makeUser({ name: 'john' }); + const mark = eg.makeUser({ name: 'mark' }); + + const state = eg.reduxState({ realm: { + ...eg.realmState(), crossRealmBots: [], nonActiveUsers: [], }, streams: [], - users: [{ email: 'john@example.com' }, { email: 'mark@example.com' }], - }; - const narrow = groupNarrow(['john@example.com', 'mark@example.com']); + users: [john, mark], + }); + const narrow = groupNarrow([john.email, mark.email]); const result = isNarrowValid(state, narrow); @@ -317,14 +297,15 @@ describe('isNarrowValid', () => { }); test('narrowing to a group chat with non-existing users is also valid', () => { - const state = { + const state = eg.reduxState({ realm: { + ...eg.realmState(), crossRealmBots: [], nonActiveUsers: [], }, streams: [], users: [], - }; + }); const narrow = groupNarrow(['john@example.com', 'mark@example.com']); const result = isNarrowValid(state, narrow); @@ -333,15 +314,17 @@ describe('isNarrowValid', () => { }); test('narrowing to a PM with bots is valid', () => { - const state = { + const bot = eg.makeCrossRealmBot({ name: 'some-bot' }); + const state = eg.reduxState({ realm: { - crossRealmBots: [{ email: 'some-bot@example.com' }], + ...eg.realmState(), + crossRealmBots: [bot], nonActiveUsers: [], }, streams: [], users: [], - }; - const narrow = privateNarrow('some-bot@example.com'); + }); + const narrow = privateNarrow(bot.email); const result = isNarrowValid(state, narrow); @@ -349,15 +332,17 @@ describe('isNarrowValid', () => { }); test('narrowing to non active users is valid', () => { - const state = { + const notActiveUser = eg.makeUser({ name: 'not active' }); + const state = eg.reduxState({ realm: { + ...eg.realmState(), crossRealmBots: [], - nonActiveUsers: [{ email: 'not-active@example.com' }], + nonActiveUsers: [notActiveUser], }, streams: [], users: [], - }; - const narrow = privateNarrow('not-active@example.com'); + }); + const narrow = privateNarrow(notActiveUser.email); const result = isNarrowValid(state, narrow); From 7718c951b1ec202f36090ca648a5188a1499bc9b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Jul 2020 12:07:40 -0700 Subject: [PATCH 13/15] topicsSelectors tests: Start typechecking, use example data. --- src/topics/__tests__/topicsSelectors-test.js | 52 +++++++++++--------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 1c7b3e0ca22..ede0a76e10a 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -1,11 +1,11 @@ -import deepFreeze from 'deep-freeze'; - +/* @flow strict-local */ import { getTopicsForNarrow, getLastMessageTopic, getTopicsForStream } from '../topicSelectors'; import { HOME_NARROW, streamNarrow } from '../../utils/narrow'; +import * as eg from '../../__tests__/lib/exampleData'; describe('getTopicsForNarrow', () => { test('when no topics return an empty list', () => { - const state = deepFreeze({}); + const state = eg.reduxState(); const topics = getTopicsForNarrow(state, HOME_NARROW); @@ -13,14 +13,15 @@ describe('getTopicsForNarrow', () => { }); test('when there are topics in the active narrow, return them as string array', () => { - const state = deepFreeze({ - streams: [{ stream_id: 123, name: 'hello' }], + const stream = { ...eg.makeStream({ name: 'hello' }), stream_id: 123 }; + const state = eg.reduxState({ + streams: [stream], topics: { - 123: [{ name: 'hi' }, { name: 'wow' }], + [stream.stream_id]: [{ name: 'hi', max_id: 123 }, { name: 'wow', max_id: 234 }], }, }); - const topics = getTopicsForNarrow(state, streamNarrow('hello')); + const topics = getTopicsForNarrow(state, streamNarrow(stream.name)); expect(topics).toEqual(['hi', 'wow']); }); @@ -28,7 +29,7 @@ describe('getTopicsForNarrow', () => { describe('getLastMessageTopic', () => { test('when no messages in narrow return an empty string', () => { - const state = deepFreeze({ + const state = eg.reduxState({ narrows: {}, }); @@ -39,32 +40,32 @@ describe('getLastMessageTopic', () => { test('when one or more messages return the topic of the last one', () => { const narrow = streamNarrow('hello'); - const state = deepFreeze({ - flags: { - mentioned: {}, - }, + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2, subject: 'some topic' }); + const state = eg.reduxState({ narrows: { [JSON.stringify(narrow)]: [1, 2], }, messages: { - 1: { id: 1 }, - 2: { id: 2, subject: 'some topic' }, + [message1.id]: message1, + [message2.id]: message2, }, }); const topic = getLastMessageTopic(state, narrow); - expect(topic).toEqual('some topic'); + expect(topic).toEqual(message2.subject); }); }); describe('getTopicsForStream', () => { test('when no topics loaded for given stream return undefined', () => { - const state = deepFreeze({ + const state = eg.reduxState({ streams: [], topics: {}, mute: [], unread: { + ...eg.baseReduxState.unread, streams: [], }, }); @@ -75,27 +76,31 @@ describe('getTopicsForStream', () => { }); test('when topics loaded for given stream return them', () => { - const state = deepFreeze({ - streams: [{ stream_id: 123, name: 'stream 123' }], + const stream = { ...eg.makeStream({ name: 'stream 123' }), stream_id: 123 }; + const state = eg.reduxState({ + streams: [stream], topics: { - 123: [{ name: 'topic', max_id: 456 }], + [stream.stream_id]: [{ name: 'topic', max_id: 456 }], }, mute: [], unread: { + ...eg.baseReduxState.unread, streams: [], }, }); - const topics = getTopicsForStream(state, 123); + const topics = getTopicsForStream(state, stream.stream_id); expect(topics).toEqual([{ name: 'topic', max_id: 456, isMuted: false, unreadCount: 0 }]); }); test('Return list of topic object with isMuted, unreadCount, topic name and max id in it.', () => { - const state = deepFreeze({ - streams: [{ stream_id: 1, name: 'stream 1' }], + const stream = { ...eg.makeStream({ name: 'stream 1' }), stream_id: 1 }; + + const state = eg.reduxState({ + streams: [stream], topics: { - 1: [ + [stream.stream_id]: [ { name: 'topic 1', max_id: 5 }, { name: 'topic 2', max_id: 6 }, { name: 'topic 3', max_id: 7 }, @@ -105,6 +110,7 @@ describe('getTopicsForStream', () => { }, mute: [['stream 1', 'topic 1'], ['stream 1', 'topic 3'], ['stream 2', 'topic 2']], unread: { + ...eg.baseReduxState.unread, streams: [ { stream_id: 1, From 8aafef149932012f0f174963ee074d1dd50f7e1d Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Jul 2020 13:37:48 -0700 Subject: [PATCH 14/15] exampleData: Export displayRecipientFromUser. We'll use this in pmConversationsSelectors-test in an upcoming commit. --- src/__tests__/lib/exampleData.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 24a082bcf6f..78d626d9665 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -217,7 +217,7 @@ export const unicodeEmojiReaction: Reaction = deepFreeze({ emoji_name: 'thumbs_up', }); -const displayRecipientFromUser = (user: User): PmRecipientUser => { +export const displayRecipientFromUser = (user: User): PmRecipientUser => { const { email, full_name, user_id: id } = user; return deepFreeze({ email, From afd013b15a97cc51b961f4bccdfbc93e4ab0a04a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 10 Jul 2020 13:38:49 -0700 Subject: [PATCH 15/15] pmConversationsSelectors tests: Start typechecking, use example data. --- .../pmConversationsSelectors-test.js | 323 ++++++++++-------- 1 file changed, 180 insertions(+), 143 deletions(-) diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index f665209a1ac..a507aa02b3d 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -1,17 +1,21 @@ -import deepFreeze from 'deep-freeze'; - +/* @flow strict-local */ import { getRecentConversations } from '../pmConversationsSelectors'; import { ALL_PRIVATE_NARROW_STR } from '../../utils/narrow'; +import * as eg from '../../__tests__/lib/exampleData'; describe('getRecentConversations', () => { + const userJohn = { ...eg.makeUser({ name: 'John' }), user_id: 1 }; + const userMark = { ...eg.makeUser({ name: 'Mark' }), user_id: 2 }; + test('when no messages, return no conversations', () => { - const state = deepFreeze({ - realm: { email: 'me@example.com' }, - users: [{ user_id: 0, email: 'me@example.com' }], + const state = eg.reduxState({ + realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], narrows: { [ALL_PRIVATE_NARROW_STR]: [], }, unread: { + ...eg.baseReduxState.unread, pms: [], huddles: [], }, @@ -23,75 +27,86 @@ describe('getRecentConversations', () => { }); test('returns unique list of recipients, includes conversations with self', () => { - const state = deepFreeze({ - realm: { email: 'me@example.com' }, - users: [ - { user_id: 0, email: 'me@example.com' }, - { user_id: 1, email: 'john@example.com' }, - { user_id: 2, email: 'mark@example.com' }, + const meAndJohnPm1 = eg.pmMessage({ + id: 1, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userJohn), + ], + }); + + const meAndMarkPm = eg.pmMessage({ + id: 2, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userMark), ], + }); + + const meAndJohnPm2 = eg.pmMessage({ + id: 3, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userJohn), + ], + }); + + const meOnlyPm = eg.pmMessage({ + id: 4, + display_recipient: [eg.displayRecipientFromUser(eg.selfUser)], + }); + + const meJohnAndMarkPm = eg.pmMessage({ + id: 0, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userMark), + eg.displayRecipientFromUser(userJohn), + ], + }); + + const state = eg.reduxState({ + realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser, userJohn, userMark], narrows: { - [ALL_PRIVATE_NARROW_STR]: [0, 1, 2, 3, 4], + [ALL_PRIVATE_NARROW_STR]: [ + meJohnAndMarkPm.id, + meAndJohnPm1.id, + meAndMarkPm.id, + meAndJohnPm2.id, + meOnlyPm.id, + ], }, messages: { - 1: { - id: 1, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 2: { - id: 2, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 3: { - id: 3, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 4: { - id: 4, - type: 'private', - display_recipient: [{ id: 0, email: 'me@example.com' }], - }, - 0: { - id: 0, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, + [meAndJohnPm1.id]: meAndJohnPm1, + [meAndMarkPm.id]: meAndMarkPm, + [meAndJohnPm2.id]: meAndJohnPm2, + [meOnlyPm.id]: meOnlyPm, + [meJohnAndMarkPm.id]: meJohnAndMarkPm, }, unread: { + ...eg.baseReduxState.unread, pms: [ { - sender_id: 0, - unread_message_ids: [4], + sender_id: eg.selfUser.user_id, + unread_message_ids: [meOnlyPm.id], }, { - sender_id: 1, - unread_message_ids: [1, 3], + sender_id: userJohn.user_id, + unread_message_ids: [meAndJohnPm1.id, meAndJohnPm2.id], }, { - sender_id: 2, - unread_message_ids: [2], + sender_id: userMark.user_id, + unread_message_ids: [meAndMarkPm.id], }, ], huddles: [ { - user_ids_string: '0,1,2', - unread_message_ids: [5], + user_ids_string: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] + .sort() + .map(String) + .join(','), + unread_message_ids: [5], // TODO: where does this come from??? }, ], }, @@ -99,27 +114,30 @@ describe('getRecentConversations', () => { const expectedResult = [ { - ids: '0', - recipients: 'me@example.com', - msgId: 4, + ids: eg.selfUser.user_id.toString(), + recipients: eg.selfUser.email, + msgId: meOnlyPm.id, unread: 1, }, { - ids: '1', - recipients: 'john@example.com', - msgId: 3, + ids: userJohn.user_id.toString(), + recipients: userJohn.email, + msgId: meAndJohnPm2.id, unread: 2, }, { - ids: '2', - recipients: 'mark@example.com', - msgId: 2, + ids: userMark.user_id.toString(), + recipients: userMark.email, + msgId: meAndMarkPm.id, unread: 1, }, { - ids: '0,1,2', - recipients: 'john@example.com,mark@example.com', - msgId: 0, + ids: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] + .sort() + .map(String) + .join(','), + recipients: [userJohn.email, userMark.email].sort().join(','), + msgId: meJohnAndMarkPm.id, unread: 1, }, ]; @@ -130,83 +148,99 @@ describe('getRecentConversations', () => { }); test('returns recipients sorted by last activity', () => { - const state = deepFreeze({ - realm: { email: 'me@example.com' }, - users: [ - { user_id: 0, email: 'me@example.com' }, - { user_id: 1, email: 'john@example.com' }, - { user_id: 2, email: 'mark@example.com' }, + // Maybe we can share these definitions with the above test; + // first, we have to sort out why the IDs are different. + + const meAndMarkPm1 = eg.pmMessage({ + id: 1, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userMark), ], + }); + + const meAndJohnPm1 = eg.pmMessage({ + id: 2, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userJohn), + ], + }); + + const meAndMarkPm2 = eg.pmMessage({ + id: 3, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userMark), + ], + }); + + const meAndJohnPm2 = eg.pmMessage({ + id: 4, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userJohn), + ], + }); + + const meJohnAndMarkPm = eg.pmMessage({ + id: 5, + display_recipient: [ + eg.displayRecipientFromUser(eg.selfUser), + eg.displayRecipientFromUser(userJohn), + eg.displayRecipientFromUser(userMark), + ], + }); + + const meOnlyPm = eg.pmMessage({ + id: 6, + display_recipient: [eg.displayRecipientFromUser(eg.selfUser)], + }); + + const state = eg.reduxState({ + realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser, userJohn, userMark], narrows: { - [ALL_PRIVATE_NARROW_STR]: [1, 2, 3, 4, 5, 6], + [ALL_PRIVATE_NARROW_STR]: [ + meAndMarkPm1.id, + meAndJohnPm1.id, + meAndMarkPm2.id, + meAndJohnPm2.id, + meJohnAndMarkPm.id, + meOnlyPm.id, + ], }, messages: { - 2: { - id: 2, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 1: { - id: 1, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 4: { - id: 4, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 3: { - id: 3, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 5: { - id: 5, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 6: { - id: 6, - type: 'private', - display_recipient: [{ id: 0, email: 'me@example.com' }], - }, + [meAndJohnPm1.id]: meAndJohnPm1, + [meAndMarkPm1.id]: meAndMarkPm1, + [meAndJohnPm2.id]: meAndJohnPm2, + [meAndMarkPm2.id]: meAndMarkPm2, + [meJohnAndMarkPm.id]: meJohnAndMarkPm, + [meOnlyPm.id]: meOnlyPm, }, unread: { + ...eg.baseReduxState.unread, pms: [ { - sender_id: 0, - unread_message_ids: [4], + sender_id: eg.selfUser.user_id, + unread_message_ids: [meAndJohnPm2.id], }, { - sender_id: 1, - unread_message_ids: [1, 3], + sender_id: userJohn.user_id, + unread_message_ids: [meAndMarkPm1.id, meAndMarkPm2.id], }, { - sender_id: 2, - unread_message_ids: [2], + sender_id: userMark.user_id, + unread_message_ids: [meAndJohnPm1.id], }, ], huddles: [ { - user_ids_string: '0,1,2', - unread_message_ids: [5], + user_ids_string: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] + .sort() + .map(String) + .join(','), + unread_message_ids: [meJohnAndMarkPm.id], }, ], }, @@ -214,27 +248,30 @@ describe('getRecentConversations', () => { const expectedResult = [ { - ids: '0', - recipients: 'me@example.com', - msgId: 6, + ids: eg.selfUser.user_id.toString(), + recipients: eg.selfUser.email, + msgId: meOnlyPm.id, unread: 1, }, { - ids: '0,1,2', - recipients: 'john@example.com,mark@example.com', - msgId: 5, + ids: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] + .sort() + .map(String) + .join(','), + recipients: [userJohn.email, userMark.email].sort().join(','), + msgId: meJohnAndMarkPm.id, unread: 1, }, { - ids: '1', - recipients: 'john@example.com', - msgId: 4, + ids: userJohn.user_id.toString(), + recipients: userJohn.email, + msgId: meAndJohnPm2.id, unread: 2, }, { - ids: '2', - recipients: 'mark@example.com', - msgId: 3, + ids: userMark.user_id.toString(), + recipients: userMark.email, + msgId: meAndMarkPm2.id, unread: 1, }, ];