8000 💡 feat: auto alert configuration with setSettings · Issue #20867 · appium/appium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

💡 feat: auto alert configuration with setSettings #20867

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

Closed
krishtoautomate opened this issue Dec 30, 2024 · 14 comments
Closed

💡 feat: auto alert configuration with setSettings #20867

krishtoautomate opened this issue Dec 30, 2024 · 14 comments
Labels
Enhancement feature Help Needed contributions wanted!

Comments

@krishtoautomate
Copy link
krishtoautomate commented Dec 30, 2024

[update]
Please take a look at #20867 (comment) comment about this issue's scope.

Is your feature request related to a problem?

autoDismissAlerts feature is currently working with capabilities only and required only during launch and is not allowing to disable it between the tests or at certain steps as some native alert validation is important

Describe the solution you'd like.

set settings to set "autoDismissAlerts", true/false or "appium:autoDismissAlerts", true/false using setSettings as well:

driver.setSetting("autoDismissAlerts", true); driver.setSetting("appium:autoDismissAlerts", true);

driver.setSetting("autoDismissAlerts", false); driver.setSetting("appium:autoDismissAlerts", false);

Describe alternatives you've considered.

No response

Additional context

No response

@KazuCocoa KazuCocoa added the Help Needed contributions wanted! label Jan 6, 2025
@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 25, 2025

When the audoDismissAlerts or autoAcceptAlerts is enabled, a new session request starts with:

  if (capabilities[FB_SETTING_DEFAULT_ALERT_ACTION]) {
    [FBSession initWithApplication:app
                defaultAlertAction:(id)capabilities[FB_SETTING_DEFAULT_ALERT_ACTION]];
  } else {

https://github.com/appium/WebDriverAgent/blob/c296efabe83bf9b2fcd1650c06b363344c5535e0/WebDriverAgentLib/Commands/FBSessionCommands.m#L213-L216

The method does:

+ (instancetype)initWithApplication:(nullable XCUIApplication *)application
                 defaultAlertAction:(NSString *)defaultAlertAction
{
  FBSession *session = [self.class initWithApplication:application];
  session.alertsMonitor = [[FBAlertsMonitor alloc] init];
  session.alertsMonitor.delegate = (id<FBAlertsMonitorDelegate>)session;
  session.defaultAlertAction = [defaultAlertAction lowercaseString];
  [session.alertsMonitor enable];
  return session;
}

https://github.com/appium/WebDriverAgent/blob/c296efabe83bf9b2fcd1650c06b363344c5535e0/WebDriverAgentLib/Routing/FBSession.m#L129

When the session ends, the alert monitor will be killed like:

  if (nil != self.alertsMonitor) {
    [self.alertsMonitor disable];
    self.alertsMonitor = nil;
  }

https://github.com/appium/WebDriverAgent/blob/c296efabe83bf9b2fcd1650c06b363344c5535e0/WebDriverAgentLib/Routing/FBSession.m#L141-L144

So, in this issue, we want to achieve:

  1. Enable the alertsMonitor and set defaultAlertAction (accept or dismiss) when the setting is configured
  2. Disable the alertsMonitor

When the settings endpoint is called, https://github.com/appium/WebDriverAgent/blob/c296efabe83bf9b2fcd1650c06b363344c5535e0/WebDriverAgentLib/Commands/FBSessionCommands.m#L363 runs. It means we want to define a new settings name to configure the behavior.

@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 25, 2025

In summary:

What we need to do here is when the settings endpoint is called,

  1. Define autoAlerts naming to accept autoAlerts settings in https://github.com/appium/WebDriverAgent/blob/c296efabe83bf9b2fcd1650c06b363344c5535e0/WebDriverAgentLib/Utilities/FBSettings.m
  2. Add a method to accept the definition in handleSetSettings method
    driver.setSetting("autoAlerts", "accept");
    driver.setSetting("autoAlerts", "dismiss");
    driver.setSetting("autoAlerts", ""); # or driver.setSetting("autoAlerts", null), then disbale it.
    
  3. When autoAlerts is acceptable words such as accept or dismiss, the 2) will enable alertMonitor
    • alertMonitor itself is currently private, so you may need to expose it in FBSession to enable/disable alertMonitor as instance methods.
  4. Then, when autoAlerts is accept or dismiss -> enable alertMonitor with the given word for defaultAlertAction
    • null -> disbale it
    • other words -> return error with acceptable words or just disable it

All work can be done in WebDriverAgent project.

@mykola-mokhnach any opinion especially the settings' naming and the spec in 2)...?

cc @AutomatedTester

[update]
Please take a look at #20867 (comment)

@mykola-mokhnach
Copy link
Collaborator
mykola-mokhnach commented Jan 25, 2025

This makes sense, although the above approach is not very flexible. In many situations accept/dismiss does not project well into alert buttons because of how WDA logic is implemented (and because the limited context that XCTest provides). Eventually, alerts might have 1, 2 or 3 buttons, where "accept"/"dismiss" don't make much sense to me.

I was thinking about a possibility to provide a predicate to press a button when an alert pops up for the most flexible approach. For example:

autoClickAlertPredicate: 'label =[c] "allow" OR label =[c] "accept"' - to click either allow or accept buttons by comparing their labels case insensitive

autoClickAlertPredicate: 'index IN {0, 1}' - to click either the first or the second alert button

autoClickAlertPredicate: 'off' - to disable autoclick

