-
Notifications
You must be signed in to change notification settings - Fork 742
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
Add grey_firstElement() to examples #135
Conversation
} | ||
|
||
let description: DescribeToBlock = { (description: GREYDescription!) -> Void in | ||
guard let description = description else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is description sometimes nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be empty but shouldn't be nil. You're supposed to append matcher's description to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting exceptions when running description.appendText("first match")
without the guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found the bug:
EarlGrey/EarlGrey/Matcher/GREYAnyOf.m
Line 37 in 0481d29
return [self matches:item describingMismatchTo:nil]; |
We should fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I agree! Opened #136
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should close this or find another way to provide it or so until we fix that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to merge since this is what people have to do right now? Then update once the bug is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this as #137 is in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bootstraponline, should the guard be removed now that your fix is in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bootstraponline, should the guard be removed now that your fix is in?
yes, updated.
LGTM |
When an application has exact duplicate elements (every attribute is the same, | ||
including their hierarchy), then the correct fix is to update the app to avoid | ||
duplicating the UI. When that's not possible, a workaround is to match on | ||
the first element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the first element, couldn't we generalize this to match on Nth element?
I believe that can be baked in to the API :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's a great idea. I received similar feedback internally.
@bootstraponline - I feel that this would serve better as a first party API. Thoughts? |
agreed |
Closing in favor of implementing Nth element matcher in EarlGrey core. |
#132