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

IterableSubject.containsMatch(Predicate) #206

Closed
ismell opened this issue Oct 2, 2015 · 15 comments
Closed

IterableSubject.containsMatch(Predicate) #206

ismell opened this issue Oct 2, 2015 · 15 comments
Assignees
Labels
type=addition A new feature

Comments

@ismell
Copy link

ismell commented Oct 2, 2015

It would be nice to have

public final void contains(Predicate<?> predicate) {...}

I could then do something like assertThat(myList).contains(p -> p.getId() == 55);

@kluever kluever self-assigned this Oct 2, 2015
@kluever
Copy link
Member

kluever commented Oct 2, 2015

Since we're still not using Java8 lang features inside of Google, we haven't really investigated anything like this yet, but it sounds pretty reasonable. Thanks for the suggestion!

@ismell
Copy link
Author

ismell commented Oct 2, 2015

I just did the lambda cuz it made it easier to type. This is what I had in mind for java7

assertThat(myList).contains(new Predicate<Blah>() {
    public boolean apply(Blah blah) { ... }
});

@kevinb9n
Copy link
Contributor

kevinb9n commented Oct 2, 2015

Right, but it'd be sad to add new APIs at this point in time that require
Guava's (legacy) functional types, then soon have to try to change them
somehow.

On Fri, Oct 2, 2015 at 1:11 PM, Raul E Rangel notifications@github.com
wrote:

I just did the lambda cuz it made it easier to type. This is what I had in
mind for java7

assertThat(myList).contains(new Predicate() {
public boolean apply(Blah blah) { ... }
});


Reply to this email directly or view it on GitHub
#206 (comment).

Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com

@cpovirk cpovirk changed the title Add Predicate filter to IterableSubject IterableSubject.containsMatch(Predicate) Oct 5, 2015
@cpovirk
Copy link
Member

cpovirk commented Oct 5, 2015

Summarizing some internal discussion:

  • This misses out on some of Truth's biggest feature, its failure messages. Still, "Not true that [a, b, c] contains a match for given predicate" is way better than "Not true that the subject is true", which is what you get if you have to resort to assertThat(...).isTrue().
  • An additional possibility is to require or at least optionally accept a description string along with the predicate: containsMatch("person with ID 55", p -> p.getId() == 55)
  • Internally, we have had discussions of Equivalence support. Probably this would mean assertThat(myList).usingElementEquivalence(idEquivalence()).contains(personWithId55). The Equivalence approach can have the same failure-message problem, but the hope is that that an Equivalence would be useful for multiple assertions and thus might be worth defining toString() on. But note also the other downside: You need to create an entire Person object that has ID 55 rather than just saying "55." This is annoying to write and annoying to read.
  • If we were to provide a way to automatically generate subjects for arbitrary classes, we could add support like assertThat(myList).thatSome(person()).hasId(55), where person() is a method that returns a generated PersonSubject.

@ham1
Copy link

ham1 commented Jun 12, 2016

Any update on this?

We are currently having to resort to:

assertThat(getListFromStream()).contains(expectedItem);

or if creating a list is expensive (or just very long)

assertThat(getStream().anyMatch(expectedItem::equals)).isTrue();

Whereas

assertThat(getStream()).contains(expectedItem);

Would be more readable and could provide better error messages.

Thanks.

@kluever
Copy link
Member

kluever commented Jun 14, 2016

We have an internal proposal for a StreamSubject, but that won't get release for a little while (since that requires the Java 8 extension).
/cc @cgruber

@cgruber
Copy link
Contributor

cgruber commented Jun 14, 2016

We should be releasing a java8 extension (to keep the core deps usable by
pre-8 things like Android) "real soon now". Real soon meaning I'm working
up alterations to a pending change that adds an entry point and
OptionalSubjext for j.u.Optional. shortly after that StreamSubjexct should
be in place. I expect all this in the next couple of weeks.

On Mon, Jun 13, 2016, 18:08 Kurt Alfred Kluever notifications@github.com
wrote:

We have an internal proposal for a StreamSubject, but that won't get
release for a little while (since that requires the Java 8 extension).
/cc @cgruber https://github.com/cgruber


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#206 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAUN4jHu_E6KuL1_3-MhKaLTExNfR-Fcks5qLf8VgaJpZM4GII6p
.

@ham1
Copy link

ham1 commented Jun 14, 2016

Wonderful. Look forward to testing it out.

@kevinb9n
Copy link
Contributor

kevinb9n commented Jan 12, 2017

A couple notes:

  1. If any users are currently using assertThat(myIterable.stream().allMatch(What::ever)).isTrue(), then at least the error messages of this would be better than that.

  2. Perhaps a Consumer-accepting method is better?

  assertThat(collectionOrStream).onEach(
      element -> {
        assertThat(element)....
      });

... storing up failures and reporting the first N all together...?

@ismell
Copy link
Author

ismell commented Jan 12, 2017

Honestly maybe we don't even need this with java8:

assertThat(myList.stream().map(Person::getId)).contains(55);

I think that reads just fine. Or if you wanted to print out the original list we could add a map method:

assertThat(myList).map(Person::getId).contains(55);

This will allow truth to print out myList, the value of the getId, and the value it was expecting.

Or using the existing API we can do this:

assertWithMessage("%s", myList).that(myList.stream().map(Person::getId)).contains(55);

Should I just close this out? Not sure we really need to do anything here.

@kluever
Copy link
Member

kluever commented Jan 12, 2017

Thanks Raul. As you point out, example #1 won't print the original list. Example #2 won't be able to print any details about what the "map transform" is. Example #3 will include all of the information we want, but it's still not best.

I think ultimately we'll want to add some APIs that take the functional types with a description string. Something like:
assertThat(myList).map(Person::getId, "get id").contains(55);

@cpovirk
Copy link
Member

cpovirk commented Sep 27, 2017

Our main answer to this nowadays is http://google.github.io/truth/fuzzy. It's still not quite as straightforward as we might someday make it, though.

@vorburger
Copy link
Member

Would it be really that crazy to have support in extensions/java8 which would let one do things like:

assertThat(things).containsExactly(thing -> thing.name().startsWith("abc"));

which, to me, is both much more obvious to read as well as write than I guess:

assertThat(things.stream().filter(thing -> thing.name().startsWith("abc")).isNotEmpty();

Or am I just missing re. doing this with better streams, to make it as obvious? I also don't really get how to do this with Fuzzy.. nobody wants to have to write a Correspondence just for this? 😃

This seems easy to do (an Iterable8Subject and add to Truth8 ?) - contribution welcome?

@PeteGillin
Copy link

So, yeah, to do this with Fuzzy Truth today, you'd have to subclass Correspondence<Thing, String> and implement compare() as return actual.name().startsWith(expected). If nameStartsWith() returns that implementation then you could write

assertThat(things).comparingElementsUsing(nameStartsWith()).containsExactly("abc");

We agree that having to subclass Correspondence is a pain, and we're currently designing APIs that will allow you to construct instances using factory methods that take functional interfaces as arguments. For example, you might be able to write something like this:

  static Correspondence<Thing, String> nameStartsWith() {
    return Correpsondence.of(String::startsWith).onResultOf(Thing::name);
  }

@cpovirk
Copy link
Member

cpovirk commented May 14, 2019

I think that Fuzzy Truth is going to be our only supported approach for this. We did add some more convenient static factories for Correspondence, so the code looks more like Pete's hypothetical example above nowadays.

An approach like @vorburger lays out is a possibility, but the failure messages are likely to be odd, since the messages for assertions like startsWith are written with a single actual value in mind. Plus, the failure path of the assertion might run many times, and it's not very fast. (I think @PeteGillin has written more about this internally.)

For users who do want an approach like that or an approach like containsMatch, it should be fairly straightforward to implement in a custom Subject -- or even just as an assertContainsMatch static method, if that's easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type=addition A new feature
Projects
None yet
Development

No branches or pull requests

8 participants