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.

Sign up for GitHub

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

Jump to bottom

[HOLD] Add Automated Tests for Onyx / withOnyx #768

Closed
wants to merge 18 commits into from
Closed

[HOLD] Add Automated Tests for Onyx / withOnyx #768

wants to merge 18 commits into from

Conversation

marcaaron
Copy link
Contributor

cc @AndrewGable

Fixed Issues

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

Tests

  1. Adds automated tests for Ion to make sure the most basic features are working as intended.

Screenshots

@marcaaron marcaaron self-assigned this Nov 6, 2020
@marcaaron marcaaron changed the title Add Automated Tests for Ion Add Automated Tests for Ion / withIon Nov 10, 2020
marcaaron
marcaaron commented Nov 10, 2020
View reviewed changes
src/libs/Log.js
@@ -46,5 +46,5 @@ export default new Logger({
clientLoggingCallback: (message) => {
console.debug(message);
},
isDebug: !CONFIG.IS_IN_PRODUCTION,
isDebug: !CONFIG.IS_JEST_RUNNING && !CONFIG.IS_IN_PRODUCTION,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the issue where we were console logging after the test has finished and Jest complains locally.

@marcaaron marcaaron marked this pull request as ready for review November 10, 2020 17:49
@marcaaron marcaaron requested a review from a team as a code owner November 10, 2020 17:49
@botify botify requested review from bondydaa and removed request for a team November 10, 2020 17:50
bondydaa
bondydaa previously approved these changes Nov 10, 2020
View reviewed changes
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.

similar to #771 you might want to get someone else who's more familiar with this repo to also review b/c this is the first 2 times i've looked at any of this code.

src/CONFIG.js
@@ -11,6 +11,7 @@ export default {
},
// eslint-disable-next-line no-undef
IS_IN_PRODUCTION: Platform.OS === 'web' ? process.env.NODE_ENV === 'production' : !__DEV__,
IS_JEST_RUNNING: typeof jest !== 'undefined',
Copy link
Contributor

Choose a reason for hiding this comment

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

depending on which is merged first you might get a conflict with this https://github.com/Expensify/ReactNativeChat/pull/771/files

@bondydaa
Copy link
Contributor

or feel free to self-merge if you don't think you need another reviewer

@marcaaron
Copy link
Contributor Author

Ok worries thanks for the review! I will wait for @AndrewGable to give this a once over 🙇‍♂️

@marcaaron
Copy link
Contributor Author

This is gonna need to be fixed up now that Ion is Onyx. Gonna keep these tests here for now.

@marcaaron marcaaron marked this pull request as draft November 12, 2020 02:51
@marcaaron marcaaron marked this pull request as ready for review November 12, 2020 02:56
@marcaaron marcaaron changed the title Add Automated Tests for Ion / withIon Add Automated Tests for Onyx / withOnyx Nov 12, 2020
@marcaaron marcaaron marked this pull request as draft November 13, 2020 19:22
@marcaaron marcaaron changed the title Add Automated Tests for Onyx / withOnyx [HOLD] Add Automated Tests for Onyx / withOnyx Nov 13, 2020
@marcaaron
Copy link
Contributor Author

Just a heads up I'm going to close this in favor of Expensify/react-native-onyx#15

I think these tests should be in the react-native-onyx repo so we can catch any issues before updating a version in ReactNativeChat.

@marcaaron marcaaron closed this Dec 2, 2020
@marcaaron marcaaron deleted the marcaaron-ionTest branch December 2, 2020 20:11
@AndrewGable
Copy link
Contributor

Makes sense!

@Taimoor0217 Taimoor0217 mentioned this pull request Nov 19, 2023
Open
@atef1995 atef1995 mentioned this pull request Nov 19, 2023
Open
@osama256 osama256 mentioned this pull request Nov 20, 2023
Open
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a16df0d
Status: ✅  Deploy successful!
Preview URL: https://f6c9cea8.helpdot.pages.dev

View logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bondydaa bondydaa bondydaa left review comments

@AndrewGable AndrewGable Awaiting requested review from AndrewGable

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcaaron @bondydaa @AndrewGable