Assertj-core: hasOnlyOneElementSatisfying treats all elements as satisfied if only one is

[andyRokit]

Summary

When a single element in hasOnlyOneElementSatisfying passes the assertions, all elements count as passing.

Example

@Test
    public void succeeds_if_iterable_has_only_one_element_and_that_element_statisfies_the_given_assertions() {
        assertThat(asList(new Jedi("Yoda", "red"), new Jedi("Luke", "green"))).hasOnlyOneElementSatisfying(yoda -> {
            assertThat(yoda.getName()).isEqualTo("Yoda");
            assertThat(yoda.lightSaberColor).isEqualTo("red");
        });
    }

Expected

Test passes

Actual

java.lang.AssertionError: 
Expected size:<1> but was:<2> in:
<[org.assertj.core.api.iterable.YodaTest$Jedi@7823a2f9,
    org.assertj.core.api.iterable.YodaTest$Jedi@4cc0edeb]>

[joel-costigliola]

The naming is confusing but the behavior is correct, it is expected that you have only one element and that element must satisfies the requirements - see the examples in the javadoc.

Can you think of a better name for this assertion? One that would not be ambiguous ?

[andyRokit]

If you required such a method it would probably be called: hasOnlyOneElementAndSatisfies

However, do we really need a method that does that? It's effectively two checks in one:

assertThat(asList(new Jedi("Yoda", "red")))
       .hasSize(1)
       .allSatisfy(yoda -> {
            assertThat(yoda.getName()).isEqualTo("Yoda");
            assertThat(yoda.lightSaberColor).isEqualTo("red");
        });

If the hasOnlyOneElementSatisfying method checked that a only a single element satisfies the condition (as the name suggests) it would be very useful. I can also think of more use cases. Would it make sense to have a method: jedi.numberOfElementsSatisfying(...)?

E.g.

assertThat(jedi).numberOfElementsSatisfying(yoda -> {
            assertThat(yoda.getName()).isEqualTo("Yoda");
            assertThat(yoda.lightSaberColor).isEqualTo("red");
        })
       .isEqualTo(0);  //hasNoElementsSatisfying
       //.isEqualTo(1);  //hasOnlyOneElementSatisfying
       //.isGreaterThan(1);  //hasAtLeastOneElementSatisfying
       //.isEqualTo(jedi.size());  //allSatisfying

If you agree with any of this I'd be happy to attempt a pull request.

Edit
Also consider changing the subject of the tests. Star Wars is great, but given the fact that the plural of Jedi is Jedi....

[joel-costigliola]

Ok what we could do is:

• rename hasOnlyOneElementSatisfying to isSingletonWhoseElementSatisfies
• add hasExactlyOneElementSatisfying with the behavior you were expecting

Why not keeping hasOnlyOneElementSatisfying ? Well some users will still think of it with the current behavior so I think it is better to break their code so that they have to explicitely choose between isSingletonWhoseElementSatisfies and hasExactlyOneElementSatisfying.

A few options that are available at the moment:

• numberOfElementsSatisfying can be replaced by filteredOn(Predicate) + hasSize()
• haveExactly(1, condition)

Edit
I'm going to call George Lucas so that we can use Jedis, he is a nice guy, he'll understand ;o)
If not, let's go with GoT examples!

[andyRokit]

I agree isSingletonWhoseElementSatisfies is a better description of the functionality than hasOnlyOneElementSatisfying. Given that isSingletonWhoseElementSatisfies is in use, it makes sense to deprecate it and remove it in a few releases time.

You raise the question of keeping the function under a different name. Currently this would be achieved by:

    @Test
    public void usingHaveExactly() {
        Condition<Jedi> isYoda = new Condition<>(
                jedi -> jedi.getName().equals("Yoda") && jedi.lightSaberColor.equals("red"),
                "Is Yoda");

        assertThat(jedis).hasSize(1).haveExactly(1, isYoda);
    }

I actually think that this is a better check in that it will give you feedback on which condition failed. Was there not a single element in the array? Expected size:<1> but was:<2> in ...
Was there a single item, but it didn't match? Expecting elements: ... to have exactly 1 times <Is Yoda>

However it's not as pretty! The Consumer argument (which hasOnlyOneElementSatisfying uses) is far more readable than the Condition argument haveExactly uses.

I know it's a big change, but I would be in favor of deprecating both hasOnlyOneElementSatisfying and haveExactly in favor of either: numberOfElementsSatisfying or (hasNoElementsSatisfying, hasOnlyOneElementSatisfying, ...). Mentioned above

I don't want to be pushy as this is my first contribution to this library, so perhaps just consider this as a suggestion for an enhancement rather than a bug.

[joel-costigliola]

@andyRokit what about adding filteredOn(Consumer<? super ELEMENT> requirements) that would only keep elements verify the given assertions ?

Example:

assertThat(list(new Jedi("Yoda", "red"), new Jedi("Luke", "green"))).filteredOn(jedi -> {
            assertThat(jedi.getName()).isEqualTo("Yoda");
            assertThat(jedi.lightSaberColor).isEqualTo("red");
        }).hasSize(1);

[andyRokit]

@joel-costigliola - sorry for the delayed response. I've been on vacation with limited internet access. Yeah, I think that is a neat solution. Here's my question: Does what you've suggested replace the role of the Condition class entirely?

I like the filteredOn approach better as it sticks with framework assertions (assertThat) rather than reverting to standard Java operations. In the interests of the framework, would it make sense to look at what would be involved an deprecating Condition as part of the framework?

_Note: I'm totally out of my depth of this point! I'm happy to act as a sounding board, but if you do consider going down this route, would it make sense for you to open a new issue/discussion and take input for some of the more experienced framework contributors?_

[joel-costigliola]

@andyRokit no problem for the late answer, really.

I'm comfortable discussing any topic and I'm happy you felt you could too!

My suggestion is an alternative to basic Condition usage, it does not quite replace them entirely, you can combine Conditions to create another Condition (see http://joel-costigliola.github.io/assertj/assertj-core-conditions.html). Conditions are an extension mechanism that let you express the domain you are working on (e.g. isJedi if you are working on a start wars software).

Condition already existed in Fest Assert which is the project AssertJ comes from, there is no plan to remove them at the moment. Having said that I always felt Condition to be a poor man Hamcrest matchers, I'm not too fond of them (nor Hamcrest). If we were to do another major version to clean up the (pretty bloated) AssertJ API, I'm not sure I would keep them but we are not there yet.

[andyRokit]

I guess in a time before Lambda expressions Condition may have been the right tool for the job. I don't think they are any more. I like where you're going with filteredOn but I think it would be a mistake for the outcome of this ticket to be to add a new method to the AssertJ.

I just went through a list of the assertions available to ListAssert and their arguments are a mix-and-match of Condition, Consumer, object instances and classes.

Given the power of Lambda expressions would it make sense to think of everything primarily as Lambda arguments and then add the other methods back in as shortcuts?

E.g. containsSequence checks for a sequence containing exact objects. But could a more generic way of thinking about it be to have a containsSequence method which has Lambdas in place of the object to compare (of which checking that they are equal is only one option).

I.e.
ListAssert<ELEMENT> containsSequence(Consumer<? super ELEMENT>... sequence)

While thinking of
ListAssert<ELEMENT> containsSequence(ELEMENT... sequence) more as an overload to this functionality?

I.e. You could think of Lambdas as the foundation of AssertJ assertions, make sure you cover 100% of the functionality you want in AssertJ, then add further overloads as "shortcut methods" where you think it would be useful.

[joel-costigliola]

We would only do so in a new major version of AssertJ where we can break the API as much as we want.

For the current line of work (3.x) we will keep everything to avoid breaking the API.

Following the containsSequence example, assertions taking lambdas are not as straightforward to use as their counterpart with objects from an IDE point of view, I don't see assertions with objects being a second class citizen.

The outcome of this ticket is to add filteredOn(Consumer<? super ELEMENT> requirements) as it addresses (I think) your original issue.

I'm happy to further discuss the lambdas approach you are suggesting (if you still have comments) but in another issue to avoid mixing everything together.

[andyRokit]

Yes - of course. The above discussion (especially the last few comments) would be no small change and would warrant incrementing the major version.

If you're comfortable with adding filteredOn at this point I won't argue. I was just feeling a bit guilty about causing clutter in the framework! Maybe the best thing would be to add it now and to review similar methods in the future as part of a major release.

[andyRokit]

Hi Joel - I took a brief look at creating a pull request for this when I realised that there's yet more ways of doing the same thing!

My new code

public SELF filteredOnAssertions(Consumer<ELEMENT> elementAssertions) {
    List<? extends ELEMENT> filteredIterable =
      stream(actual.spliterator(), true).filter(element -> {
          try {
            elementAssertions.accept(element);
            return true;
          } catch(AssertionError e) {
            return false;
          }
        }).collect(toList());

    return newAbstractIterableAssert(filteredIterable).withAssertionState(myself);;
  }

Note: Had to call it filteredOnAssertions to avoid clash with below signature

Existing (similar) code

public SELF filteredOn(Predicate<? super ELEMENT> predicate) {
    checkArgument(predicate != null, "The filter predicate should not be null");
    List<? extends ELEMENT> filteredIterable = stream(actual.spliterator(), false).filter(predicate).collect(toList());
    return newAbstractIterableAssert(filteredIterable).withAssertionState(myself);
  }

So we've got Condition, Predicate and Consumer all doing essentially the same thing here. I'm struggling with my conscience here! You're sure you want this? If so I'm happy to submit a PR.