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

fix: remove BASEDRIVER_HANDLED_SETTINGS since no longer exists in the base driver #498

Closed
wants to merge 2 commits into from

Conversation

KazuCocoa
Copy link
Member

@KazuCocoa KazuCocoa commented Jan 25, 2022

I noticed the BASEDRIVER_HANDLED_SETTINGS is no longer in the base driver since the settings are in images plugin. appium/appium#16368

So, this driver no longer needs BASEDRIVER_HANDLED_SETTINGS for now, which is driver based settings.
UIA2 driver only has plugins for the uia2 server one.

For example, test failures in https://dev.azure.com/AppiumCI/Appium%20CI/_build/results?buildId=19412&view=results (to use appium 2.0 as its test) failed by this undefined BASEDRIVER_HANDLED_SETTINGS

@mykola-mokhnach
Copy link
Contributor

AFAIK this change was done because uia2 throws an exception if an unknown setting name is supplied (which is the case while supplying image-related settings). I'm not quite sure how that is supposed to work now. @jlipps could you please provide more details?

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Jan 26, 2022

ah.. i see. I tested with below:

@driver.update_settings(
          {
            defaultImageTemplateScale: 4,
            imageMatchThreshold: 0.9,
            actionAcknowledgmentTimeout: 200
          }
        )

Then, only actionAcknowledgmentTimeout should be applied if no images plugin.
This PR returns an error. #499 did not apply all of them.

So, we need to update the server side to apply acceptable settings only in this case like xcuitest driver since we should expect other plugins also have their own settings apis.

@KazuCocoa KazuCocoa closed this Jan 26, 2022
@KazuCocoa KazuCocoa deleted the fix-settings-update branch January 26, 2022 08:26
@KazuCocoa KazuCocoa restored the fix-settings-update branch January 26, 2022 08:27
@KazuCocoa KazuCocoa reopened this Jan 26, 2022
@KazuCocoa
Copy link
Member Author

ah, maybe this change + server-side change to apply acceptable settings only as same as xcuitest-driver...?

@KazuCocoa
Copy link
Member Author

mm, no. The xcuitest-driver is still v1's base driver. So anyway this change and #499 are not enough.

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.

2 participants