assertj icon indicating copy to clipboard operation
assertj copied to clipboard

Using 'extracting' with InstanceOfAssertFactory will lead to an unexpected AssertionError if the extracted value is null

Open kamil-sita opened this issue 2 years ago • 8 comments

Describe the bug AssertJ allows extracting values from the original object using "extracting" method in order to test them in a more fluent way. AssertJ also allows using InstanceOfAssertFactory in "extracting" to specify which assertions should be used to test the extracted value.

However, there's an unexpected non-null check when using InstanceOfAssertFactory, which leads to an exception being thrown if the extracted value is null. This is not the case when not using InstanceOfAssertFactory, see the example below:

import org.assertj.core.api.InstanceOfAssertFactories;
import org.testng.annotations.Test;

import static org.assertj.core.api.Assertions.assertThat;

public class Reproduced {

    @Test
    public void should_testNullCorrectly() {
        // given
        StringHolder holder = new StringHolder(null);

        // then
        assertThat(holder)
            .extracting(StringHolder::value)
            .isNull(); // this test will succeed
    }

    @Test
    public void should_testNullCorrectlyWithAssertFactory() {
        // given
        StringHolder holder = new StringHolder(null);

        // then
        assertThat(holder)
            .extracting(StringHolder::value, InstanceOfAssertFactories.STRING)
            .isNull(); // this test will fail
    }

    public record StringHolder(String value) { }
}
  • assertj core version: tested with 3.16.1 and 3.24.2
  • java version: 17.0.6
  • test framework version: tested with TestNG 6.9.10 and JUnit 5.9.2

Test case reproducing the bug


import org.assertj.core.api.InstanceOfAssertFactories;
import org.testng.annotations.Test;

import static org.assertj.core.api.Assertions.assertThat;

public class Reproduced {

    @Test
    public void should_testNullCorrectlyWithAssertFactory() {
        // given
        StringHolder holder = new StringHolder(null);

        // then
        assertThat(holder)
            .extracting(StringHolder::value, InstanceOfAssertFactories.STRING)
            .isNull(); // this test will fail
    }

    public record StringHolder(String value) { }
}

kamil-sita avatar Nov 21 '23 14:11 kamil-sita

extracting(Function, InstanceOfAssertFactory) internally relies on asInstanceOf which performs a preliminary isInstanceOf check, the source of the AssertionError you are experiencing.

Is there a reason why you are using InstanceOfAssertFactories.STRING even though you already expect the value to be null?

On a side note, the Javadoc of extracting and asInstanceOf could be improved to cover such cases.

scordio avatar Nov 21 '23 14:11 scordio

Is there a reason why you are using InstanceOfAssertFactories.STRING even though you already expect the value to be null?

Not exactly, no. I have created helper methods for exporting properties of the objects I'm testing, like this:

public AbstractStringAssert<?> assertionForPropertyXyz() {
   // some complicated code I would prefer not to repeat tens of times
   
   return assertThat(abc)
       .extracting(Abc::getXyz, InstanceOfAssertFactories.STRING);
}

and have planned to use them in my code like so:

// test 1
   someObject.assertionForPropertyXyz().startsWith("lorem ipsum");
// test 2
   someObject.assertionForPropertyXyz().isNull();

I realize that it might be an uncommon approach, however I also think that having extracting methods behave differently here is very surprising. Personally I think that both should behave in more-or-less the same way.

kamil-sita avatar Nov 21 '23 15:11 kamil-sita

It's a fair request 🙂 However, we need a more comprehensive analysis of what this change would mean, especially because we expose some more generic versions of extracting for third-party extensions.

We'll discuss it internally and get back to you.

scordio avatar Nov 21 '23 15:11 scordio

Thank you!

kamil-sita avatar Nov 21 '23 15:11 kamil-sita

Hi, I wonder if you had an occasion to discuss this issue internally?

kamil-sita avatar Jan 29 '24 16:01 kamil-sita