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

Improve the explanation about custom assertions #255

Merged
merged 4 commits into from
Oct 5, 2018

Conversation

rocboronat
Copy link
Member

Hello from South Korea! 🌏

#254 is a great PR, but I needed to read its code twice to understand what it was offering. The readme didn't explain the whole power of the new feature.

This PR will try to better explain it. Feel free to improve it as much as possible. I tried to focus on newbies instead of Espresso experts because that's what I am 🤗

By the way, if I misunderstood the API that was offered, feel free to shame on me 💩and commit on this PR without previous notice.

Copy link
Member

@alorma alorma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool addition @rocboronat!

Maybe we should add the examples @Sloy made
on: https://twitter.com/sloydev/status/1046741921979387905

What do you think?

README.md Outdated

Do your own asserts on given view, no more custom *Matcher*
If you have a special case not covered by the given assertions API, we encourage you to assert these special cases with our custom assertions API. It's a convenient way to replace plain `Matcher`s with complex assertions. With Barista, you can match any kind of view by knowing its type and passing its `viewId`, `text`, or a common `Matcher<View>`. Once you matched it, you will be able to assert all its properties without adding any complex `Matcher` to your project.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common or custom Matcher?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmmm you can also use a common withId or those given by Espresso... but you are right. I think I could just avoid the word and it will work better 💃

@rocboronat
Copy link
Member Author

@alorma totally! Tomorrow I'll update the PR

@rocboronat
Copy link
Member Author

@alorma done! I copied them and I think I improved them a bit. Now the examples show all the power of the feature. Let's check again! There's always space for the improvement 🚀🍌

@rocboronat
Copy link
Member Author

After some time, I think it's good enough. At least for a while. We can always open a new PR to improve it even more. Thanks for the reviews!

@rocboronat rocboronat merged commit c856e9c into master Oct 5, 2018
@rocboronat rocboronat deleted the readme-custom-assertions branch October 5, 2018 06:15
@Sloy
Copy link
Member

Sloy commented Oct 5, 2018

Hi! I'm off this week, but the error message is wrong :)
It's not an error message, it's the "expected" message. Because when the matcher fails Espresso will print "condition didn't match ".
It's not very intuitive, but it's how matchers work. They're called descriptions, not error messages.

@rocboronat
Copy link
Member Author

@Sloy totally agree, but we are hiding the complexity and counterintuitive things of the Matchers with our approach, aren't we?
If we aren't, what do you think about changing assertion error message by assertion description message in the README?

@Sloy
Copy link
Member

Sloy commented Oct 9, 2018

@rocboronat no, in this case we aren't. Because when assertion like this fails assertAny(R.id.button, "The button was not enabled") { .... } it will print an error like didn't match condition (The button was not enabled). And we know a double negation is an affirmation, so it doesn't make sense 😅

We don't have control of the message outside the parentheses. That's just how Espresso works (well, it's Hamcrest actually). It will append parts of matchers and assertions to create a full message. I don't think we should go against it.

If we aren't, what do you think about changing assertion error message by assertion description message in the README?

Yes, that's how it should be called in my opinion ^.^
Also, since we're using Kotlin there we could encourage people to use named parameters in the readme to avoid confusion, like assertAny(R.id.button, description = "is enabled") { ... }. It's common to use them with optional parameters, and add that little bit of extra context.

@rocboronat
Copy link
Member Author

@Sloy thanks for the clarification! The thing is that description is broad enough that I didn't understand its meaning. I thought you were talking about the description of the View, not the assertion.

By the way, going to fix the comment inlined in the code.

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

Successfully merging this pull request may close these issues.

3 participants