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

Accept repeated views #38

Merged
merged 8 commits into from
Apr 4, 2017
Merged

Accept repeated views #38

merged 8 commits into from
Apr 4, 2017

Conversation

Sloy
Copy link
Member

@Sloy Sloy commented Mar 29, 2017

This is a solution proposal for #37

The trick is to combine firstViewOf() with allOf(), and include the verification as a condition.
Yeah, that crazy. But it should work.

@Sloy Sloy requested a review from rocboronat March 29, 2017 19:14
@Sloy
Copy link
Member Author

Sloy commented Mar 29, 2017

What do you think @rocboronat @SmasSive?
Is it too much hacky?

I think the only cons are that the error message in case of exception would be different, and that the Barista code gets more complex.

If you guys like it maybe we should apply this technique to other assertions as well.

@rocboronat
Copy link
Member

Wep @Sloy! Why not just catch the "MultipleMatchesException" and return green?

@Sloy
Copy link
Member Author

Sloy commented Mar 30, 2017

There could be multiple matches for withText but be hidden

@SmasSive
Copy link
Member

I like your idea, could be a little tricky but I like more than Roc's because catching an exception to do something expected... Usually it's not a good idea because you know, an exception it's something unexpected.
I don't have experience with espresso but for me, if the text is not visible it's like it's not there so if the first is not visible and the second one is, test should go green.
Does it help you? I hope so!

@rocboronat
Copy link
Member

@SmasSive all Barista is a try/catch library XD Actually, it manages this kind of Espresso things so, if we need to do bad things... we do them :·D

But, in this case, @Sloy is totally right: My idea was really bad :·D @Sloy , great solution! I can't figure anything better :·)

android:id="@+id/repeated_view_1"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:visibility="gone"
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding a test that check that this TextView is invisible and the next ones visible? Just to check that the context that we're testing is the good one. If not, someone could remove this TextView, break your feature, and the tests will give a green instead of a red.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea. But I think it should be part of the "Given" of the new test, instead of a new test. As you said, it's to verify the context of this test, not something independent. Agree?

Copy link
Member

Choose a reason for hiding this comment

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

Totally... the thing is that I don't know if it's common to check the context in the "given" part instead of setting it. But... let's give it a try :·)

Copy link
Member

Choose a reason for hiding this comment

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

Wep @Sloy how's the approach to this suggestion? It's something that we can do? Or it's too hard for this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you're right. I thought I already did it! Let me add it real quick.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! :·)

Copy link
Member Author

Choose a reason for hiding this comment

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

@rocboronat how about now?

@rocboronat
Copy link
Member

By the way, to save you a click to Travis:

/home/travis/build/SchibstedSpain/Barista/library/src/main/java/com/schibsted/spain/barista/BaristaAssertions.java:39: Useless parentheses.

@rocboronat
Copy link
Member

Going to do the simplest job ever seen... :D

@rocboronat
Copy link
Member

Mmmmm @Sloy , what do you think about doing the same at assertDisplayed(int id)?

@Sloy
Copy link
Member Author

Sloy commented Mar 30, 2017

@rocboronat yes, it's part of my first comment :P

If you guys like it maybe we should apply this technique to other assertions as well.

@rocboronat
Copy link
Member

Op, your comment was wider enough to understand that you're talking about lots of other assertions, not the other assertDisplayed(). Please, be concise... we're techs 😝

@rocboronat rocboronat changed the title [WIP] Accept repeated views Accept repeated views Mar 30, 2017
@Sloy
Copy link
Member Author

Sloy commented Apr 3, 2017

@rocboronat ok let's just go with displayed for now.

Sloy and others added 7 commits April 3, 2017 20:21
By including the isDisplayed condition as a requirement for onView, we filter any views that matches the text but are not displayed.
A bit tricky though.
Because the usage is different from allOf. It's not a matcher filter, but a view filter.
@Sloy Sloy force-pushed the feature/repeated_views branch from 7c3b60d to 3174754 Compare April 3, 2017 18:23
@Sloy Sloy added review-please and removed wip labels Apr 3, 2017
@Sloy
Copy link
Member Author

Sloy commented Apr 3, 2017

Rebased with master! Definitive review?

@Sloy Sloy closed this Apr 3, 2017
@Sloy Sloy reopened this Apr 3, 2017
@@ -50,6 +50,14 @@ public void checkVisibleViews_breaksWhenNeeded() {
}

@Test
public void checkVisible_withRepeatedViews() throws Exception {
assertNotDisplayed(R.id.repeated_view_1_gone);
Copy link
Member

Choose a reason for hiding this comment

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

Great way to check it! And so easy, I guess! :·D

@rocboronat
Copy link
Member

@Sloy great! Feel free to merge it! :·)

@Sloy Sloy removed the review-please label Apr 4, 2017
@Sloy Sloy merged commit 47f0b32 into master Apr 4, 2017
@Sloy Sloy deleted the feature/repeated_views branch April 4, 2017 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants