Element search by 'class chain' locator type #442

mykola-mokhnach · 2017-01-09T10:05:11Z

The idea for this PR is borrowed from Adobe Flex open-source testing framework, where xpath queries are not supported natively. I did it, because I observe many people still build xpath queries, like this

/XCUIElementTypeApplication/XCUIElementTypeWindow/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther[2]/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeTable/XCUIElementTypeCell/XCUIElementTypeStaticText[2]

And they are quite slow, since xpath is not natively supported by XCTest.

My implementation uses the same query syntax as xpath (only class names and indexes are supported though), but the search itself takes much less time in comparison to xpath, because it looks for direct child nodes only and uses single native method call (childrenMatchingType) for this purpose. Examples of valid class chain queries:

XCUIElementTypeWindow/XCUIElementTypeButton[3] - select the third child button of the first child window element (indexing starts from 1. zero index is invalid, negative indexes are allowed)

XCUIElementTypeWindow - select all the children windows

XCUIElementTypeWindow[2] - select the second child window in the hierarchy. Indexing starts at 1

XCUIElementTypeWindow/XCUIElementTypeAny[3] - select the third child (of any type) of the first child window

XCUIElementTypeWindow/*[3] - same as above

XCUIElementTypeWindow[2]/XCUIElementTypeAny - select all the children of the second child window

XCUIElementTypeWindow[2]/XCUIElementTypeAny[-2] - select the second last child of the second child window

I've also implemented nice descriptions for syntax errors.

marekcirkos · 2017-01-10T12:35:36Z

@mykola-mokhnach Can you email me or poke me on Facebook?

mykola-mokhnach · 2017-01-10T12:49:25Z

@marekcirkos Sorry, but I'm not registered on FB and not gonna do it ever.
I'd be happy to contact you via email, but don't know your address.

jlipps

I don't have the obj c expertise to evaluate this code, but I like the idea!

marekcirkos

Please resolve conflicts.

Add tests for FBClassChainQueryParser.

I am not a big fun of this approach. It is super sensitive to tree hierarchy changes and class name changes. Instead people should use explicit accessibility identifiers and the whole xpath stuff is not needed.

marekcirkos · 2017-01-18T11:00:31Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+    @throw [NSException exceptionWithName:FBClassChainQueryParseException reason:error.localizedDescription userInfo:error.userInfo];
+    return nil;
+  }
+  NSMutableArray *unmatchedSnapshots = [NSMutableArray arrayWithArray:[self fb_lookupChain:parsedChain root:self.fb_lastSnapshot]];


or [self fb_lookupChain:parsedChain root:self.fb_lastSnapshot].mutableCopy

marekcirkos · 2017-01-18T11:03:03Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+  while (candidateElementIdx < candidateElements.count) {
+    XCUIElement *candidateElement = [candidateElements objectAtIndex:candidateElementIdx];
+    NSUInteger snapshotIdx = 0;
+    BOOL isMatchFound = NO;


This is always false. Doesn't seem like you need it

Good catch. Fixed.

marekcirkos · 2017-01-18T11:04:07Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+      break;
+    }
+    if (isMatchFound) {
+      [candidateElements removeObjectAtIndex:candidateElementIdx];


This never happens

marekcirkos · 2017-01-18T11:05:18Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+    candidateElements = [[[candidateElements reverseObjectEnumerator] allObjects] mutableCopy];
+  }
+  NSUInteger candidateElementIdx = 0;
+  while (candidateElementIdx < candidateElements.count) {


Fast enumeration should be better for (element in candidateElements)

Now this is OK after isMatchFound behaviour is fixed

I am not talking about speed only. Also readability will be much better

we need while loop here, because the candidateElements array is changed while iteration is in progress. This won't work for for .. in loop.

Why don't you simply iterate candidateElements? This is effectively what is happening, but is really hard to follow when reading this code.

[candidateElements removeObjectAtIndex:candidateElementIdx];

because of that. I cannot change the structure being currently iterated, otherwise this will corrupt my iterator.

I want to remove matched items immediately to reduce iterations count

marekcirkos · 2017-01-18T11:09:14Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+  FBClassChainElement *chainElement = [query firstObject];
+  NSArray *filteredChildren = root.children;
+  if (XCUIElementTypeAny != chainElement.type) {
+    filteredChildren = [filteredChildren filteredArrayUsingPredicate:[NSPredicate predicateWithFormat:@"elementType == %@", @(chainElement.type)]];


Please use FBStringify instead of plain strings, https://github.com/facebook/WebDriverAgent/blob/2eefc210ab3a775cfd1eff1b47e1a81484b64b50/WebDriverAgentLib/Commands/FBFindElementCommands.m#L77

fixed. thx for the hint

marekcirkos · 2017-01-18T11:10:56Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+{
+  self = [super init];
+  if (self) {
+    self.asString = stringValue;


marekcirkos · 2017-01-18T11:11:12Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+  return [NSCharacterSet characterSetWithCharactersInString:@""];
+}
+
+ (unsigned int)maxLength


NSUInteger

marekcirkos · 2017-01-18T11:12:36Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+
+@implementation FBClassChainQueryParser
+
+ (void)tokenizationErrorWithIndex:(unsigned int)index originalQuery:(NSString *)originalQuery error:(NSError **)error


If you accept an error you should return a BOOL to indicate whether operation was successful.

This is just a helper method to initialize NSError instance with extended description. Anyway, I've changed returned result type to BOOL.

marekcirkos · 2017-01-18T11:12:51Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+
+ (nullable NSArray<Token *> *)tokenizedQueryWithQuery:(NSString*)classChainQuery error:(NSError **)error
+{
+  unsigned int queryStringLength = (unsigned int)classChainQuery.length;


NSUInteger

marekcirkos · 2017-01-18T11:13:11Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface Token : NSObject


You need FB prefix in all of your classes. Also I would be more precise and name it something like FBClassChainToken ?

This is an internal class, which is not used anywhere else, except of this particular implementation . Do we still need the prefix

In Objc you need to always prefix class names.

mykola-mokhnach · 2017-01-18T12:26:24Z

I am not a big fun of this approach. It is super sensitive to tree hierarchy changes and class name changes. Instead people should use explicit accessibility identifiers and the whole xpath stuff is not needed

Me too, but I still see many people are using terribly long xpath locators autogenerated by inspector, because either application developers are too lazy to assign identifiers or these people don't know Appium/WDA features well enough. Class chain locator usage could be kind of quick patch for lookup speed in such case.

mykola-mokhnach · 2017-01-18T12:30:41Z

@marekcirkos There are already unit tests for chain parser. Please check FBClassChainTests.m in UnitTests folder.

mykola-mokhnach · 2017-01-31T15:54:34Z

@marekcirkos Are there any new comments to this PR?

marekcirkos · 2017-02-03T15:19:29Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.h

+ Returns an array of descendants matching given class chain query. This type of queries is a subset of xpath queries, but works much faster because of the internal implementation features.
+
+ @param classChainQuery requested class chain query. This query is constructed like xpath, but can only include indexes and valid class names. Only search by direct children elements of the current element is supported. Examples of such requests:
+ XCUIElementTypeWindow/XCUIElementTypeButton[3] - select the third child button of the first child window element


nit: I would keep @param short and keep juicy documentation above.

marekcirkos · 2017-02-03T15:26:01Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.h

+   -1 is the last child element and -2 is the second last element
+ @return FBClassChainElement instance
+ */
+- (id) initWithType:(XCUIElementType)type position:(NSInteger)position;


