Skip to content
This repository has been archived by the owner on May 29, 2023. It is now read-only.

Add appium collection condition and appium condition classes #135

Merged

Conversation

amuthansakthivel
Copy link
Contributor

Proposed changes

Fix for selenide/selenide#2300

Checklist

  • Checkstyle and unit tests are passed locally with my changes by running gradlew check command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@amuthansakthivel
Copy link
Contributor Author

@asolntsev @BorisOsipov - Please review

@asolntsev
Copy link
Member

@amuthansakthivel Thank you for your contribution. Let's discuss it a bit.
What attributes do you really need to check? If you need to check texts, then probably this suggestion might be easier to use? selenide/selenide#2300

@sonarcloud
Copy link

sonarcloud bot commented Mar 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@amuthansakthivel
Copy link
Contributor Author

selenide/selenide#2300 (comment)

yes @asolntsev - Text is an attribute in appium and yes if we can fetch attributes dynamically that's what we need

@asolntsev
Copy link
Member

@amuthansakthivel But do I correctly understand that we still need to get different Text attributes in iOS and Android?

@amuthansakthivel
Copy link
Contributor Author

@amuthansakthivel But do I correctly understand that we still need to get different Text attributes in iOS and Android?

yes @asolntsev

@asolntsev
Copy link
Member

@amuthansakthivel Same question as in #151 (comment) :

Can't we make this api more generic?

I mean, this line is hard to read:

$(mobileElement)
      .shouldHave(AppiumCondition.attributeWithValue("content-desc", "name", "expected-value"));
  1. The reader doesn't know which one is for Android and which one is for iOS.
  2. What happens in a 3rd mobile OS will appear next year?

Maybe some builder for attributes like this?

$(mobileElement)
      .shouldHave(AppiumCondition.attributeWithValue(MobileAttributes.android("content-desc").ios("name"), "expected-value"));

@amuthansakthivel
Copy link
Contributor Author

@amuthansakthivel Same question as in #151 (comment) :

Can't we make this api more generic?

I mean, this line is hard to read:

$(mobileElement)
      .shouldHave(AppiumCondition.attributeWithValue("content-desc", "name", "expected-value"));
  1. The reader doesn't know which one is for Android and which one is for iOS.
  2. What happens in a 3rd mobile OS will appear next year?

Maybe some builder for attributes like this?

$(mobileElement)
      .shouldHave(AppiumCondition.attributeWithValue(MobileAttributes.android("content-desc").ios("name"), "expected-value"));

This looks good to me. But I need sometime to work on this @asolntsev - Little busy with personal side of things

@asolntsev asolntsev self-assigned this May 22, 2023
@sonarcloud
Copy link

sonarcloud bot commented May 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@asolntsev asolntsev modified the milestones: 2.8.0, 2.9.0 May 22, 2023
@asolntsev asolntsev merged commit ed7e5db into selenide:main May 22, 2023
5 of 6 checks passed
asolntsev added a commit that referenced this pull request May 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants