alexruiz / fest-assert-2.x

FEST Fluent Assertions 2.x
http://fest.easytesting.org
Apache License 2.0
401 stars 69 forks source link

Add convenience methods for MapEntry assertions #117

Closed sberan closed 9 years ago

sberan commented 11 years ago

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

alexruiz commented 11 years ago

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 commented 11 years ago

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

joel-costigliola commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

Good point Joel! I agree with you.

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

christophsturm commented 11 years ago

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

joel-costigliola commented 11 years ago

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

jmnarloch commented 11 years ago

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 commented 11 years ago

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/alexruiz/fest-assert-2.x/pull/117#issuecomment-15557068 .

joel-costigliola commented 11 years ago

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