comparingOnlyFields gives false positive on iterable objects

[mikehaertl]

Describe the bug

When comparingOnlyFields() is used on a list of objects to recursively compare object properties it never reports a failure.

I would have expected it to behave like a complement to ignoringFields() (which works as expected on a list of objects).

• assertj core version: 3.24.2
• java version: Temurin-17.0.6+10 (build 17.0.6+10)
• test framework version: junit-jupiter:5.9.1

Test case reproducing the bug

package foo;

import java.util.Arrays;
import java.util.List;

import org.junit.jupiter.api.Test;

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

class FooTest {

    public class Person {
        String firstname;
        String lastname;

        public Person(String firstname, String lastname) {
            this.firstname = firstname;
            this.lastname = lastname;
        }
    }

    @Test
    public void listExample() {
        Person foo = new Person("Foo First", "Foo Last");
        Person bar = new Person("Bar First", "Bar Last");

        Person expectedFoo = new Person("Foo First", "wrong");
        Person expectedBar = new Person("Bar First", "wrong");

        List<Person> people = Arrays.asList(foo, bar);
        List<Person> expectedPeople = Arrays.asList(expectedFoo, expectedBar);

        // Works
        assertThat(people)
                .usingRecursiveComparison()
                .ignoringFields("lastname")
                .isEqualTo(expectedPeople);

        // Fails as expected
        assertThat(people)
                .usingRecursiveComparison()
                .ignoringFields("firstname")
                .isEqualTo(expectedPeople);


        // Should fail - but works!
        assertThat(people)
                .usingRecursiveComparison()
                .comparingOnlyFields("lastname")
                .isEqualTo(expectedPeople);

        // Should also fail - but works!
        assertThat(people)
                .usingRecursiveComparison()
                .comparingOnlyFields("bla", "blub")
                .isEqualTo(expectedPeople);

    }
}

[aindriu-aiven]

I was able to replicate the behavior on tags/assertj-build-3.24.2 where both of the below pass.
// Should fail - but works!
assertThat(people)
.usingRecursiveComparison()
.comparingOnlyFields("lastname")
.isEqualTo(expectedPeople);

    // Should also fail - but works!
    assertThat(people)
            .usingRecursiveComparison()
            .comparingOnlyFields("bla", "blub")
            .isEqualTo(expectedPeople);

But the first one is working correctly on main and so I believe it has already been fixed.
I do think that the second test case above should fail, if a field that does not exist is entered into the comparingOnlyFields as i would think spelling mistakes could be quiet common and could lead to a lot of silently failing tests.

I'd like to pick this issue up if the assertj team see the benefit in it?

[joel-costigliola]

Thanks for checking on main @aindriu-aiven, I agree that the recursive comparison should fail if comparingOnlyFields is given non-existing fields so your PR is welcome.

[aindriu-aiven]

Thanks @joel-costigliola I'll pick it up!

[aindriu-aiven]

@joel-costigliola
I have opened PR #3137 to check for non-existing fields.
One thing to note is that the following test currently ignores the comparingOnlyFields as it is in a list and a deep compare is instead done to make sure they are the same. I didn't want to change this behaviour as it seemed like a much larger change that should be discussed first.
// Should also fail - but works!
assertThat(people)
.usingRecursiveComparison()
.comparingOnlyFields("bla", "blub")
.isEqualTo(expectedPeople);

[joel-costigliola]

Integrated thanks @aindriu-aiven !