Add e2e tests, bug fixes for testIDs #22537

rickhanlonii · 2018-12-06T02:41:54Z

Overview

This PR adds e2e tests for the Picker and DatePicker components.

While writing these tests, I also found and fixed two bugs where we wern't passing the testID down to the native components, so detox couldn't look them up. This confirms what was mentioned by @rotemmiz here

Test Plan:

Changelog:

[General] [Fixed] - Fix testIDs not being passed in Picker and DatePicker

rickhanlonii · 2018-12-06T02:43:10Z

RNTester/js/DatePickerIOSExample.js

-              this.state.date.toLocaleTimeString()}
+              this.state.date.toLocaleTimeString([], {
+                hour: '2-digit',
+                minute: '2-digit',


I switched the formatting here since having seconds is non-deterministic

rickhanlonii · 2018-12-06T02:43:42Z

RNTester/e2e/config.json

@@ -1,5 +1,5 @@
 {
-  "setupTestFrameworkScriptFile" : "./test-init.js",
+  "setupFilesAfterEnv" : ["./test-init.js"],


This is a Jest change that was printing a warning

Can you do that in a separate commit?

elicwhite · 2018-12-06T19:46:27Z

it is surprising to me that each example in the DatePickerIOS suite is so slow, more than twice as long as the other tests:

Is setColumnToValue just really slow? Can you experiment a little and see if you can identify which command is taking so long?

PASS  RNTester/e2e/__tests__/Switch-test.js (40.372s)
  Switch
    ✓ Switch that starts on should switch (801ms)
    ✓ Switch that starts off should switch (704ms)
    ✓ disabled switch should not toggle (398ms)

detox[25343] INFO:  [DetoxServer.js] server listening on localhost:49370...
 PASS  RNTester/e2e/__tests__/DatePickerIOS-test.js (44.554s)
  DatePickerIOS
    ✓ Should change indicator with datetime picker (6351ms)
    ✓ Should change indicator with date-only picker (5990ms)
    ✓ Should change indicator with time-only picker (6282ms)

detox[25343] INFO:  [DetoxServer.js] server listening on localhost:49422...
 PASS  RNTester/e2e/__tests__/Button-test.js (34.574s)
  Button
    ✓ Simple button should be tappable (1546ms)
    ✓ Adjusted color button should be tappable (1909ms)
    ✓ Two buttons with JustifyContent:'space-between' should be tappable (3824ms)
    ✓ Disabled button should not interact (1271ms)

detox[25343] INFO:  [DetoxServer.js] server listening on localhost:49442...
 PASS  RNTester/e2e/__tests__/Picker-test.js (26.454s)
  Picker
    ✓ should be selectable by ID (626ms)

elicwhite · 2018-12-06T19:48:38Z

RNTester/js/RNTesterPage.js

@@ -41,7 +41,10 @@ class RNTesterPage extends React.Component<Props> {
    return (
      <View style={styles.container}>
        {title}
-        <ContentWrapper style={styles.wrapper} {...wrapperProps}>
+        <ContentWrapper
+          testID="scroll-view"


Should this have a more specific testID?

elicwhite · 2018-12-06T19:52:32Z

Based on the video it seems like scrolling to the bottom could glitch out if the place it puts the cursor down is in the middle of the picker, scrolling the picker instead of the scrollview. Not sure if that is how detox works though. Can you see if you can cause that issue, if it can then it would be a footgun we'll for sure hit later when running this on a different device size or changing the contents of the view.

rickhanlonii · 2018-12-06T22:42:12Z

I think it's slow in aggregate because they have 6 or 7 actions/assertion, each taking a good amount of time. Here's an analysis of work time per statement in the first test block (note I ran the analysis multiple times with similar results).

Legend for the statements tested:

// 0, no actions/assertions

await expect(indicator).toBeVisible(); // 1
await expect(testElement).toBeVisible(); // 2

await testElement.setColumnToValue(0, 'Dec 4'); // 3
await testElement.setColumnToValue(1, '4'); // 4
await testElement.setColumnToValue(2, '10'); // 5
await testElement.setColumnToValue(3, 'AM'); // 6

await expect(indicator).toHaveText('12/4/2005 4:10 AM'); // 7

Results:

Line	Method	Agg ms	Δ ms	Δ %
0	-	3	-	-
1	toBeVisible	768	765	%%
2	toBeVisible	1,224	456	59%
3	setColumnToValue	2,046	822	67%
4	setColumnToValue	2,349	303	15%
5	setColumnToValue	3,366	1,017	43%
6	setColumnToValue	4,216	850	25%
7	toHaveText	5,253	1,037	25%

rickhanlonii · 2018-12-07T00:56:04Z

@TheSavior good find on the scroll issue, here's what happens

I removed the test for now, but will revisit a generic filtering strategy to use on every test page instead of scrolling 👍

facebook-github-bot

@rickhanlonii has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@rickhanlonii has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@rickhanlonii has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@rickhanlonii has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2018-12-07T23:00:05Z

@rickhanlonii merged commit 9fdbf60 into facebook:master.

Summary: This PR adds e2e tests for the Picker and DatePicker components. While writing these tests, I also found and fixed two bugs where we wern't passing the `testID` down to the native components, so detox couldn't look them up. This confirms what was mentioned by rotemmiz [here](wix/Detox#798 (comment)) Pull Request resolved: #22537 Reviewed By: cpojer Differential Revision: D13371307 Pulled By: rickhanlonii fbshipit-source-id: a4dfcdb5913645bceca0c7353328eeb9ad0f6558

Summary: This PR adds e2e tests for the Picker and DatePicker components. While writing these tests, I also found and fixed two bugs where we wern't passing the `testID` down to the native components, so detox couldn't look them up. This confirms what was mentioned by rotemmiz [here](wix/Detox#798 (comment)) Pull Request resolved: facebook#22537 Reviewed By: cpojer Differential Revision: D13371307 Pulled By: rickhanlonii fbshipit-source-id: a4dfcdb5913645bceca0c7353328eeb9ad0f6558

Summary: This PR adds e2e tests for the Picker and DatePicker components. While writing these tests, I also found and fixed two bugs where we wern't passing the `testID` down to the native components, so detox couldn't look them up. This confirms what was mentioned by rotemmiz [here](wix/Detox#798 (comment)) Pull Request resolved: facebook/react-native#22537 Reviewed By: cpojer Differential Revision: D13371307 Pulled By: rickhanlonii fbshipit-source-id: a4dfcdb5913645bceca0c7353328eeb9ad0f6558

Add detox tests, bug fixes for testIDs

8be4aa2

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Tests This PR adds or edits a test case. labels Dec 6, 2018

rickhanlonii commented Dec 6, 2018

View reviewed changes

rickhanlonii requested a review from elicwhite December 6, 2018 02:43

Fix flow

502fde6

elicwhite reviewed Dec 6, 2018

View reviewed changes

elicwhite approved these changes Dec 6, 2018

View reviewed changes

LeoNatan mentioned this pull request Dec 6, 2018

UIPickerView does not contain desired value wix/Detox#1051

Closed

Remove test

ddd4129

facebook-github-bot reviewed Dec 7, 2018

View reviewed changes

rm scroll-view test id

673be06

facebook-github-bot reviewed Dec 7, 2018

View reviewed changes

Prettier

69c62a7

facebook-github-bot reviewed Dec 7, 2018

View reviewed changes

Revert config field rename

10e4a4a

facebook-github-bot reviewed Dec 7, 2018

View reviewed changes

facebook-github-bot closed this in 9fdbf60 Dec 7, 2018

facebook locked as resolved and limited conversation to collaborators Dec 7, 2018

react-native-bot added the Merged This PR has been merged. label Dec 7, 2018

zpao deleted the rh-add-detox-tests branch January 31, 2019 01:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add e2e tests, bug fixes for testIDs #22537

Add e2e tests, bug fixes for testIDs #22537

rickhanlonii commented Dec 6, 2018

rickhanlonii Dec 6, 2018

rickhanlonii Dec 6, 2018

cpojer Dec 7, 2018

elicwhite commented Dec 6, 2018 •

edited

Loading

elicwhite Dec 6, 2018

elicwhite commented Dec 6, 2018

rickhanlonii commented Dec 6, 2018

rickhanlonii commented Dec 7, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

react-native-bot commented Dec 7, 2018

Add e2e tests, bug fixes for testIDs #22537

Add e2e tests, bug fixes for testIDs #22537

Conversation

rickhanlonii commented Dec 6, 2018

Overview

Test Plan:

Changelog:

rickhanlonii Dec 6, 2018

Choose a reason for hiding this comment

rickhanlonii Dec 6, 2018

Choose a reason for hiding this comment

cpojer Dec 7, 2018

Choose a reason for hiding this comment

elicwhite commented Dec 6, 2018 • edited Loading

elicwhite Dec 6, 2018

Choose a reason for hiding this comment

elicwhite commented Dec 6, 2018

rickhanlonii commented Dec 6, 2018

rickhanlonii commented Dec 7, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Dec 7, 2018

elicwhite commented Dec 6, 2018 •

edited

Loading