@KazuCocoa KazuCocoa changed the title 💡 feat: AutoDismissAlerts with setSettings 💡 feat: auto alert configuration with setSettings Jan 25, 2025
@krishtoautomate
Copy link
Author
krishtoautomate commented Jan 25, 2025

Why not maintain some default names for accept/dismiss if not defined, example: for accept alerts : name IN {allow, yes, ok, accept} for dismiss : name IN {no, dismiss, dont}

@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 25, 2025

I was thinking of using the existing acceptAlertButtonSelector / dismissAlertButtonSelector to set concrete rules, so this was to enable/disable auto alert behavior only to not conflict rules with them.

@mykola-mokhnach
Copy link
Collaborator

Why not maintain some default names for accept/dismiss if not defined, example: for accept alerts : name IN {allow, yes, ok, accept} for dismiss : name IN {no, dismiss, dont}

This is also not flexible as button names vary as well as could be localized

@mykola-mokhnach
Copy link
Collaborator

I was thinking of using the existing acceptAlertButtonSelector / dismissAlertButtonSelector to set concrete rules, so this was to enable/disable auto alert behavior only to not conflict rules with them.

Above settings have different purpose. Also they use class chains rather than predicates.

@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 26, 2025

I probably understood.

I was talking about https://github.com/appium/WebDriverAgent/blob/master/WebDriverAgentLib/Routing/FBSession.m#L54-L72 such as acceptWithError called from https://github.com/appium/WebDriverAgent/blob/master/WebDriverAgentLib/Utilities/FBAlertsMonitor.m#L58 but your suggestion was fb_alertElement's detection logic in https://github.com/appium/WebDriverAgent/blob/master/WebDriverAgentLib/Utilities/FBAlertsMonitor.m#L56C34-L56C49 (https://github.com/appium/WebDriverAgent/blob/c296efabe83bf9b2fcd1650c06b363344c5535e0/WebDriverAgentLib/Categories/XCUIApplication%2BFBAlert.m#L69-L70) ?

So, autoClickAlertPredicate will customize the alert element's detection logic itself (what kind of elements the Appium/WDA handles as 'alert') and enable the alertsMonitor if given. It could affect other behavior which refers to fb_alertElement then...?

@mykola-mokhnach
Copy link
Collaborator
mykola-mokhnach commented Jan 26, 2025

So, autoClickAlertPredicate will customize the alert element's detection logic itself (what kind of elements the Appium/WDA handles as 'alert')

Not really. The alert detection logic remains the same. The given predicate must be executed inside of the alert element.

and enable the alertsMonitor if given

yes, correct. I assume it should also have priority over other alert-related capabilities

@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 26, 2025

Gotcha, I am getting a better understanding right now. It makes sense to accept the predicate idea itself for me, btw.

Then, what about:

"autoClickAlert": "accept" # does the existing 'appium:autoAcceptAlerts'. It can be customized with 'acceptAlertButtonSelector'
"autoClickAlert": "dismiss" # does the existing 'appium:autoDismissAlerts'. It can be customized with 'dismissAlertButtonSelector'
"autoClickAlert": "off" # or "null". Then appium tuens it off
"autoClickAlert": "accept predict rule" # https://github.com/appium/appium/issues/20938

So, "autoClickAlert": "accept predict rule" case, it will get a new case in https://github.com/appium/WebDriverAgent/blob/master/WebDriverAgentLib/Routing/FBSession.m#L54-L72 to handle the predicate logic. e.g. add a new method, named clickAlertPredicateWithError for instance, and it will do the given predicate over the acceptAlertButtonSelector / dismissAlertButtonSelector. To simplify, I'll create a new feature issue to add "autoClickAlert": "accept predict rule" case.

What about this...? Actually, my original intention in #20867 (comment) to accept a string as a parameter was to accept a more complex rule for future implementation.

@mykola-mokhnach
Copy link
Collaborator
mykola-mokhnach commented Jan 26, 2025

acceptPredicateWithError

I would not use accept/dismiss in that context because it might be confusing. This predicate is supposed to click the matched element on an alert as soon as it is detected, whether it would be accept or dismiss or anything else is up to the user to decide based on the provided predicate. If no element has been matched then no click is going to happen alas

@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 26, 2025

Oh, yea. The method naming example for WDA, when “autoClickAlert” gives predicate, was not intended. E.g. clickPredicateWithError, like that

@KazuCocoa
Copy link
Member
KazuCocoa commented Jan 27, 2025

Modified the naming in #20867 (comment) as clickAlertPredicateWithError. Also, have created #20938 to do the accept predict rule case only so that this issue can focus on the below cases if it looks good.

"autoClickAlert": "accept" # does the existing 'appium:autoAcceptAlerts'. It can be customized with 'acceptAlertButtonSelector'
"autoClickAlert": "dismiss" # does the existing 'appium:autoDismissAlerts'. It can be customized with 'dismissAlertButtonSelector'
"autoClickAlert": "off" # or "null". Then appium tuens it off

If we'd like to add only predicate string only as settings API, I'll close this issue as rejected (since this is to enable appium:autoDismissAlerts / appium:autoAcceptAlerts via settings API ordinally) and keep working on #20938 to discuss further. (I'm good for either)

@mykola-mokhnach
Copy link
Collaborator

Resolved by appium/a 6627 ppium-xcuitest-driver#2550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement feature Help Needed contributions wanted!
Projects
None yet
Development

No branches or pull requests

3 participants
0