Introduce generic assertAnyView function #198

Sloy · 2017-12-18T08:10:11Z

TL;DR:

This method allows asserting that any view on the screen matches a certain condition, without getting the infamous Espresso error AmbiguousViewMatcherException.

Rationale

We say we do magic for things like views below a scroll or duplicated views on screen. But actually that only work for some Assertions and Interactions, not all of them implement it. E.g.: #107 #46

Solution

In this PR I propose to extract the assertion code to one place, and reuse it everywhere it's needed. (The same should be done for Interactions, see #196).
I created a function to be used like this:

assertAnyView(withId(R.id.button), isDisplayed())

We don't usually expose kotlin APIs, but since this method is intended for internal usage I also added an extension function alias:

withId(R.id.button).assertAny(isDisplayed())

I also created some infix extension functions to make it even easier to write. It extends any Matcher<View>, String or Int (resource). I added a `not` operator to Matcher<View>. I documented the behaviour the best I could. I replaced the in-site code with this function, and applied it to other methods of the BaristaVisibilityAssertions that were not doing any magic.

SmasSive · 2017-12-18T08:21:00Z

Hi, mimimi here.
I don't agree with infix function, maybe is my lack of knowledge of Kotlin or that I didn't use it enough but for me is not natural to use the API like this withId(R.id.button) magicAssert not(isDisplayed()) I think is better to use it semantically like this withId(R.id.button).magicAssert(not(isDisplayed())) but it's ok since we don't force anyone to use it only in a certain way, remember I am just a mimimi.

On the other hand, I don't agree either with the alias for the not operator, users are used to Espresso and they write things like not, so I don't see any reason to convert it to !, I think it only can confuse users about its usage.

Sloy · 2017-12-18T08:17:47Z

library/src/main/java/com/schibsted/spain/barista/internal/MagicAssert.kt

