RNTester: Setup E2E tests for iOS and Android with Appium and Jest #35016
Conversation
Base commit: 270584a |
Base commit: efd39ee |
react-native-bot
added
the
No CLA
|
Hi @mateuszm22! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
react-native-bot
added
the
Type: Enhancement
|
PR build artifact for 0a186ee is ready. |
|
PR build artifact for 0a186ee is ready. |
| @@ -0,0 +1,41 @@ | |||
| import { defineFeature, loadFeature } from 'jest-cucumber'; | |||
| import { givenUserOnMainPage } from '../common_steps/commonSteps.steps'; | |||
| import { thenVerifyAlertBoxHasText, thenVerifyThatTheButtonComponentIsDisplayed, thenVerifyThatTheButtonHeaderIsDisplayed, whenUserClicksOnTheButtonComponent, whenUserClicksOnTheCancelApplicationButton, whenUserClicksOnTheOKButton, whenUserClicksOnTheSubmitApplicationButton } from '../steps/buttonComponentScreen.steps'; | |||
There was a problem hiding this comment.
This seems like unnecessary decoupling of code that's tightly coupled with this file. I believe we could move all of those helpers into this file and achieve similar readability and higher code colocation
There was a problem hiding this comment.
It has been resolved by importing all functions (*). Then one line to be able to use the functions without an alias. Please, check if the current implementation is suitable
| }; | ||
|
|
||
| export const whenUserClicksOnTheSubmitApplicationButton = (when) => { | ||
| when(/^User clicks on the Submit Application button$/, async () => { |
There was a problem hiding this comment.
this could have been a regular string instead of a regex, right?
There was a problem hiding this comment.
I believe we should keep the consistency of the steps format
There was a problem hiding this comment.
I see a regex rather an exception from the rule of using strings. There's nothing wrong with having a little bit of inconsistency here. Personally it would be just harder for me to write a working regex than a plain string. Using a regex also triggers a different mental path for me (my mind goes "whats weird about this code that it uses regex instead of a string?" – if it finds nothing weird, then it's the time that I could save by using a faster string lookup that I'd normally expect). Again, I'm speaking for my own. If you find this is saving you time and no one else complains, please go with it
There was a problem hiding this comment.
I have checked couple of repos to verify how it is resolved there. Most of them use consistent regex format for every step. But we are fine with both approaches. I understand your point of view. I have changed steps to the expected format. Thanks! c:
| if (process.env.E2E_DEVICE.includes('ios')) { | ||
| deviceLocator = this.buttonComponentIOS; | ||
| } | ||
| if (process.env.E2E_DEVICE .includes('android')) { | ||
| deviceLocator = this.buttonComponent; | ||
| } |
There was a problem hiding this comment.
if more of similar logic is expected, I'd suggest creating a helper like RN's Platform.select, which could look like this:
const deviceLocator = platformSelect({
ios: this.buttonComponentIOS,
android: this.buttonComponent
});Since you're using a class, it could be done once in property initializer or constructor:
class ComponentsScreen {
element = platformSelect({
ios: '[label="Button Simple React Native button component."]',
android: '~Button Simple React Native button component.'
})
async checkButtonComponentIsDisplayed() {
return await Utils.checkElementExistence(element);
}
}
| @@ -0,0 +1,31 @@ | |||
| const Utils = require('../helpers/utils'); | |||
There was a problem hiding this comment.
can we use import instead of require?
| } | ||
| if (process.env.E2E_DEVICE .includes('android')) { | ||
| deviceLocator = this.btnOK; | ||
| } |
There was a problem hiding this comment.
yeah, let's create the platformSelect helper mentioned here: https://github.com/facebook/react-native/pull/35016/files#r1024331245 and refactor
| Then Verify that the "Button" header is displayed | ||
| When User clicks on the Cancel Application button | ||
| Then Verify that the alert box has text: "Your application has been cancelled!" | ||
| When User clicks on the OK button |
There was a problem hiding this comment.
I would add here one more Then step e.g.
Then User is on the main screen
The same in second scenario.
There was a problem hiding this comment.
fixed
Then Verify that the "Button" header is displayed has been added
packages/rn-tester/e2e/.eslintrc.js
Outdated
| @@ -0,0 +1,4 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
this repo has a root eslintrc so we can remove this one :)
|
|
||
| export const thenVerifyAlertBoxHasText = (then) => { | ||
| then(/^Verify that the cancel|submit alert box has text: "(.*)"$/, async (alertBoxType, alertBoxText) => { | ||
| switch(alertBoxType) { |
|
PR build artifact for faff18a is ready. |
|
PR build artifact for faff18a is ready. |
|
PR build artifact for 192ae42 is ready. |
|
PR build artifact for 192ae42 is ready. |
mateuszm22
changed the title
mateuszm22
changed the title
|
PR build artifact for ee7fbb9 is ready. |
|
PR build artifact for ee7fbb9 is ready. |
|
PR build artifact for a304fc9 is ready. |
|
PR build artifact for a304fc9 is ready. |
|
PR build artifact for d3e745b is ready. |
|
PR build artifact for d3e745b is ready. |
|
PR build artifact for da78d7c is ready. |
|
PR build artifact for bc70302 is ready. |
|
PR build artifact for d1a238f is ready. |
|
PR build artifact for d1a238f is ready. |
packages/rn-tester-e2e/README.md
Outdated
| If you are M1 mac user: | ||
| 1. Install ffi package `gem install ffi -v '1.15.5' --source 'https://rubygems.org/'` | ||
| 2. Install pods with new architecture, e.g. using JSC `USE_HERMES=0 arch -x86_64 pod install` | ||
| 3. Open the generated `RNTesterPods.xcworkspace`. | ||
| 4. Build the app. |
There was a problem hiding this comment.
this should not be needed anymore (the ffi workaround)
There was a problem hiding this comment.
okay i will remove
| @@ -0,0 +1,21 @@ | |||
| Feature: Button component screen | |||
There was a problem hiding this comment.
this file should not be needed since we don't have cucumber in this PR, right?
There was a problem hiding this comment.
sure, thanks. fixed
packages/rn-tester-e2e/package.json
Outdated
| "babel-jest": "^26.6.3", | ||
| "eslint": "^7.32.0", | ||
| "jest": "^26.6.3", | ||
| "jest-html-reporter": "^3.7.0", | ||
| "metro-react-native-babel-preset": "^0.70.3", | ||
| "react-test-renderer": "18.0.0", |
There was a problem hiding this comment.
some of these dependencies need to be realigned with the rest of the monorepo
packages/rn-tester-e2e/README.md
Outdated
| @@ -0,0 +1,80 @@ | |||
| # Building the app | |||
There was a problem hiding this comment.
we're going to need some more docs on the readme, with instructions like how to setup Appium, etc.
There was a problem hiding this comment.
it is in README-addTestAndExecute
mateuszm22
changed the title
|
(cross-reference: second PR here: https://github.com/facebook/react-native/pull/35824/files) |
|
Crossposting here: please check out the comments that were left on the 2nd PR: as a part of them overlap with mines + a few more are actionable now; a third part need to be handled in the future. |
| @@ -0,0 +1 @@ | |||
| put app in this folder | |||
There was a problem hiding this comment.
nit: we should change the name of this file to README.txt so that it shows up by default if you navigate to it via ex. GH web ui
packages/rn-tester-e2e/jest.setup.js
Outdated
| import { beforeEach, afterEach, jest } from '@jest/globals'; | ||
|
|
||
|
|
||
| jest.setTimeout(40000); |
There was a problem hiding this comment.
the timeout is already set to 60000 in jest.config.js, any reason why it's re-set here and it's a different value?
There was a problem hiding this comment.
yep, removed this one and keep the value in jest.config.js
packages/rn-tester-e2e/jest.setup.js
Outdated
| path: '/wd/hub', | ||
| host: 'localhost', | ||
| port: 4723, | ||
| waitforTimeout: 30000, |
There was a problem hiding this comment.
yet another timeout with a different value from 60000 and 40000, let's just set the same one everywhere - or there's a reason why they are different?
There was a problem hiding this comment.
value has changed
packages/rn-tester-e2e/jest.setup.js
Outdated
| host: 'localhost', | ||
| port: 4723, | ||
| waitforTimeout: 30000, | ||
| logLevel: 'silent', |
There was a problem hiding this comment.
I think we might want it to be at least error:
| logLevel: 'silent', | |
| logLevel: 'error', |
| ios: '[label="Button"]', | ||
| android: '//android.view.ViewGroup/android.widget.TextView[@text="Button"]', |
There was a problem hiding this comment.
There was a problem hiding this comment.
this is the folder we need the files are: https://github.com/facebook/react-native/tree/main/packages/rn-tester/js
There was a problem hiding this comment.
why not use testID and accessibilityLabel ? so xpath will look like this $(//*[@content-desc="Button1" or @name="Button1"]) it will work for iOS and android. But I would use accessibilityLabels/testID $('~Button1') instead of xpath's. Xpaths lookups is really slow :)
There was a problem hiding this comment.
@lukutism that's what I'm proposing, you can also read more here https://github.com/kelset/react-native-e2e-jest-appium-webdriverio#notes-on-e2e-how-does-it-work
There was a problem hiding this comment.
overall general note, per Nick's comment here, it's likely we'll have to add a copyright note at the top of each *.js file as such:
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
* @format
*/
(we might not need the @flow @Format refs but we should look into that)
mateuszm22
force-pushed
the
feature/rn-tester-e2e
branch
from
8a411dd to
b8350e9
Compare
HeadsupWork is now moved here ---> #36267 |
Summary
The motivation is to create an e2e testing solution for the rn-tester default app. With cooperation with Callstack developers, we are trying to make a working tool. Now it's based on wdio+appium.
Next steps:
Changelog
[General] [Added] - Added first working configuration for e2e testing
Used tools
Test Plan
Follow Readme File
Videos
android_Screen.Recording.2022-11-10.at.12.43.52.mov
iOS_Screen.Recording.2022-11-10.at.11.03.23.mov