Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD][No QA] Add automated tests for Report actions/ Pusher #771

Closed
wants to merge 9 commits into from

Conversation

marcaaron
Copy link
Contributor

cc @AndrewGable

This adds a mocked test for one of our most critical flows

Optimistic comment > Create comment in server (this is mocked) > Pusher handles comment > Expected response verified in Ion

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/145497

Tests

Automated tests were added.

Screenshots

@marcaaron marcaaron self-assigned this Nov 6, 2020
@marcaaron marcaaron marked this pull request as ready for review November 10, 2020 17:48
@marcaaron marcaaron requested a review from a team as a code owner November 10, 2020 17:48
@botify botify requested review from bondydaa and removed request for a team November 10, 2020 17:48
bondydaa
bondydaa previously approved these changes Nov 10, 2020
Copy link
Contributor

@bondydaa bondydaa left a comment

Choose a reason for hiding this comment

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

you may want someone who's actually worked with Ion and Pusher to review this as well b/c all of it is a little over my head but this seems to make sense. Is there anything we can do to test a failure case and make sure it's handled properly?

@marcaaron
Copy link
Contributor Author

Is there anything we can do to test a failure case and make sure it's handled properly?

To clarify, do mean something like "change how Ion works to see if the tests are still passing" or "write test cases that should fail and verifying they fail"? I guess I'm not sure what you want me to do exactly. But I'm up for it if you think it's necessary.

@marcaaron
Copy link
Contributor Author

Adding @AndrewGable for another set of eyes since he has the most experience with tests.

@marcaaron marcaaron changed the title [No QA] Add automated tests for Report actions/ Pusher [HOLD][No QA] Add automated tests for Report actions/ Pusher Nov 13, 2020
@marcaaron marcaaron marked this pull request as draft November 13, 2020 19:22
@@ -18,6 +18,7 @@
"android-build": "fastlane android build",
"test": "jest",
"lint": "eslint .",
"lint-tests": "eslint tests/**",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we separating out linting tests and the source? Ideally they are the same, right?

src/libs/API.js Outdated
@@ -70,6 +70,11 @@ function isAuthTokenRequired(command) {
* @returns {Promise}
*/
function queueRequest(command, data) {
// Mock all requests for now
if (CONFIG.IS_JEST_RUNNING) {
Copy link
Contributor

Choose a reason for hiding this comment

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

While this works, I thought the preferred method was to mock API.js and make queueRequest just return an empty function (or something similar). Is there a reason we can't go that route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I'll look into this.

require('pusher-js-mock').PusherMock
));

jest.mock('../../node_modules/@react-native-community/netinfo', () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if we'd prefer to put all these mocks in a different file and then import them? It might DRY up the tests a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to get this working.

@marcaaron
Copy link
Contributor Author

Gonna close this until I have time to investigate how to fix this. Also I think we should have a broader conversation about testing and test conventions. Specifically, agreeing to the use of async/await to make linear testing of asynchronous JS easier would be nice. That was my main blocker when attempting to write these tests.

@marcaaron marcaaron closed this Dec 24, 2020
@marcaaron marcaaron deleted the marcaaron-pusher-tests branch December 24, 2020 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants