Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix documentation/name for AbstractMapAssert#containsOnlyKeys #23

Open
C-Otto opened this issue Nov 19, 2019 · 8 comments
Open

Fix documentation/name for AbstractMapAssert#containsOnlyKeys #23

C-Otto opened this issue Nov 19, 2019 · 8 comments

Comments

@C-Otto
Copy link

C-Otto commented Nov 19, 2019

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

@C-Otto C-Otto changed the title Fix documentation/name for AbstractMapAssert.html#containsOnlyKeys Fix documentation/name for AbstractMapAssert#containsOnlyKeys Nov 19, 2019
@joel-costigliola
Copy link
Member

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
Copy link
Author

C-Otto commented Nov 23, 2019

  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
Copy link
Member

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
Copy link
Author

C-Otto commented Nov 24, 2019

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
Copy link
Member

scordio commented May 26, 2021

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?

@C-Otto
Copy link
Author

C-Otto commented May 26, 2021

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
Copy link
Member

scordio commented May 26, 2021

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
Copy link
Author

C-Otto commented Jun 3, 2021

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants