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 cli arg include-simulators #445

Conversation

kandji-joel
Copy link
Contributor

Closes #443

appium server -ka 800 --use-plugins=device-farm,appium-dashboard -pa /wd/hub --plugin-device-farm-platform=ios --plugin-device-farm-include-simulators=false

image

@saikrishna321
Copy link
Member

@kandji-joel Thanks for the PR. Can you please add Unit tests for this?

@saikrishna321
Copy link
Member

@kandji-joel Can you please fix the CI. Also, can you add tests which filter only simulators.

@kandji-joel
Copy link
Contributor Author

@saikrishna321 I fixed CI issues and added some tests -- I haven't wrote those style tests before so let me know you'd like to see some changes

Also, can you add tests which filter only simulators.

With the above implementation, you can only exclude simulators from real devices. You cannot exclude real devices from simulators. If you think it would be equally beneficial to exclude real devices I could change the argument from --plugin-device-farm-include-simulators=true(default)|false to something like --plugin-device-farm-device-types=both(default)|simulated|real. Thoughts?

@sudharsan-selvaraj
Copy link
Collaborator

sudharsan-selvaraj commented Jul 12, 2022

@saikrishna321 I fixed CI issues and added some tests -- I haven't wrote those style tests before so let me know you'd like to see some changes

Also, can you add tests which filter only simulators.

With the above implementation, you can only exclude simulators from real devices. You cannot exclude real devices from simulators. If you think it would be equally beneficial to exclude real devices I could change the argument from --plugin-device-farm-include-simulators=true(default)|false to something like --plugin-device-farm-device-types=both(default)|simulated|real. Thoughts?

@kandji-joel Introducing the generic cli option will be the ideal way to solve the issue. IMO, we can go with --plugin-device-farm-device-types option same as how we use plugin-device-farm-platform for uniformity.

@saikrishna321 @SrinivasanTarget Your thoughts?

@SrinivasanTarget
Copy link
Member

@sudharsan-selvaraj agree with you. Probably we can merge this as-is and improve the same over the next set of PRs. @kandji-joel what do you think?

@saikrishna321
Copy link
Member

Happy to merge! As @SrinivasanTarget mentioned we can handle it in another PR and also move these CLI args to server config file.

Thoughts @sudharsan-selvaraj?

@sudharsan-selvaraj
Copy link
Collaborator

@saikrishna321 Sounds good. We can merge this PR then.

@saikrishna321
Copy link
Member

@kandji-joel can you please fix conflict

@kandji-joel
Copy link
Contributor Author

@saikrishna321 @sudharsan-selvaraj sounds good to me, added an issue here #455

@saikrishna321 saikrishna321 merged commit bd9ec24 into AppiumTestDistribution:main Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Option to not include simulators
4 participants