Polish "Add consumer test utility" #592
Conversation
- update javadocs a bit - reorder some constructors based on specificity - add null check to "withSchema" - rename the test to end w/ "Tests" (plural) for consistency - simplified doc examples a bit w/ String message type - drop the exact match API - rename the conditions factory methods
| @@ -1,42 +1,46 @@ | |||
| [[testing-applications]] | |||
| = Testing Applications | |||
|
|
|||
There was a problem hiding this comment.
When there is a blank line it prevents it from showing the TOC.
Once we publish the module then we can link this awesome document you added to the navbar.adoc.
| * @param expectation the expected value | ||
| * @param <T> the type of the message | ||
| * @return the condition | ||
| */ | ||
| static <T> ConsumedMessagesCondition<T> anyMessageMatchesExpected(T expectation) { | ||
| return messages -> messages.stream().anyMatch(message -> message.getValue().equals(expectation)); | ||
| static <T> ConsumedMessagesCondition<T> atLeastOneMessageMatches(T expectation) { |
There was a problem hiding this comment.
The naming of these I struggled with until I started using the util.
"consume messages until at least one message matches 'a'"
helps me understand what this is doing.
Likewise w/ the N variant below,
"consume messages until at least one message matches each of 'a', 'b', and 'c'"
| */ | ||
| @SafeVarargs | ||
| @SuppressWarnings("varargs") | ||
| static <T> ConsumedMessagesCondition<T> containsExactlyExpectedValues(T... expectation) { |
There was a problem hiding this comment.
I am proposing to drop this variant as I think it may be a bit confusing as well as it will keep looking for matches up to the timeout even though we know that once it passes N messages w/o a match we could short circuit (i.e. it will never match).
Also, I would like to avoid maintaining an ever growing list of condition permutations. I propose we go out w/ these 3 and then per user request add new conditions when people ask for them. Sound good?
| @@ -67,6 +69,13 @@ public SchemaSpec<T> fromTopic(String topic) { | |||
| return this; | |||
| } | |||
|
|
|||
| @Override | |||
| public ConditionsSpec<T> withSchema(Schema<T> schema) { | |||
| Assert.notNull(schema, "Schema must not be null"); | |||
There was a problem hiding this comment.
Moved this up in order-must-be-executed for a nice readable mental flow and also added null check. Did you leave it out for a particular reason that I am missing?
| @Override | ||
| public List<Message<T>> get() { | ||
| var messages = new ArrayList<Message<T>>(); | ||
| try { | ||
| String subscriptionName = UUID.randomUUID() + "-test-consumer"; | ||
| var subscriptionName = "test-consumer-%s".formatted(UUID.randomUUID()); |
There was a problem hiding this comment.
Swapped to suffix as it will be easier to debug later "test-consumer-lfkdlsajskdljdfsa" than "lfjdslfjfadl-test-consumer" (probably)
| * | ||
| * @author Jonas Geiregat | ||
| */ | ||
| class ConsumedMessagesConditionsTests { |
There was a problem hiding this comment.
I'm sorry these are showing up as delete/add rather than a rename 😿
All I did was collapse some spacing and re-order a bit. These tests you did are solid.
|
|
||
| private DefaultPulsarConsumerFactory<String> pulsarConsumerFactory; | ||
|
|
||
| private static String testTopic(String suffix) { |
There was a problem hiding this comment.
Same as above 😿
- I also scoped the topic name a bit more w/ prefix of acronym of the test class name (we do this in other tests as well).
- I also used the more specific AssertJ "assertThat(IllegalArgument|IllegalState|ExceptionOfType)` APIs
@jonas-grgt amazing work on the PulsarConsumerTestUtil 🎉
I was doing normal "polish" commit and I ended up making a few changes that I wanted to run by you rather than just piggyback in.
Nit polish things
Changes
Some of these things I missed during code review and some of them I just did not see until I pulled the util down and started to use it etc..