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

refactor: Improve readability for Execute in AppiumCommandExecutor.cs #629

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

Dor-bl
Copy link
Collaborator

@Dor-bl Dor-bl commented Aug 10, 2023

Change list

  1. Split the execute method into 3 small methods:
    a. HandleCommandException
    b. HandleCommandCompletion
    c. HandleNewSessionCommand

  2. Since I've noticed flakiness when we send commands, I've implemented a retry mechanism based on response status in the Execute method. The method now attempts command execution up to MaxRetryAttempts times, checking the success status of the response before returning. I'm still in between about this change, not sure if it's a good approach at all, all I know is that it has improved the stability of the tests.

Types of changes

What types of changes are you proposing/introducing to .NET client?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New feature (non-breaking change which adds value to the project)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Documentation

  • Have you proposed a file change/ PR with appium to update documentation?

This can be done by navigating to the documentation section on http://appium.io selecting the appropriate command/endpoint and clicking the 'Edit this doc' link to update the C# example

Integration tests

  • Have you provided integration tests to pass against the beta version of appium? (for Bugfix or New feature)

Details

Please provide more details about changes if it is necessary. If there are new features you can provide code samples which show the way they
work and possible use cases. Also you can create gists with pasted C# code samples or put them here using markdown.
About markdown please read Mastering markdown and Writing on GitHub

@Dor-bl Dor-bl mentioned this pull request Aug 10, 2023
6 tasks
@Dor-bl Dor-bl added this to the v5.0.0-rc1 milestone Aug 10, 2023
@Dor-bl Dor-bl removed this from the v5.0.0-rc1 milestone Sep 6, 2023
@laolubenson
Copy link
Collaborator

@Dor-bl sorry i missed this PR. Did you find the reason why the execute method is flaky?

@Dor-bl
Copy link
Collaborator Author

Dor-bl commented Sep 14, 2023

@laolubenson, all good.
Currently trying to fix the damage I did after the latest rebase :)

@Dor-bl
Copy link
Collaborator Author

Dor-bl commented Sep 14, 2023

@laolubenson, I'm in between here regarding the retry mechanism, it's not so elegant but does the trick when it comes to the flakiness of the execute method.
Maybe @mykola-mokhnach & @jlipps can add their input on why we have this issue in the first place that sometimes the server fails to succeed only after the 2nd try.

@Dor-bl Dor-bl marked this pull request as draft September 19, 2023 11:17
@Dor-bl Dor-bl marked this pull request as ready for review September 20, 2023 20:19
@Dor-bl Dor-bl changed the title refactor: Improve readability for Execute and add a retry logic in AppiumCommandExecutor.cs refactor: Improve readability for Execute in AppiumCommandExecutor.cs Sep 20, 2023
@Dor-bl Dor-bl added this to the v5.0.0-rc.2 milestone Sep 26, 2023
@Dor-bl Dor-bl merged commit 41ecc17 into appium:main Sep 28, 2023
3 checks passed
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.

4 participants