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

device: add resetContentAndSettings #217

Merged
merged 2 commits into from
Aug 15, 2017

Conversation

formatlos
Copy link
Contributor

I added a new method to device to cleanup the device (like "Simulator > Reset Content and Settings" menu-item does), because I want to test specific screens which are only shown when the permissions are not set yet (e.g. a screen for explaining why the specific permission is needed)

@@ -173,6 +173,14 @@ class Fbsimctl {
await this._execFbsimctlCommand(options);
}

async resetContentAndSettings(udid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is better suited as an input param to device.launchApp(), inline with other API we have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would you name that param? clean?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure it fits launchApp since it has no relation to the app, it's more of a device function (like setLocation or setOrientation)

Copy link
Member

Choose a reason for hiding this comment

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

clean can be confusing, since a user might think this option cleans the app, not resets the entire simulator.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so is it alright then or do you want me to change something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LeoNatan just wanted to know if there are still concerns about the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. Need @rotemmiz to approve it, as he is the JS master around here. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Never wished for this title :( ...
Let's talk a bit about the API:

  1. I don't this is not a good API for reseting a permission, I much prefer if we can add an unset state to permissions with appleSimUtils Unsetting an already set permission AppleSimulatorUtils#8
  2. Can there still can be a valid test case for resetting a device?
  3. Since we aim Detox to be multi-platform framework I prefer not to give this method a name which is strongly recognized with Apple's terms, let's try thinking of something more generic maybe, like reset or clearData (not sure if these are better names). WDYT ?

@@ -1,4 +1,4 @@
describe('Device', () => {
describe.only('Device', () => {
Copy link
Member

Choose a reason for hiding this comment

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

don't forget to remove the only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, good point

@@ -173,6 +173,14 @@ class Fbsimctl {
await this._execFbsimctlCommand(options);
}

async resetContentAndSettings(udid) {
Copy link
Member

Choose a reason for hiding this comment

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

Never wished for this title :( ...
Let's talk a bit about the API:

  1. I don't this is not a good API for reseting a permission, I much prefer if we can add an unset state to permissions with appleSimUtils Unsetting an already set permission AppleSimulatorUtils#8
  2. Can there still can be a valid test case for resetting a device?
  3. Since we aim Detox to be multi-platform framework I prefer not to give this method a name which is strongly recognized with Apple's terms, let's try thinking of something more generic maybe, like reset or clearData (not sure if these are better names). WDYT ?

@formatlos
Copy link
Contributor Author

formatlos commented Aug 14, 2017

  1. Well that's why I didn't call it unsetPermission whatever ... So it's not just about resetting the permissions, that was just one example. Even more important for me is to be sure to start with a clean state of the simulator. Once in a while my detox tests start to fail, because the simulator seems to be in a "dirty" state (after resetContentAndSettings everything runs as expected again).
    Another thing would be to clean up the bundle Documents folder, local DBs, etc.

  2. good question

  3. In general I also don't like the name being tied to one specific Platform. I also thought about reset or something along the lines in the first place, but in the end decided against it for one specific reason: It's not just resetting this one app and not just cleaning the data, it's resetting the whole simulator to a clean state.
    Also there might not even be a similar thing in android, so I thought it's better to stick with the ios term and not try to introduce new nomenclature for something that doesn't exist platform independent anyway.
    And also: if there is the functionality for android to reset all content and settings of the device the name resetContentAndSettings would be more than appropriate also for android, right?
    Apart from this concerns I'm also fine with renaming it...

@rotemmiz rotemmiz merged commit 4814bac into wix:master Aug 15, 2017
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants