-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add Jest unit tests and Detox end to end tests #415
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
Conversation
| @@ -0,0 +1,10 @@ | |||
| export default { | |||
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.
Rather than using a mock file for this, could a .env file be used instead like .env.travis?
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 is how the library recommended to do it: https://github.com/luggit/react-native-config#jest
We might be able to create a env.travis file, then import it in this specific file (I think the file name is the most important), then use the variables?
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.
OK, thanks for that info! What we could do is use dotenv (lib which we already use) to read the .env.travis file and export the config values in the same format being done here. Like this:
const travisConfig = dotenv('.env.travis');
export default {
REPORT_IDS: travisConfig.REPORT_IDS,
};
or maybe even just:
const travisConfig = dotenv('.env.travis');
export default travisConfig;
|
Ok so now the default Jest tests pass that were originally included with the project, but now I need to dig into https://reactnative.dev/docs/testing-overview to really understand what type of tests we'd like to add. Any ideas @tgolen? |
|
Ok so after reading and discussing with @marcaaron, in addition to the code added here, I think we should explore a simple E2E style test on all platforms to verify that the app at minimum boots as we expect. I believe this is the highest value test we could add to verify we aren't breaking production with our continuous deploy changes. The two instances that we should be testing for are: 1. Bad style props that cause native clients not to boot 2. bad import statements that cause the build to break It looks like |
|
Lots of good stuff in that article! The few things standing out to me are:
1. We probably don't need end-to-end tests, that's what Applause is for
1. Historically, the only valuable unit tests we have in JS have been
around code that is ripe for regressions (like working with multi-tag
levels). I think we should use some common sense to not "test everything"
but to test the stuff that is valuable
1. That article says that it won't be able to test any platform-specific
code, so we need to rely on Applause uncovering platform bugs
1. We could approach a bug mentality for testing, where a bug has to be
reproduced in a test first before it can be fixed. While this is an
interesting idea, I don't think it's very practical and I don't know that
it would be too valuable except for catching regressions.
1. Bad style props that cause native clients not to boot 2. bad import
statements that cause the build to break
I agree with having a test that verifies both of those! +1
…On Tue, Sep 8, 2020 at 3:09 PM Andrew Gable ***@***.***> wrote:
Ok so now the default Jest tests pass that were originally included with
the project, but now I need to dig into
https://reactnative.dev/docs/testing-overview to really understand what
type of tests we'd like to add. Any ideas @tgolen
<https://github.com/tgolen>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#415 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMABYJZTXJ6C4R7OHYYXLSE2MPNANCNFSM4QZVWF6Q>
.
|
|
So from reading the docs, I couldn't figure out if we can test for those two cases using just Unit tests, so I assumed that we needed a basic E2E test that in the end is just making sure the specific platform layer booted successfully. I agree that we probably shouldn't write super extensive automated tests that run on the device, but what does everyone think about having a boot test (that is an E2E test using detox)? |
|
Using detox seems overkill for just doing a boot test. I agree that the boot test is valuable, but is there another way to accomplish that without the overhead of detox? (also, using something other than detox would help to avoid people wanting to add a bunch of E2E tests just for the sake of having them, which puts us down the road of maintaining them, etc. etc. flash back to selenium...). |
|
OK, I guess it's fine to see where this takes us.
…On Tue, Sep 8, 2020 at 5:46 PM Andrew Gable ***@***.***> wrote:
I think we could verify we built and booted a simulator, but I don't think
we could confirm that we are on the sign in page after we booted without
detox, which is when we've had these error pop up. They appear to be run
time errors seen when launching the app, for example:
[image: redbox]
<https://user-images.githubusercontent.com/2838819/92538134-c3f1ba00-f1f2-11ea-826a-f62b165b9a32.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#415 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMABZOJA6XCZNAJLWUIFLSE2645ANCNFSM4QZVWF6Q>
.
|
|
Detox tests are passing locally: But are failing to start on GitHub actions: |
|
Any ideas on how we can get this working? |
|
Yeah good question, I need to put some more time into figuring out why the simulator is failing to launch on GitHub Actions. |
|
Ok circling back to this, it looks like detox has a guide on this: https://github.com/wix/Detox/blob/master/docs/Guide.RunningOnCI.md I'll check it out and try to get things running |
|
Updated with recommended solution and locally they pass, on GitHub they fail still 😞 Local: GitHub: |
|
Tests are passing, I'd love a review, thanks! |
marcaaron
left a comment
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.
Looks awesome!!
.github/workflows/lint.yml
Outdated
| @@ -1,4 +1,4 @@ | |||
| name: eslint | |||
| name: ESLint JavaScript Lint | |||
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.
NAB, not sure why this format is changing but the extra Lint seems extra maybe just Lint JavaScript
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.
Just a name change, I thought that being a bit more descriptive was better, but I like the suggestion 👍
.github/workflows/test.yml
Outdated
| - uses: actions/checkout@v2 | ||
| - name: Use Node.js | ||
| uses: actions/setup-node@v1 | ||
| - uses: webfactory/ssh-agent@v0.4.0 |
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.
Do we need to update this guy re: that other PR?
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.
Yes, good catch, I will update.
babel.config.js
Outdated
| plugins: [], | ||
| }; | ||
|
|
||
|
|
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.
NAB extra new line
tests/e2e/environment.js
Outdated
| super(config); | ||
|
|
||
| // Can be safely removed, if you are content with the default value (=300000ms) | ||
| this.initTimeout = 300000; |
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.
Are we content?
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.
Yes, we are, I shall remove this value 👍
sketchydroide
left a comment
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.
LGTM
In all honesty this is hard to understand, but from I could understand it looks good.
618b789
|
Got a conflict but ready for merging :) |
marcaaron
left a comment
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.
Looks great!
@tgolen everything looks good to me but feel free to take a look when you get back from OOO
|
Since this PR has been merged Any ideas what I'm missing, or does anyone remember successfully testing this PR on mobile? (to rule out an issue on this PR) I've tried clearing NPM cache, npm install, ect. Additional info on this Slack thread |
|
Well, please ignore the above. Restarting my machine fixed this issue... No idea what the actual issue is, and why restarting works but I'll add it to my RN troubleshooting steps 😞 |
| jest.setTimeout(120000 * 10); | ||
|
|
||
| describe('Test login page', () => { | ||
| beforeEach(async () => { |
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.
From looking at the Jest documentation, using promises should be pretty straight forward. Can you please switch these to be promises?
| @@ -0,0 +1,10 @@ | |||
| export default { | |||
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.
Can this file just import and parse the .env file so that this stuff doesn't need to be defined in more than one place?
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.
If .env was a JS file, maybe. But this is how they recommend setting it up: https://github.com/luggit/react-native-config#testing
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.
Even without the .env file being JS, it can still be easily parsed using the dotenv package that we already have installed and are using elsewhere. That will DRY this config up and make sure that all of our configs are only defined in a single place.
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.
Ah I see, let me see if I can get it to work in the tests 👍
|
|
||
| ## Running the tests 🎰 | ||
| * To run the **Jest Unit Tests**: `npm run test` | ||
| ### Unit tests |
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.
Could you please add more info in this Readme about the difference between unit tests and e2e tests and when you would use one instead of the other and the benefits of either of them?
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.
Yes!
sketchydroide
left a comment
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.
LGTM

@tgolen @marcaaron - Will you please review?
Adds test two frameworks to RNC repo,
jestunit tests anddetoxe2e tests. While the tests are sparse at the moment, I hope they enable the ability to add future tests that will grow into a valuable addition.