Skip to content

Remove persistence from RNTester app#22596

Closed
CodingItWrong wants to merge 4 commits intofacebook:masterfrom
CodingItWrong:remove-persistence
Closed

Remove persistence from RNTester app#22596
CodingItWrong wants to merge 4 commits intofacebook:masterfrom
CodingItWrong:remove-persistence

Conversation

@CodingItWrong
Copy link
Copy Markdown
Contributor

@CodingItWrong CodingItWrong commented Dec 11, 2018

Previously the RNTester app saved what screen you were on and what filter text was entered into the initial screen. This made e2e testing complex, as each test needed to manually restore the state to the home screen. If the state ever got out of sync with the test's expectations, it could lead to multiple failed tests.

There is still one specific component that uses persistence: RNTesterSettingSwitchRow. Persistence can be removed from this component next time tests for it are updated. As a result, RNTesterStatePersister is not yet entirely removed from the app.

Test Plan:

CI running yarn test-ios-e2e will confirm the tests still pass.

You can also run RNTester manually to confirm that after quitting and restarting the app, you're returned to the home screen of the app, and the filter state is not saved.

Changelog:

[General] [Removed] - Remove persistence from RNTester app

@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 11, 2018
@elicwhite
Copy link
Copy Markdown
Member

Looks like you have a flow error:

Error ----------------------------------------------------------------------------- RNTester/js/RNTesterApp.ios.js:119:9

Cannot create `RNTesterExampleList` element because property `searchTextInputStyle` is missing in props [1] but exists
in `Props` [2].

   RNTester/js/RNTesterApp.ios.js:119:9
                v-------------------
   119|         <RNTesterExampleList
   120|           onNavigate={this._handleAction}
   121|           list={RNTesterList}
   122|         />
                -^ [1]

References:
   RNTester/js/RNTesterExampleList.js:72:51
    72| class RNTesterExampleList extends React.Component<Props, $FlowFixMeState> {
                                                          ^^^^^ [2]

@CodingItWrong
Copy link
Copy Markdown
Contributor Author

I'm not 100% sure if the above fix for the Flow error is correct. But I can't find anywhere the searchTextInputStyle was passed. So it seems like something about previously using RNTesterStatePersister was preventing Flow from erroring out although that prop was not passed. So the correct fix seems to be to remove the unused prop.

@CodingItWrong
Copy link
Copy Markdown
Contributor Author

@TheSavior bump; passing now

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Dec 11, 2018
Copy link
Copy Markdown
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.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Copy Markdown
Contributor

cpojer commented Dec 11, 2018

Looks good to me! Thanks for improving the RN test infra <3

@react-native-bot
Copy link
Copy Markdown
Collaborator

@CodingItWrong merged commit 33fb70f into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Dec 11, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Dec 11, 2018
grabbou pushed a commit that referenced this pull request Jan 3, 2019
Summary:
Previously the RNTester app saved what screen you were on and what filter text was entered into the initial screen. This made e2e testing complex, as each test needed to manually restore the state to the home screen. If the state ever got out of sync with the test's expectations, it could lead to multiple failed tests.

There is still one specific component that uses persistence: `RNTesterSettingSwitchRow`. Persistence can be removed from this component next time tests for it are updated. As a result, `RNTesterStatePersister` is not yet entirely removed from the app.
Pull Request resolved: #22596

Differential Revision: D13413457

Pulled By: cpojer

fbshipit-source-id: 3faa26a94139397b4bce6b62ff43e9c2f870b145
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
Previously the RNTester app saved what screen you were on and what filter text was entered into the initial screen. This made e2e testing complex, as each test needed to manually restore the state to the home screen. If the state ever got out of sync with the test's expectations, it could lead to multiple failed tests.

There is still one specific component that uses persistence: `RNTesterSettingSwitchRow`. Persistence can be removed from this component next time tests for it are updated. As a result, `RNTesterStatePersister` is not yet entirely removed from the app.
Pull Request resolved: facebook#22596

Differential Revision: D13413457

Pulled By: cpojer

fbshipit-source-id: 3faa26a94139397b4bce6b62ff43e9c2f870b145
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 18, 2019
Summary:
Previously the RNTester app saved what screen you were on and what filter text was entered into the initial screen. This made e2e testing complex, as each test needed to manually restore the state to the home screen. If the state ever got out of sync with the test's expectations, it could lead to multiple failed tests.

There is still one specific component that uses persistence: `RNTesterSettingSwitchRow`. Persistence can be removed from this component next time tests for it are updated. As a result, `RNTesterStatePersister` is not yet entirely removed from the app.
Pull Request resolved: facebook/react-native#22596

Differential Revision: D13413457

Pulled By: cpojer

fbshipit-source-id: 3faa26a94139397b4bce6b62ff43e9c2f870b145
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. Merged This PR has been merged. Tests This PR adds or edits a test case.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants