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

Add convenience methods for MapEntry assertions #117

Closed
wants to merge 1 commit into from

Conversation

sberan
Copy link

@sberan sberan commented Oct 12, 2012

Quite often, I find myself testing map entries in the following way:

Map<String, String> address = ....
assertThat(address)
                .hasSize(15)
                .includes(
                        MapAssert.entry("zip", "43865"),
                        MapAssert.entry("lastName", "Frazier"),
                        MapAssert.entry("state", "IA"),
                        MapAssert.entry("city", "Culloden")
                );

This change allows me to simplify the map assertions by getting rid of the repeated MapAssert.entry calls:

Map<String, String> address = ....
assertThat(address)
                .hasSize(15)
                .includesEntry("zip", "43865")
                .includesEntry("lastName", "Frazier")
                .includesEntry("state", "IA")
                .includesEntry("city", "Culloden");

Please let me know if there's anything I can do to clean up the request!

Thanks for all your work on FEST! Assert and Reflect are life savers!
Sam

 - containsEntry(K key, V value)
 - doesNotContainEntry(K key, V value)
@alexruiz
Copy link
Owner

This is really, really nice. Thanks Sam!

Also, thank you for kind words about FEST :)

We are in the middle of a mega huge refactoring. Once we are done, I'll merge this pull requests.

Cheers,
-Alex

@alexruiz
Copy link
Owner

I'll be removing the method includes(Entry) after merging your pull request.

@joel-costigliola
Copy link
Contributor

I like the idea, it makes it easier to check for entries than with MapAssert.entry parameters.
But on our latest version we use contains assertions instead of includes to be consistent with Iterables assertions.
So I think it should be changed to :

 Map<String, String> address = ....
 assertThat(address).hasSize(15)
                    .containsEntry("zip", "43865")
                    .containsEntry("lastName", "Frazier")
                    .containsEntry("state", "IA")
                    .containsEntry("city", "Culloden");

Moreover, we should also change doesNotContain(MapEntry... entries) to doesNotContainEntry(K, V)

@sberan
Copy link
Author

sberan commented Oct 12, 2012

Awesome, thanks Alex.

Let me know if you have any issues applying the patch after the refactor
and I'll fix it up.

Sam

@alexruiz
Copy link
Owner

Good point Joel! I agree with you.

Thanks Sam. I'll let you know if I find any issues while merging it.

@christophsturm
Copy link

so what about the refactoring and this pull request? will it be merged soon?

@joel-costigliola
Copy link
Contributor

Alex is making a huge refactoring, contributions are in a pending state for the time being ...

@jmnarloch
Copy link

Hi,

First of all please take into consideration that includes() method error message is hard to read for multiple parameters that have different values. So instead we have asserting invidual keys:

Map<String, int> map = ...

assertThat(map.get("key1")).isEqualTo(1);
assertThat(map.get("key2")).isEqualTo(2);

Although such code looks very repetetive.

I have lately came up with similar idea as mentioned in this issue, although I have prototyped different API, which look like this:

Map<String, int> map = ...

assertThat(map)
               .entry("key1").isEqualTo(1)
               .entry("key2").isEqualTo(2)

Although there would be limitation to single predicate per entry, but we could simply return a subclass of AbstractAssert, so we could have a much grater flexibility.

If you like to I could prepare a pull request for that.

@sberan
Copy link
Author

sberan commented Mar 27, 2013

I really like this idea. I wonder if it would be worth renaming the entry()
method valueForEntry() so as to make it a little more readable. You could
also add a method doesNotExist() to the entry assertion. This would allow
you to say things like:

assertThat(map)
    .valueForEntry("key1").isEqualTo("foo")
    .valueForEntry("key2").doesNotExist()

On Wednesday, March 27, 2013, Jakub Narloch wrote:

Hi,

First of all please take into consideration that includes() method error
message is hard to read for multiple parameters that have different values.
So instead we have asserting invidual keys instead:

Map<String, int> map = ...

assertThat(map.get("key1")).isEqualTo(1);
assertThat(map.get("key2")).isEqualTo(2);

Although such code looks very repetetive.

I have lately came up with similar idea as mentioned in this issue,
although I have prototyped different API, which look like this:

Map<String, int> map = ...

assertThat(map)
.entry("key1").isEqualTo(1)
.entry("key2").isEqualTo(2)

Although there would be limitation to single predicate per entry, but we
could simply return a subclass of AbstractAssert, so we could have a much
grater flexibility.

If you like to I could prepare a pull request for that.


Reply to this email directly or view it on GitHubhttps://github.com//pull/117#issuecomment-15557068
.

@joel-costigliola
Copy link
Contributor

I like the latest ideas, I will probably add them in AssertJ core.
Just to let you know the convenience methods containsEntry and doesNotContainEntry has been added in AssertJ 1.2.0 release (AssertJ is a fork of Fest Assert).

Regards,

Joel Costigliola

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

Successfully merging this pull request may close these issues.

None yet

5 participants