-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA] Add Pusher Automated Tests #1153
Changes from 25 commits
f0a9e60
1c37dd5
d4e9ef1
29d02a1
5970909
c409ac3
ccc3778
68f1d37
478f094
918fa0a
781f82b
da7746e
f65b202
fa53652
6d59757
e8e0b3c
2f8dae6
39684fe
9704aa9
c6a235a
8e55d77
7298525
cd18cdb
2ae40c9
fb69894
84bbfb6
422ae85
739bbb7
fefa0ef
b895dce
acb4b80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export default { | ||
addEventListener: jest.fn(), | ||
requestPermissions: jest.fn(() => Promise.resolve()), | ||
getInitialNotification: jest.fn(() => Promise.resolve()), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import {PusherMock} from 'pusher-js-mock'; | ||
|
||
export default PusherMock; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,12 @@ | ||
export default { | ||
setUserNotificationsEnabled: jest.fn(), | ||
}; | ||
|
||
const EventType = { | ||
NotificationResponse: 'notificationResponse', | ||
PushReceived: 'pushReceived', | ||
}; | ||
|
||
export { | ||
EventType, | ||
}; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,102 @@ | ||||||
import moment from 'moment'; | ||||||
import Onyx from 'react-native-onyx'; | ||||||
import ONYXKEYS from '../../src/ONYXKEYS'; | ||||||
import * as Pusher from '../../src/libs/Pusher/pusher'; | ||||||
import PusherConnectionManager from '../../src/libs/PusherConnectionManager'; | ||||||
import CONFIG from '../../src/CONFIG'; | ||||||
import {addAction, subscribeToReportCommentEvents} from '../../src/libs/actions/Report'; | ||||||
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; | ||||||
import PushNotification from '../../src/libs/Notification/PushNotification'; | ||||||
import {signInWithTestUser, fetchPersonalDetailsForTestUser} from '../utils/TestHelper'; | ||||||
|
||||||
PushNotification.register = () => {}; | ||||||
PushNotification.deregister = () => {}; | ||||||
|
||||||
describe('actions/Report', () => { | ||||||
it('should store a new report action in Onyx when one is handled via Pusher', () => { | ||||||
const TEST_USER_ACCOUNT_ID = 1; | ||||||
const TEST_USER_LOGIN = 'test@test.com'; | ||||||
const REPORT_ID = 1; | ||||||
const ACTION_ID = 1; | ||||||
const REPORT_ACTION = { | ||||||
actionName: 'ADDCOMMENT', | ||||||
actorAccountID: TEST_USER_ACCOUNT_ID, | ||||||
actorEmail: TEST_USER_LOGIN, | ||||||
automatic: false, | ||||||
avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_3.png', | ||||||
isAttachment: false, | ||||||
isFirstItem: false, | ||||||
loading: true, | ||||||
message: [{type: 'COMMENT', html: 'Testing a comment', text: 'Testing a comment'}], | ||||||
person: [{type: 'TEXT', style: 'strong', text: 'Test User'}], | ||||||
sequenceNumber: ACTION_ID, | ||||||
shouldShow: true, | ||||||
timestamp: moment().unix(), | ||||||
}; | ||||||
|
||||||
// When using the Pusher mock the act of calling Pusher.isSubscribed will create a | ||||||
// channel already in a subscribed state. These methods are normally used to prevent | ||||||
// duplicated subscriptions, but we don't need them for this test. | ||||||
Pusher.isSubscribed = jest.fn().mockReturnValue(false); | ||||||
Pusher.isAlreadySubscribing = jest.fn().mockReturnValue(false); | ||||||
|
||||||
// Connect to Pusher | ||||||
PusherConnectionManager.init(); | ||||||
Pusher.init({ | ||||||
appKey: CONFIG.PUSHER.APP_KEY, | ||||||
cluster: CONFIG.PUSHER.CLUSTER, | ||||||
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=Push_Authenticate`, | ||||||
}); | ||||||
|
||||||
let reportActions; | ||||||
Onyx.connect({ | ||||||
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, | ||||||
callback: val => reportActions = val, | ||||||
}); | ||||||
|
||||||
// Set up Onyx with some test user data | ||||||
return signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN) | ||||||
.then(() => { | ||||||
subscribeToReportCommentEvents(); | ||||||
return waitForPromisesToResolve(); | ||||||
}) | ||||||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
.then(() => fetchPersonalDetailsForTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN, { | ||||||
[TEST_USER_LOGIN]: { | ||||||
accountID: TEST_USER_ACCOUNT_ID, | ||||||
email: TEST_USER_LOGIN, | ||||||
firstName: 'Test', | ||||||
lastName: 'User', | ||||||
}, | ||||||
})) | ||||||
.then(() => { | ||||||
// This is a fire and forget response, but one it completes we should be able to verify that we | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// have an "optimistic" report action in Onyx. | ||||||
addAction(REPORT_ID, 'Testing a comment'); | ||||||
return waitForPromisesToResolve(); | ||||||
}) | ||||||
.then(() => { | ||||||
const resultAction = reportActions[ACTION_ID]; | ||||||
expect(resultAction).toEqual(REPORT_ACTION); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding this assertion will help identify that yes, this is definitely an optimistic comment, and not the final result.
This, of course, is already implied in the current assertion, but it's an extra mental jump that has to be made which isn't totally obvious. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that makes sense to me. I think it'll be a more interesting test when the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this will look cool once the clientID stuff happens, and it will be a perfect use for these tests! |
||||||
}) | ||||||
.then(() => { | ||||||
// Now that we are subscribed we need to simulate a reportComment action Pusher event. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Subscribed to what? Is this referring to the original Pusher subscription or to Onyx? This reads a little weird because I would expect the thing before this to be code that does subscribing, but the thing before this is adding the report action. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can make this more explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It probably made more sense before when the thing immediately before this was |
||||||
// Then verify that action was handled correctly and merged into the reportActions. | ||||||
const channel = Pusher.getChannel('private-user-accountID-1'); | ||||||
channel.emit('reportComment', { | ||||||
reportID: REPORT_ID, | ||||||
reportAction: REPORT_ACTION, | ||||||
}); | ||||||
|
||||||
// Once this happens we should see the comment get processed by the callback and added to the | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
"this" could be referring to a number of things, so be more explicit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is also ambiguous. Is it the pusher callback, the onyx callback, or a different callback entirely? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can make this more explicit. |
||||||
// storage so we must wait for promises to resolve again and then verify the data is in Onyx. | ||||||
return waitForPromisesToResolve(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see how maybe this is necessary to prevent any race conditions with the local data stored in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right. The callback should happen in realtime and then we set the data async via |
||||||
}) | ||||||
.then(() => { | ||||||
const resultAction = reportActions[ACTION_ID]; | ||||||
|
||||||
// Verify that our action is no longer in the loading state | ||||||
REPORT_ACTION.loading = false; | ||||||
expect(resultAction).toEqual(REPORT_ACTION); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a different assertion might make it a little more clear what's being tested and prevents the need to modify the contents of Suggestion:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this is much better |
||||||
}); | ||||||
}); | ||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import {signIn, fetchAccountDetails} from '../../src/libs/actions/Session'; | ||
import {fetch as fetchPersonalDetails} from '../../src/libs/actions/PersonalDetails'; | ||
import HttpUtils from '../../src/libs/HttpUtils'; | ||
import waitForPromisesToResolve from './waitForPromisesToResolve'; | ||
|
||
/** | ||
* Simulate signing in and make sure all API calls in this flow succeed. Every time we add | ||
* a mockImplementationOnce() we are altering what Network.post() will return. | ||
* | ||
* @param {Number} accountID | ||
* @param {String} login | ||
* @param {String} password | ||
* @param {String} authToken | ||
* @return {Promise} | ||
*/ | ||
function signInWithTestUser(accountID, login, password = 'Password1', authToken = 'asdfqwerty') { | ||
HttpUtils.xhr = jest.fn(); | ||
HttpUtils.xhr.mockImplementation(() => Promise.resolve({ | ||
jsonCode: 200, | ||
accountExists: true, | ||
canAccessExpensifyCash: true, | ||
requiresTwoFactorAuth: false, | ||
})); | ||
|
||
// Simulate user entering their login and populating the credentials.login | ||
fetchAccountDetails(login); | ||
return waitForPromisesToResolve() | ||
.then(() => { | ||
// First call to Authenticate | ||
HttpUtils.xhr | ||
.mockImplementationOnce(() => Promise.resolve({ | ||
jsonCode: 200, | ||
accountID, | ||
authToken, | ||
email: login, | ||
})) | ||
|
||
// Next call to CreateLogin | ||
.mockImplementationOnce(() => Promise.resolve({ | ||
jsonCode: 200, | ||
accountID, | ||
authToken, | ||
email: login, | ||
})); | ||
signIn(password); | ||
return waitForPromisesToResolve(); | ||
}); | ||
} | ||
|
||
/** | ||
* Fetch and set personal details with provided personalDetailsList | ||
* | ||
* @param {Number} accountID | ||
* @param {String} email | ||
* @param {Object} personalDetailsList | ||
* @returns {Promise} | ||
*/ | ||
function fetchPersonalDetailsForTestUser(accountID, email, personalDetailsList) { | ||
// Mock xhr() | ||
HttpUtils.xhr = jest.fn(); | ||
|
||
// Get the personalDetails | ||
HttpUtils.xhr | ||
|
||
// fetchPersonalDetails | ||
.mockImplementationOnce(() => Promise.resolve({ | ||
accountID, | ||
email, | ||
personalDetailsList, | ||
})) | ||
|
||
// fetchTimezone | ||
.mockImplementationOnce(() => Promise.resolve({})); | ||
|
||
fetchPersonalDetails(); | ||
return waitForPromisesToResolve(); | ||
} | ||
|
||
export { | ||
signInWithTestUser, | ||
fetchPersonalDetailsForTestUser, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a little more clarity.