Assertj-core: Maps.assertContainsOnly(info, actual, entries) has inconsistent behaviour if `entries` is empty

[oxygenecore]

https://github.com/assertj/assertj-core/blob/f27b022a369ba731ed72413f6c10c82a9e539495/src/main/java/org/assertj/core/internal/Maps.java#L745
The call mentioned in the link fails with IllegalArgumentException if entries is empty, and actual is not. This, however, would work if actual is empty as well (the mentioned line is not executed in this case). Thus, execution success depends on actual input, which is the most frequently changing part of the test.
I would expect this code to fail with AssertionError with proper message (actual should be empty, but was XXX)

[joel-costigliola]

I agree the error message should be as you said.
Are you keen to contribute it?

[abhijeetshuklaoist]

@joel-costigliola I would like to pick it up. If it's free for me to take, please assign it to me.

[joel-costigliola]

Thanks @abhijeetshuklaoist

[joel-costigliola]

@abhijeetshuklaoist have you made any progress?

[abhijeetshuklaoist]

Sorry I have been occupied for some time. I will get to it right away.
Thanks!

[abhijeetshuklaoist]

@joel-costigliola What should be the correct behaviour here?

Below are the use cases that I think should be there, if let me know if my understanding is correct.

1. If both Actual and Entries are empty : Should be an assertion success
2. If Actual is empty but Entries is not empty: AssertionFail with message "Actual is empty but should contain Entries"
3. If Actual is not empty but Entries is empty: AssertionFail with message "Actual should be empty but was Entries", but for this scenario users should actually be using assertEmpty, right?
   Thanks!

[joel-costigliola]

that's correct, 1. and 2. are already handled properly, 3. can be improved with a better error message.
We must not change failIfEmpty because it is used elsewhere, instead we replace it in assertContainsOnly and assertContainsExactly by calling assertEmpty as you suggested (good idea!) if entries is empty.

Does that make sense?