assertj / assertj

AssertJ is a library providing easy to use rich typed assertions
https://assertj.github.io/doc/
Apache License 2.0
2.63k stars 700 forks source link

Consider adding `reduce(S initial, BiFunction<S state, T element> reducer)` method to all `AbstractArrayAssert` instances #2920

Open mazurkin opened 1 year ago

mazurkin commented 1 year ago

Feature summary

So let's assume we have a huge array (for example of float numbers). It is too verbose to check all items there, but pretty easy to check a hash code or sum of all items.

So I'd like to do something like:

float[] values = ...

// check minimum (or maximum)
assertThat(values)
    .reduce(Float.MAX_VALUE, Math::min)
    .isCloseTo(...)

// check sum
assertThat(values)
    .reduce(0f, Float::sum)
    .isCloseTo(...)    

// check hash code
assertThat(values)
    .reduce(0f, (s, n) -> 37.0f * s + n)
    .isCloseTo(...)     

Or it could be just a business requirement:


float[] values = ... 

assertThat(values)
    .reduce(0, Float::sum)
    .describedAs("Total sum of all factors must be 1.0")
    .isEqualTo(1.0f);

``
mazurkin commented 1 year ago

A mixed type scenario: array of string, and state is HashSet<String> (to collect unique string):

String[] values = ...

assertThat(values)
    .reduce(new HashSet<String>(), (s, i) -> {
        s.add(i);
        return s;   
    })
    .containsExactly("TRUE", "FALSE")
vlsi commented 1 year ago

Reduce functions can't be described automatically, so adding reduces into assertj does not change much. On contrary, it would make the API harder to use and maintain, and it would provide no value 🤷

At the same time, .extracting(Function) already exists: https://github.com/assertj/assertj/blob/main/assertj-core/src/main/java/org/assertj/core/api/AbstractObjectAssert.java#L961

joel-costigliola commented 1 year ago

Thanks, we will look into this !

mazurkin commented 1 year ago

At the same time, .extracting(Function) already exists: https://github.com/assertj/assertj/blob/main/assertj-core/src/main/java/org/assertj/core/api/AbstractObjectAssert.java#L961

Not in AbstractArrayAssert

mazurkin commented 1 year ago

To my personal taste I would prefer:

double[] result = ...

assertThat(result)
    .isNotNull()
    .reduce(Double.MIN_VALUE, Double::max)
    .isCloseTo(1.000, Offset.offset(0.001));

over this:

double[] result = ...

assertThat(result)
   .isNotNull();

assertThat(Doubles.max(result))
   .isCloseTo(1.000, Offset.offset(0.001));

Unfortunately there is no .extracting(Function) for arrays in AssertJ. Even if we have it sometimes .reduce() is more handy, for example there is no sum(float[] values) in any of common libraries (Guava, Apache Common, Java util/lang), but there is Float::sum available in Java lang.

Reduce is a standard and widely-used concept in functional programming.

vlsi commented 1 year ago

Reduce is a standard and widely-used concept in functional programming.

If you use that as an excuse, then you should consider adding all the rest: map, flatMap, fold, lfold, and so on. Why adding reduce only?

for example there is no sum(float[] values) in any of common libraries

Anyone can write such a function, especially if you operate with float which is quite a niche. At the same time, Kotlin does have standard functions to sum arrays, including FloatArray: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/sum.html

For double[] there's DoubleStream.of(...).sum().


    .reduce(Double.MIN_VALUE, Double::max)
...
    .reduce(Float.MAX_VALUE, Math::min)
...
    .reduce(new HashSet<String>(), (s, i) -> {
        s.add(i);
        return s;   
    })

I believe what you suggest are the anti-patterns: they make the error message obscure, and assertj won't really help there, because assertj won't see beyond the functions.

On contrary, a targeted assertions would be way better. For instance, instead of reduce(HashSet one can already use

String[] values = ...

assertThat(values)
    .containsOnly("TRUE", "FALSE")

The code would be easier to follow, and assertj can prepare a much better message than the one when user calls .reduce(new HashSet...

scordio commented 1 year ago

IMHO we need to carefully evaluate if AssertJ should replicate the functional programming features already offered by the JDK via the Stream API.

extracting(Function) is kind of already belonging to this topic.

mazurkin commented 1 year ago

extracting(Function) in AbstractArrayAssert will solve 95% of my cases.

and the additional map() in AbstractArrayAssert which convert the items of the array/list (for example converts float[] to double[] or double[][] to double[] by aggregating rows) will solve the other 5%.

probably extracting() + map() with arrays/lists will be even better, probably reduce looks too scary/specific

vlsi commented 1 year ago

The issue with extracting is it provides no information on what is extracted.

I believe, extracting should have a mandatory parameter that dercribes the extraction function

scordio commented 1 year ago

probably extracting() + map() with arrays/lists will be even better

@mazurkin aside from naming, aren't extracting(Function) and map(Function) the same?

Iterable assertions already have map(Function) and extracting(Function), one being the alias of the other.

Object[] assertions already have extracting(Function).

We could argue that all the subclasses of AbstractArrayAssert should be at least at the same functional level as AbstractObjectArrayAssert.

vlsi commented 1 year ago

There are slightly different cases:

I do not mean all of them are needed in AssertJ.

joel-costigliola commented 1 year ago

A bit of history, extracting was introduced a long time ago (java 6 time!) and the main use case was to extract some properties from the elements under test, ex the id property of a list of persistent objects.

Then came java 8 and functional programming, we added support for lambdas and map(lambda) as an alias of extracting(lambda). In my view extracting is kinda a specialized case of map where you dig in the elements under test the values to ... extract.

mazurkin commented 1 year ago

first let me thank you for you job, AssertJ is great, I am using AssertJ instead of hamcrest.assertThat for the new project which contains a lot of math and so a lot of numerical arrays.

aside from naming, aren't extracting(Function) and map(Function) the same?

@scordio please check, @vlsi already explained: extracting(Function) converts the whole array into some other type, map converts T[] into S[] (I suspect there are might be type issues with such a conversion).

My main problems for me right now:

Of course none of these are blockers, I can make one other extra check, I just want to combine everything into one single chained sequence:

assertThat(array)
   .isNotNull()
   .containsExactly(0.36, 0.42, 0.22)
   .extracting(StatUtils::sum) <-- here
   .isCloseTo(1.000, Offset.offset(0.000001)); 

// or   

assertThat(array)
   .isNotNull()
   .containsExactly(0.36, 0.42, 0.22)
   .reduce(0.0, Double::sum) <-- here
   .isCloseTo(1.000, Offset.offset(0.000001)); 

// instead of 

assertThat(array)
   .isNotNull()
   .containsExactly(0.36, 0.42, 0.22);

assertThat(StatUtils.sum(array))
   .isCloseTo(1.000, Offset.offset(0.000001));   

You already provide extraction(function) but for some reason it is not available for arrays.

scordio commented 1 year ago

Thank you for the kind words, @mazurkin 🙂

@scordio please check, @vlsi already explained: extracting(Function) converts the whole array into some other type, map converts T[] into S[] (I suspect there are might be type issues with such a conversion).

This is not how extracting(Function) works for Iterable or Object[] assertions. I suggest to double-check its Javadoc and/or implementation.

Out of this conversation, I see at least two obvious takeaways:

These however do not address the original need of @mazurkin where a reduce API would seem more appropriate. I see adding that API as a bigger change that would require more careful design (https://github.com/assertj/assertj/issues/2920#issuecomment-1382799163).