-
Notifications
You must be signed in to change notification settings - Fork 155
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
Change food items list test to ignore order #144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment inline for testing collection equality without regard for order.
"CHOCOLATE_CHIP cookie", "GLAZED donut"); | ||
assertEquals(expected, coffeeShopOrder.getFoodItemsForOrder()); | ||
List<String> expected = List.of("CHOCOLATE_CHIP cookie", "EVERYTHING bagel with HERB_GARLIC_CREAM_CHEESE", "GLAZED donut"); | ||
assertThat(coffeeShopOrder.getFoodItemsForOrder()).hasSameElementsAs(expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to use a Bag
from Eclipse Collections to test that two Lists contain the same elements without requiring the same order. Since Eclipse Collections is already a dependency in other modules in this repo, I would prefer Bag
be used here to test element only equality instead of adding a new library dependency.
The code would look as follows:
assertEquals(Bags.mutable.of(expected), Bags.mutable.of(coffeeShopOrder.getFoodItemsForOrder()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Don! I think assertThat.hasSameElementsAs() reads easier for beginners, and people may not be familiar with Bag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new dependency (with other transitive dependencies) for a single assertion method does not make sense to me. The kata is intended to teach new Java features, not to teach a third-party assertion library. Add a method named "assertHasSameElementsAs(List, List)" and the readability is similar.
public void assertHasSameElementsAs(List<?> expected, List<?> actual)
{
assertEquals(Bags.mutable.of(expected), Bags.mutable.of(actual));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @emilie-robichaud, Bag is an advanced topic and Eclipse Collections is itself not covered as part of this kata and it would not make sense to add it in tests just for assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prathasirisha @emilie-robichaud The method assertHasSameElementsAs above can be implemented using JDK collections only then.
coffee-shop-kata/jdk21/pom.xml
Outdated
<groupId>org.assertj</groupId> | ||
<artifactId>assertj-core</artifactId> | ||
<version>3.24.2</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add test to this dependency
9834d40
to
16f6276
Compare
…tFoodItemsForOrder Signed-off-by: Emilie <[email protected]>
16f6276
to
f615dcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.