instancetype instead of id

Kill space after (id). I noticed it in few other places cross diff..

marekcirkos · 2017-02-03T15:29:46Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+  return YES;
+}
+
+ (nullable FBClassChain)compiledQueryWithTokenizedQuery:(NSArray<FBBaseClassChainToken *> *)tokenizedQuery originalQuery:(NSString *)originalQuery error:(NSError **)error


This is an NSArray not FBClassChain

+/*! Type alias for the product of class chain query parsing */
+typedef NSArray<FBClassChainElement *> * FBClassChain;

marekcirkos · 2017-02-03T15:30:50Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+- (NSArray<XCUIElement *> *)fb_descendantsMatchingClassChain:(NSString *)classChainQuery shouldReturnAfterFirstMatch:(BOOL)shouldReturnAfterFirstMatch
+{
+  NSError *error;
+  FBClassChain parsedChain = [FBClassChainQueryParser parseQuery:classChainQuery error:&error];


This NSArray not FBClassChain. FBClassChain is undefined, I can't find any definition.

+/*! Type alias for the product of class chain query parsing */
+typedef NSArray<FBClassChainElement *> * FBClassChain;

marekcirkos · 2017-02-03T15:35:31Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+
+@implementation XCUIElement (FBClassChain)
+
+- (NSArray<XCUIElement *> *)fb_descendantsMatchingClassChain:(NSString *)classChainQuery shouldReturnAfterFirstMatch:(BOOL)shouldReturnAfterFirstMatch


This method is really hard to read. Can you break it to smaller methods that are more descriptive?

sure will do

marekcirkos · 2017-02-03T15:38:59Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.h

+@interface FBClassChainElement : NSObject
+
+/*! Element's position */
+@property (readonly) NSInteger position;


you should make all properties nonatomic unless you really, really want them atomic.

marekcirkos · 2017-02-03T15:50:22Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+
+@implementation FBClassChainQueryParser
+
+ (BOOL)tokenizationErrorWithIndex:(NSUInteger)index originalQuery:(NSString *)originalQuery error:(NSError **)error


Why not return an error here, if it is a error constructor?

changed both methods to return NSError* instead

marekcirkos · 2017-02-03T15:51:26Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+  return result.copy;
+}
+
+ (BOOL)compilationErrorWithQuery:(NSString *)originalQuery description:(NSString *)description error:(NSError **)error


changed both method to return NSError* instead

marekcirkos · 2017-02-03T15:53:06Z

WebDriverAgentLib/Utilities/FBClassChainQueryParser.m

+        [self.class compilationErrorWithQuery:originalQuery description:description error:error];
+        return nil;
+      }
+      NSNumberFormatter *f = [[NSNumberFormatter alloc] init];


Formatters are expensive to create you should do this in constructor and reuse it.

ok, thanks for the hint

marekcirkos · 2017-02-03T15:53:41Z

WebDriverAgentTests/UnitTests/FBClassChainTests.m

+- (void)testInvalidChains
+{
+  NSArray *invalidQueries = @[
+                              @"/XCUIElementTypeWindow"


Fix spacing

marekcirkos · 2017-02-28T08:53:42Z

WebDriverAgentLib/Categories/XCUIElement+FBClassChain.m

+
+NSString *const FBClassChainQueryParseException = @"FBClassChainQueryParseException";
+
+@implementation XCUIElement (FBClassChain)


Can add fb_ prefix to all methods in that class? Let's land it then.

added and squashed

marekcirkos · 2017-02-28T10:54:20Z

Travis is not happy, looks like testSpecialXPathQuery and https://travis-ci.org/facebook/WebDriverAgent/jobs/206118659 are broken.

mykola-mokhnach · 2017-02-28T11:07:07Z

Sorry, my bad. Both should be fixed now

facebook-github-bot · 2017-03-02T11:15:01Z

@marekcirkos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

marekcirkos · 2017-03-02T12:09:19Z

Finally! Thank you @mykola-mokhnach for your patience

* Begin to work on CI metrics * Actively call patch function instead of passively running on require * Remove uuid dependency

Summary: The idea for this PR is borrowed from Adobe Flex open-source testing framework, where xpath queries are not supported natively. I did it, because I observe many people still build xpath queries, like this ``` /XCUIElementTypeApplication/XCUIElementTypeWindow/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther[2]/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeOther/XCUIElementTypeTable/XCUIElementTypeCell/XCUIElementTypeStaticText[2] ``` And they are quite slow, since xpath is not natively supported by XCTest. My implementation uses the same query syntax as xpath (only class names and indexes are supported though), but the search itself takes much less time in comparison to xpath, because it looks for direct child nodes only and uses single native method call (_childrenMatchingType_) for this purpose. Examples of valid class chain queries: ``` XCUIElementTypeWindow/XCUIElementTypeButton[3] - Closes facebookarchive/WebDriverAgent#442 Differential Revision: D4642444 Pulled By: marekcirkos fbshipit-source-id: 9c5e4dd884ddfde3e2f394de9a413f0015e30efb

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 9, 2017

jlipps reviewed Jan 17, 2017

View reviewed changes

marekcirkos suggested changes Jan 18, 2017

View reviewed changes

marekcirkos suggested changes Feb 3, 2017

View reviewed changes

marekcirkos reviewed Feb 28, 2017

View reviewed changes

Implement ‘class chain’ location strategy

57771e7

Address Travis issues

0e0a6af

facebook-github-bot added GH Review: accepted Import Started and removed GH Review: review-needed labels Feb 28, 2017

facebook-github-bot closed this in 64962d9 Mar 2, 2017

mykola-mokhnach deleted the class_chain_search branch March 6, 2017 16:06

mykola-mokhnach mentioned this pull request Mar 12, 2017

Add class chain locator to XCUITest driver appium/appium-xcuitest-driver#391

Merged

imurchie referenced this pull request in appium/WebDriverAgent Nov 16, 2017

Begin to work on CI metrics (#442)

be745d1

* Begin to work on CI metrics * Actively call patch function instead of passively running on require * Remove uuid dependency

hanneshauer mentioned this pull request May 23, 2020

fix: Update "find elements using"-strings appium/WebDriverAgent#334

Closed

snyk-bot mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities dongseok1985/WebDriverAgent#16

Open


		@implementation FBClassChainQueryParser

		+ (void)tokenizationErrorWithIndex:(unsigned int)index originalQuery:(NSString )originalQuery error:(NSError *)error


		@implementation XCUIElement (FBClassChain)

		- (NSArray<XCUIElement > )fb_descendantsMatchingClassChain:(NSString *)classChainQuery shouldReturnAfterFirstMatch:(BOOL)shouldReturnAfterFirstMatch


		@implementation FBClassChainQueryParser

		+ (BOOL)tokenizationErrorWithIndex:(NSUInteger)index originalQuery:(NSString )originalQuery error:(NSError *)error


		NSString *const FBClassChainQueryParseException = @"FBClassChainQueryParseException";

		@implementation XCUIElement (FBClassChain)


		NS_ASSUME_NONNULL_BEGIN

		@interface Token : NSObject

Element search by 'class chain' locator type #442

Element search by 'class chain' locator type #442

Conversation

mykola-mokhnach commented Jan 9, 2017

marekcirkos commented Jan 10, 2017

mykola-mokhnach commented Jan 10, 2017

jlipps left a comment

Choose a reason for hiding this comment

marekcirkos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach commented Jan 18, 2017

mykola-mokhnach commented Jan 18, 2017

mykola-mokhnach commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekcirkos commented Feb 28, 2017

mykola-mokhnach commented Feb 28, 2017

facebook-github-bot commented Mar 2, 2017

marekcirkos commented Mar 2, 2017

mykola-mokhnach Feb 3, 2017 •

edited

Loading