assertj/doc

Fix documentation/name for `AbstractMapAssert#containsOnlyKeys`

Opened this issue · 8 comments

C-Otto commented

According to the method name and it's documentation I expect assertThat(emptyMap()).containsOnlyKeys("x") to NOT fail.

There's one example in the Javadoc that helps me understand the real semantics. However, I'd like to see that also in the text above, and ideally in the method name.

Related: https://math.stackexchange.com/questions/202452/why-is-predicate-all-as-in-allset-true-if-the-set-is-empty

See assertj/assertj#200

joel-costigliola commented

Hi @C-Otto,
The doc states:

Verifies that the actual map contains only the given keys and nothing else, in any order.
which seems clear (to me at least) that the given keys must be in the map thus the assertion will always fail for empty map.

Could you write the text you would you like to see in the javadoc?

C-Otto commented
  1. Verifies that the actual map contains the given keys and nothing else, in any order.
  2. Verifies that the actual map contains exactly the given keys, in any order.

To me the word "only" in "X ... only Y" implies a restriction on X so that Y must be satisfied. As such, the condition is satisfied, as long as every item of the set X conforms to whatever Y expresses. This also means that "X ... only Y" automatically is satisfied for empty X, no matter the Y.

I'm not a native speaker, but several translations to my native language lead to the same thoughts.

joel-costigliola commented

But the condition is not on every item, it's on the collection.
The reasoning for this assertion is that an empty collection does not contain anything, if we agree on that then containsOnlyKeys(key1, key2, ...) must fail for empty collections.

C-Otto commented

I totally get what the code is doing, and in fact it is doing what I need. I just don't like the name and the documentation, as they don't match the code (in my understanding). I also see that you have a different understanding. I'd just like to avoid confusion by making the description easier to understand. I might just be plain wrong, that's for you to decide.

An empty collection does not contain anything, that's correct. I also see that the invocation you mentioned should fail. However, the "only" in the method name still leads me to the thought that it should pass! "The keys that are contained in the actual map (namely none) satisfiy any criterion, especially the one that they are key1 or key2 or key3, and nothing else. As such, the check should be true."

Maybe it helps if we don't talk about the empy map/set/...?

assertThat(singletonMap("a", ".")).containsOnlyKeys("a", "b")

I'd say this is OK, as every single key in the actual map ("a") is either "a" or "b".

scordio commented

Hi @C-Otto, based on your last update, I wanted to react on:

assertThat(singletonMap("a", ".")).containsOnlyKeys("a", "b")

I'd say this is OK, as every single key in the actual map ("a") is either "a" or "b".

The behavior described would fit an assertion named like containsAnyOfKeys(), but personally I don't find "any of" and "only" having interchangeable meaning.

Would you consider this issue still valid?

  • 👍1
C-Otto commented

Yes. My expectation based on my background in math/logic and my non-native English skills:

  • assertThat(singletonMap("a", ".")).containsOnlyKeys("a", "b") -> true (every key is either a or b)
  • assertThat(Map.of("a", ".", "c", ".")).containsOnlyKeys("a", "b") -> false (there is a key (c) that is not a and not b)
  • assertThat(emptyMap()).containsOnlyKeys("a", "b") -> true (every key is either a or b)
  • assertThat(emptyMap()).containsAnyOfKeys("a", "b") -> false (at least one of a or b is expected)
  • assertThat(singletonMap("a", ".")).containsAnyOfKeys("a", "b") -> true (key a is either a or b)
  • assertThat(Map.of("a", ".", "c", ".")).containsAnyOfKeys("a", "b") -> true (key a is either a or b)

For me, "X containsOnly p" translates to "for all x in X it holds that p(x) is true".
Likewise, "X containsAnyOf p" translates to "there exists (at least one) x in X for which p(x) is true".
I'm not exactly sure how "p(x)" should be evaluated in the context of a set. Maybe this is where the confusion comes from? No matter what, for the "for all x in X" case where X is empty, it should not matter what p(x) means, as the result (for the whole expression) should be true.

"for all" and "there exists" are very fundamental principles in math/logic, and for me AssertJ does not stick to the semantics I'm used to.

scordio commented

I'm sorry, on some points I struggle to follow your reasoning.

I understand that you refer as X to the input map, while x is the element of the map (it could be the key or the entry, but I think it's not relevant at the moment).

I don't understand what p and p(x) are. Could you please clarify them?

If the assertion were named containsExactlyKeysInAnyOrder, would you still find it confusing?

C-Otto commented

I use p as a proposition function (i.e., a proposition if provided an input) which reasons about entries, i.e. p(x) is a proposition which reasons about x and provides true or false (https://en.wikipedia.org/wiki/Propositional_calculus).

Yes, containsExactlyKeysInAnyOrder would be a fitting name. As said before, I'd also be happy if the documentation made clear what the semantics of the assertions are (so that any name-related confusion might be fixed by RTFM).