+        onView(viewMatcher)
+                .withFailureHandler(spyFailureHandler)
+                .check(ViewAssertions.matches(condition))
+    } catch (firstError: RuntimeException) {


@SmasSive look! A try/catch in the wild!

I tried implementing this with some kind of Try monad to avoid anidating these try/catch, and it worked. But I'm not sure it's worth that complexity just for one method 😕 (or two in the future).

Here's what I was able to implement with Kategory.

It might be simplified even more with some custom structure instead of this one. Not sure if that's enough though.
Thoughts? 💭

I do love the implementation with Kategory, it simplifies a lot the code. I'd love it even more if you separate the Trys in methods, just to improve readability, something like:

tryToAssertView() .recoverWith { tryToAssertFirstViewOf() } .getOrElse { spy... }

What do you think?

You know I hate you, right? 🤣
Ok let me see what I can do.

Here are a couple of examples I've put together.

With Kategory:

With a custom DSL-ish:

I like how the second one looks. But again, I'm not sure if we're being pragmatic here. Seems like a lot of complexity for something that will be use in just two places.

I like the second option too but the key here I think is to see how it is implemented, if it adds a lot of complexity I'd discard it but if it doesn't, go for it!
BTW, I invoke some of the usual contributors like @rocboronat @alorma @sergiandreplace to light us up here 👯‍♂️

Well... For the implementation I could use a hand.

This is something that works, not too obvious though:

I'm starting to think the initial double try/catch is the best solution 🤣

Ok, looks complex... So my call would go for the first implementation with Kategory.
Please please please, don't kill me ❤️

Sloy · 2017-12-18T08:25:44Z

Not mimimi at all. I did those things "because I could". Now it's the moment to think if they make sense or not.
Yep, I too think the not operator can add confusion.
The infix... I like it for some reason. But it's not rational, just a personal feeling. I don't have real arguments for or against it.

How about step 3?
Would you still do the extension R.id.button.magicAssert(not(isDisplayed())) for ids?

Sloy · 2017-12-18T08:31:17Z

Actually, what I think I like of the infix is that is clearly separates the view finder and the actual condition. In a long line like withId(R.id.button).magicAssert(not(isDisplayed())) with many dots and parenthesis, you have to stop (a bit) and see where those two parts are separated. With the infix is easier to spot.
But anyways, it's true I haven't seen many APIs using it either, don't know what's the good practice around it.

SmasSive · 2017-12-18T08:37:28Z

Step 3 is more like the not, I think we should ask ourselves... Do we want to abstract as much as possible the Espresso API? Or do we want that Barista is just a "enabler" of Espresso? The answer to that question will light our path, if we want to create our own semantic, abstract as much as possible the usage of Espresso, let's go for ! and id's. If we don't, if we want that Barista remains as the sugar of Espresso, we should not add too many changes.

Sloy · 2017-12-18T08:39:19Z

So you go for the more espresso-like syntax.

rocboronat · 2017-12-27T09:17:59Z

Let me share an idea: we love magic when interacting with the views, but we must hate magic when asserting. For example, if I assert if a view is displayed or not, if it's not displayed because of a scroll, for me, the test should fail. We want to be happy and friendly with the interactions, but not with the assertions.

By the way, magicAssert is a really magic name...

Sloy · 2018-02-01T10:55:05Z

Hi! After 1.5 months of holidays and work stuff, I'm back to this PR!

I'm going to reset myself and just make the simpler version possible of the discussed changes. There's really no need to experiment with cool Kotlin things here. That way nobody can complain that it's too weird of complex. Muahahaha.

About your comment @rocboronat: this doesn't add any more magic to assertions than we already have. We won't try to scroll or anything when asserting. The assertions magic is much simpler than interactions. In this case it's documented in the method JavaDoc:

* Attempts to assert using multiple scenarios for the [viewMatcher]:
 * 1. Just one view matches the [viewMatcher].
 * 2. Multiple views match the [viewMatcher]: will pass if at least one of them matches the [condition].

Which is basically what we did for #37, #68 or #46.

What this PR does is apply those same conditions to the rest of assertions and keep the code in one place, instead opening a new issue for each assertion that doesn't work as expected and duplicating the code.

The name magicAssert is really just a placeholder because I'm not sure how to call it, and wanted to discuss the implementation first. But suggestions are welcomed, because really I don't know how to call it. We always talk about "Barista does some magic for you", and we even have a Readme section explaining it. But explaining it in a method name it's not that easy...

Sloy · 2018-02-01T11:24:44Z

I only kept one extension function:

withId(R.id.button).magicAssert(not(isDisplayed()))

I think it's the more agreeable one. You guys are ok with that?

Now, another topic: the naming. As @rocboronat says, "magicAssert" is a very magic name, which is usually bad. What's the alternative?

ivngrz · 2018-02-01T13:22:28Z

It would be cool to think of ways to use the Kotlin stuff to make the tests more readable, but I agree we should probably discuss that in another PR.

And yep, it would be better to find a name that actually describes what the method is doing 😆

How about assertAny, or assertAtLeastOnce? Would that cover it?

We could also go for something more generic, that just says this "not as strict as a default Espresso check", like softAssert, or lenientAssert or something along those lines.

Sloy · 2018-03-01T08:22:47Z

I like assertAny. I think that's a pragmatic description of that the method does.
withText("button").assertAny(isDisplayed()).

SmasSive · 2018-03-01T08:27:57Z

Go for assertAny!

Sloy · 2018-03-01T10:27:11Z

❗️❗️❗️ Ok, this is the final proposal❗️❗️❗️

I updated the code, the title and the description.

Any last complains? Otherwise, let's merge it!

Sloy · 2018-03-01T11:33:39Z

Thanks everyone!

Sloy requested a review from rocboronat as a code owner December 18, 2017 08:10

Sloy added 8 commits December 18, 2017 09:12

Rename and improve error message of BaristaArgumentTypeException

316a515

Apply magicAssert to EnabledAssertions

b1b326a

Apply magicAsset on CheckedAssertions

31cbb87

Apply magicAssert to BackgroundAssertions

c9aadce

Apply magicAssert to FocusedAssertions

47e4f77

Apply magicAssert to HintAssertions

5ffefe0

Apply magicAssert to ImageViewAssertions

a26c1fe

Sloy force-pushed the more-magic branch from c89aef4 to a26c1fe Compare December 18, 2017 08:12

Sloy added help wanted review-please labels Dec 18, 2017

Sloy mentioned this pull request Dec 18, 2017

Created ViewActionExecutor to reuse performAction() code #194

Merged

Sloy commented Dec 18, 2017

View reviewed changes

Sloy added the discussion label Dec 19, 2017

Sloy added 2 commits December 19, 2017 16:14

Extract assertion attempts to methods

a45319d

Remove operator for matchers

a27a202

Simplify magicAssert api

fb30147

Sloy mentioned this pull request Mar 1, 2018

Made public: #209

Closed

Rename to assertAny

88e0011

Sloy changed the title ~~Introduce magic assertion~~ Introduce generic assertAny assertion Mar 1, 2018

Sloy removed discussion help wanted labels Mar 1, 2018

Sloy changed the title ~~Introduce generic assertAny assertion~~ Introduce generic assertAnyView function Mar 1, 2018

SmasSive approved these changes Mar 1, 2018

View reviewed changes

ivngrz approved these changes Mar 1, 2018

View reviewed changes

gopito approved these changes Mar 1, 2018

View reviewed changes

Litemn approved these changes Mar 1, 2018

View reviewed changes

Sloy merged commit b720c50 into master Mar 1, 2018

Sloy deleted the more-magic branch March 1, 2018 11:33

Sloy mentioned this pull request Jul 3, 2018

Issue when writing in an EditText that appears in another page of a ViewPager #107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce generic assertAnyView function #198

Introduce generic assertAnyView function #198

Sloy commented Dec 18, 2017 •

edited

SmasSive commented Dec 18, 2017

Sloy Dec 18, 2017

Sloy Dec 18, 2017

SmasSive Dec 18, 2017

Sloy Dec 18, 2017

Sloy Dec 19, 2017

SmasSive Dec 19, 2017

Sloy Dec 19, 2017

Sloy Dec 19, 2017

SmasSive Dec 19, 2017

Sloy commented Dec 18, 2017

Sloy commented Dec 18, 2017

SmasSive commented Dec 18, 2017

Sloy commented Dec 18, 2017

rocboronat commented Dec 27, 2017

Sloy commented Feb 1, 2018

Sloy commented Feb 1, 2018

ivngrz commented Feb 1, 2018

Sloy commented Mar 1, 2018

SmasSive commented Mar 1, 2018

Sloy commented Mar 1, 2018

Sloy commented Mar 1, 2018

Introduce generic assertAnyView function #198

Introduce generic assertAnyView function #198

Conversation

Sloy commented Dec 18, 2017 • edited

TL;DR:

Rationale

Solution

SmasSive commented Dec 18, 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

Sloy commented Dec 18, 2017

Sloy commented Dec 18, 2017

SmasSive commented Dec 18, 2017

Sloy commented Dec 18, 2017

rocboronat commented Dec 27, 2017

Sloy commented Feb 1, 2018

Sloy commented Feb 1, 2018

ivngrz commented Feb 1, 2018

Sloy commented Mar 1, 2018

SmasSive commented Mar 1, 2018

Sloy commented Mar 1, 2018

Sloy commented Mar 1, 2018

Sloy commented Dec 18, 2017 •

edited