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

Add detox tests for Switch #22470

Closed
wants to merge 4 commits into from
Closed

Conversation

elicwhite
Copy link
Member

Reorganized some of the switch examples to be more testable:

Before:
simulator screen shot - iphone xs - 2018-12-01 at 02 27 47

After:
simulator screen shot - iphone xs - 2018-12-01 at 02 27 06

Tests pass!

yarn build-ios-e2e && yarn test-ios-e2e

screen shot 2018-12-01 at 2 20 33 am

Changelog:

Help reviewers and the release process by writing your own changelog entry. When the change doesn't impact React Native developers, it may be ommitted from the changelog for brevity. See below for an example.

[Internal] [Added] - Detox tests for Switch

@facebook-github-bot 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 1, 2018
@pull-bot
Copy link

pull-bot commented Dec 1, 2018

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

⚠️

📋 Changelog - This PR may have incorrectly formatted Changelog.

Generated by 🚫 dangerJS

await expect(element(by.id(onTestID))).toHaveValue('1');
await expect(element(by.id(onIndicatorID))).toHaveText('On');

try {
Copy link
Member Author

@elicwhite elicwhite Dec 1, 2018

Choose a reason for hiding this comment

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

This feels gross. When tapping on an element that is disabled, EarlGrey throws the following error:

err:  Cannot perform action due to constraint(s) failure.
    Exception with Action: {
      "Action Name":  "Tap",
      "Element Description":  "<RCTSwitch:0x7f91a1615b60; AX=Y; AX.id='disabled-initial-on'; AX.value='1'; AX.frame={{20.5, 295}, {51, 31}}; AX.activationPoint={46, 310.5}; AX.traits='UIAccessibilityTraitButton'; AX.focused='N'; frame={{0, 0}, {51, 31}}; opaque; alpha=1; disabled>",
      "Failed Constraint(s)":  "enabled",
      "All Constraint(s)":  "(!(isSystemAlertViewShown) && ((respondsToSelector(isAccessibilityElement) && isAccessibilityElement) || kindOfClass('UIView')) && (enabled && !(((kindOfClass('UIView') || respondsToSelector(accessibilityContainer)) && ancestorThatMatches(!(enabled))))) && interactable)",
      "Recovery Suggestion":  "Adjust element properties so that it matches the failed constraint(s)."
    }
    
    
    [
      {
        "Description":  "Cannot perform action due to constraint(s) failure.",
        "Error Domain":  "com.google.earlgrey.ElementInteractionErrorDomain",
        "Error Code":  "1",
        "File Name":  "GREYBaseAction.m",
        "Function Name":  "-[GREYBaseAction satisfiesConstraintsForElement:error:]",
        "Line":  "66"
      }
    ]

any ideas of a cleaner way to handle this case @rotemmiz?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely have better error logs, but what do you mean by cleaner way?

Copy link
Member Author

Choose a reason for hiding this comment

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

One example of something cleaner would be using jest's expect .toThrow. In this example it would look like this:

await jestExpect(element(by.id(onTestID)).tap()).rejects.toThrow('Cannot perform action due to constraint(s) failure');

However, the error we get back is actually an Error object, with the message field set to an Error, with that's message field set to the long string, making it impossible to assert as if it was a normal error.


class BasicSwitchExample extends React.Component<{}, $FlowFixMeState> {
type OnOffIndicatorProps = $ReadOnly<{|on: boolean, testID: string|}>;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking I'll end up pulling these out to use on more tests but I don't want to overabstract until we know they are needed

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@elicwhite
Copy link
Member Author

Apparently there are screenshot tests for the SwitchExample file in RNTester.

I tried to follow the instructions on the website to rerecord them but ran into a snag.

I made this change:

--- a/RNTester/RNTesterIntegrationTests/RNTesterIntegrationTests.m
+++ b/RNTester/RNTesterIntegrationTests/RNTesterIntegrationTests.m
@@ -36,7 +36,7 @@
 - (void)setUp
 {
   _runner = RCTInitRunnerForApp(@"IntegrationTests/IntegrationTestsApp", nil, nil);
-  _runner.recordMode = NO;
+  _runner.recordMode = YES;
 }

And ran

./scripts/objc-test-ios.sh test

However this tries to launch an iOS 11.4 simulator and I apparently only have iOS 12 simulators installed:

xcodebuild: error: Unable to find a destination matching the provided destination specifier:
		{ platform:iOS Simulator, OS:11.4, name:iPhone 5s }

	The requested device could not be found because no available devices matched the request.

	Available destinations for the "RNTester" scheme:
		{ platform:iOS Simulator, id:29147A0B-D51B-4655-90D1-BCABEFA7D4CD, OS:12.1, name:iPad (5th generation) }
		{ platform:iOS Simulator, id:7BDE7B0B-DE31-4BF6-A335-E1656073AD7B, OS:12.1, name:iPad (6th generation) }
		{ platform:iOS Simulator, id:F78A1E21-BE4F-42B4-9F2E-A415CCBB97E3, OS:12.1, name:iPad Air }
		{ platform:iOS Simulator, id:5CBC191C-9BD4-43F5-A939-F512D3B216C4, OS:12.1, name:iPad Air 2 }
		{ platform:iOS Simulator, id:27A6B7FC-4F73-4952-87D0-A6AAF027B469, OS:12.1, name:iPad Pro (9.7-inch) }
		{ platform:iOS Simulator, id:9BD1664F-47C8-4689-8A93-C4C43997CDFB, OS:12.1, name:iPad Pro (10.5-inch) }
		{ platform:iOS Simulator, id:0F8845AA-8170-466A-A224-542B8E096059, OS:12.1, name:iPad Pro (11-inch) }
		{ platform:iOS Simulator, id:7090B030-ECBD-438A-835B-366D54AA679F, OS:12.1, name:iPad Pro (12.9-inch) }
		{ platform:iOS Simulator, id:FBFAF668-035F-46BF-A3FE-D50308CF72D3, OS:12.1, name:iPad Pro (12.9-inch) (2nd generation) }
		{ platform:iOS Simulator, id:0E16C8BA-86F7-43D9-B2BB-79666976E004, OS:12.1, name:iPad Pro (12.9-inch) (3rd generation) }
		{ platform:iOS Simulator, id:535EE36C-2EB6-44B1-8813-7E86BCE76A61, OS:12.1, name:iPhone 5s }
		{ platform:iOS Simulator, id:ECD88DE1-534C-45A5-B305-AC9BAA3CE8B5, OS:12.1, name:iPhone 6 }
		{ platform:iOS Simulator, id:996F77F5-C384-46DB-AB2E-2D191491E11B, OS:12.1, name:iPhone 6 Plus }
		{ platform:iOS Simulator, id:B686631F-1FD8-43FF-97B1-1A1945BBBA9F, OS:12.1, name:iPhone 6s }
		{ platform:iOS Simulator, id:621E7397-1E3B-45EA-AE59-60667EEB1ED8, OS:12.1, name:iPhone 6s Plus }
		{ platform:iOS Simulator, id:A0AB5A8A-1BA9-4BB3-B1FD-C3454F3A4251, OS:12.1, name:iPhone 7 }
		{ platform:iOS Simulator, id:BD2F1B5B-A876-4ED5-BEA2-479C5D4C57D8, OS:12.1, name:iPhone 7 Plus }
		{ platform:iOS Simulator, id:ED5E4E30-C497-4F6C-A0DB-1CE2B72FB126, OS:12.1, name:iPhone 8 }
		{ platform:iOS Simulator, id:201C55BF-0169-486D-AE81-026EAEA3FCC2, OS:12.1, name:iPhone 8 Plus }
		{ platform:iOS Simulator, id:9B844351-B4D3-40CD-9968-0D35A8087CDE, OS:12.1, name:iPhone SE }
		{ platform:iOS Simulator, id:019FF61D-4F01-4A6E-AE5B-9AC391F647D1, OS:12.1, name:iPhone X }
		{ platform:iOS Simulator, id:5D5535E3-42BA-4E54-966F-11D6FE684BDD, OS:12.1, name:iPhone XR }
		{ platform:iOS Simulator, id:3CEF4AA7-E60F-4D73-8100-2CE35289768F, OS:12.1, name:iPhone XS }
		{ platform:iOS Simulator, id:10F5EBCA-9B41-4B9F-9F76-CBF4AE3EC233, OS:12.1, name:iPhone XS Max }

@shergin, do you know how to re-record these? Also, why didn't the screenshot tests fail on the PR? Is nothing running them? cc @hramos

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

function ExampleRow({children}: ExampleRowProps) {
return (
<View
style={{

Choose a reason for hiding this comment

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

react-native/no-inline-styles: Inline style: { flexDirection: 'row',
justifyContent: 'space-between',
alignItems: 'center',
marginBottom: 10 }

@react-native-bot
Copy link
Collaborator

@TheSavior merged commit 4dea677 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Dec 5, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Dec 5, 2018
@hramos
Copy link
Contributor

hramos commented Dec 7, 2018

Re: screenshot tests. It looks like they are running on Circle, but we're not handling the exit code correctly when the test script is invoked so Circle thinks the job was successful. I'll fix it.

grabbou pushed a commit that referenced this pull request Dec 17, 2018
Summary:
Reorganized some of the switch examples to be more testable:

Before:
![simulator screen shot - iphone xs - 2018-12-01 at 02 27 47](https://user-images.githubusercontent.com/249164/49327066-bcc35580-f510-11e8-860d-fc07a574f80c.png)

After:
![simulator screen shot - iphone xs - 2018-12-01 at 02 27 06](https://user-images.githubusercontent.com/249164/49327068-bf25af80-f510-11e8-95c6-7aa4a9095b91.png)

Tests pass!

```
yarn build-ios-e2e && yarn test-ios-e2e
```
<img width="711" alt="screen shot 2018-12-01 at 2 20 33 am" src="https://user-images.githubusercontent.com/249164/49327070-c64cbd80-f510-11e8-8cad-84c3fe42941e.png">

Changelog:
----------
Help reviewers and the release process by writing your own changelog entry. When the change doesn't impact React Native developers, it may be ommitted from the changelog for brevity. See below for an example.

[Internal] [Added] - Detox tests for Switch
Pull Request resolved: #22470

Reviewed By: RSNara

Differential Revision: D13290329

Pulled By: TheSavior

fbshipit-source-id: 91c1b895dd5e1acc4330618e6d3165c7f9215997
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
Reorganized some of the switch examples to be more testable:

Before:
![simulator screen shot - iphone xs - 2018-12-01 at 02 27 47](https://user-images.githubusercontent.com/249164/49327066-bcc35580-f510-11e8-860d-fc07a574f80c.png)

After:
![simulator screen shot - iphone xs - 2018-12-01 at 02 27 06](https://user-images.githubusercontent.com/249164/49327068-bf25af80-f510-11e8-95c6-7aa4a9095b91.png)

Tests pass!

```
yarn build-ios-e2e && yarn test-ios-e2e
```
<img width="711" alt="screen shot 2018-12-01 at 2 20 33 am" src="https://user-images.githubusercontent.com/249164/49327070-c64cbd80-f510-11e8-8cad-84c3fe42941e.png">

Changelog:
----------
Help reviewers and the release process by writing your own changelog entry. When the change doesn't impact React Native developers, it may be ommitted from the changelog for brevity. See below for an example.

[Internal] [Added] - Detox tests for Switch
Pull Request resolved: facebook#22470

Reviewed By: RSNara

Differential Revision: D13290329

Pulled By: TheSavior

fbshipit-source-id: 91c1b895dd5e1acc4330618e6d3165c7f9215997
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 18, 2019
Summary:
Reorganized some of the switch examples to be more testable:

Before:
![simulator screen shot - iphone xs - 2018-12-01 at 02 27 47](https://user-images.githubusercontent.com/249164/49327066-bcc35580-f510-11e8-860d-fc07a574f80c.png)

After:
![simulator screen shot - iphone xs - 2018-12-01 at 02 27 06](https://user-images.githubusercontent.com/249164/49327068-bf25af80-f510-11e8-95c6-7aa4a9095b91.png)

Tests pass!

```
yarn build-ios-e2e && yarn test-ios-e2e
```
<img width="711" alt="screen shot 2018-12-01 at 2 20 33 am" src="https://user-images.githubusercontent.com/249164/49327070-c64cbd80-f510-11e8-8cad-84c3fe42941e.png">

Changelog:
----------
Help reviewers and the release process by writing your own changelog entry. When the change doesn't impact React Native developers, it may be ommitted from the changelog for brevity. See below for an example.

[Internal] [Added] - Detox tests for Switch
Pull Request resolved: facebook/react-native#22470

Reviewed By: RSNara

Differential Revision: D13290329

Pulled By: TheSavior

fbshipit-source-id: 91c1b895dd5e1acc4330618e6d3165c7f9215997
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: Switch Merged This PR has been merged. p: Facebook Partner: Facebook Partner Tests This PR adds or edits a test case.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants