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 missing fields in assertion error (#639) #640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artyom-razinov
Copy link

@artyom-razinov artyom-razinov commented Oct 7, 2017

There are more fields in error in logs of failure handler than in inout error parameter.

Previous code replaces fields (so some are lost) with only kErrorDetailActionNameKey and kErrorDetailElementMatcherKey, new code only modifies kErrorDetailElementMatcherKey.

See issue: #639

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@artyom-razinov artyom-razinov force-pushed the issue-639-fix-amount-of-info-in-action-error branch from 2415de9 to e2de78e Compare October 7, 2017 15:09
@artyom-razinov artyom-razinov reopened this Oct 7, 2017
@artyom-razinov artyom-razinov force-pushed the issue-639-fix-amount-of-info-in-action-error branch from e2de78e to e08007b Compare October 7, 2017 15:13
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 7, 2017
@tirodkar
Copy link
Collaborator

tirodkar commented Oct 8, 2017

This looks great. It would be great if you could add this to grey_handleFailureOfAssertion as well. Thanks a lot!

@artyom-razinov
Copy link
Author

Ok! 👌

@khandpur
Copy link
Collaborator

Hey @artyom-razinov, just checking if you still plan on working on this in future.

@tirodkar
Copy link
Collaborator

@artyom-razinov we could make any changes and add them to the cl as well to preserve your commit, you'd need to allow contributions to it from your side though.

@artyom-razinov artyom-razinov force-pushed the issue-639-fix-amount-of-info-in-action-error branch from e08007b to 63e1abc Compare October 18, 2017 19:43
@artyom-razinov
Copy link
Author

@tirodkar

It would be great if you could add this to grey_handleFailureOfAssertion as well.

I've just added it. Sorry for the delay, in my company we planned to gradually get rid of our fork of EarlGrey, but it is not a high priority task.

We also have support of Russian keyboard, however the code is not ready to be pushed to EarlGrey, because it is not scalable for every other keyboard. Is there any progress on supporting different keyboard layouts already? (#31)

If not I'll make a PR maybe this month.

@tirodkar
Copy link
Collaborator

A PR for that would be great. You can use grey_replaceText() to type in different languages till then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants