Enhance consumer test util #601

KartikShrivastava · 2024-03-10T15:56:05Z

Resolves #599

jonas-grgt

Wonderful to see someone pick up and improve spring-pulsar based on the work I did a couple of weeks back! Way to go @KartikShrivastava 🎉

You did a really nice job and it's definitely a worthwhile contribution. I did raise a couple of questions.

There are also some formatting violations that are easy to fix simply by running ./gradlew format

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

jonas-grgt · 2024-03-10T19:51:43Z

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

@@ -85,6 +122,12 @@ public ConditionsSpec<T> awaitAtMost(Duration timeout) {

 	@Override
 	public ConditionsSpec<T> until(ConsumedMessagesCondition<T> condition) {
+		if (calledUntil) {


Do we really need to introduce a new variable? Is there a reason why we can't rely on this.condition being not being null to verify if until has been called before?

I believe we do since we do not have to specify a condition (e.g. PCTUT.whenConditionIsNotSpecifiedMessagesAreConsumedUntilAwaitDuration).

I would like to suggest to rename the var to untilMethodAlreadyCalled as the word until in general is a confusing one that makes my brain pause when reading the code "until something happens". It seems nit picky but including the "method" in there helps readability for me anyways.

untilMethodAlreadyCalled is better than calledUntil imo

onobc

@KartikShrivastava, I could not agree w/ @jonas-grgt more on this being a great contribution. Thank you for this.

I also like that you submitted the WIP before investing too much into testing etc..that way we can align on current direction. Good call.

I have left a few comments/suggestions.

Great work.

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

onobc · 2024-03-10T22:19:43Z

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

@@ -85,6 +122,12 @@ public ConditionsSpec<T> awaitAtMost(Duration timeout) {

 	@Override
 	public ConditionsSpec<T> until(ConsumedMessagesCondition<T> condition) {
+		if (calledUntil) {


I believe we do since we do not have to specify a condition (e.g. PCTUT.whenConditionIsNotSpecifiedMessagesAreConsumedUntilAwaitDuration).

I would like to suggest to rename the var to untilMethodAlreadyCalled as the word until in general is a confusing one that makes my brain pause when reading the code "until something happens". It seems nit picky but including the "method" in there helps readability for me anyways.

...-test/src/test/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtilTests.java

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

KartikShrivastava · 2024-03-11T14:43:07Z

Thank you @jonas-grgt and @onobc, for your kind words and detailed suggestion for improvements. Checking them out now

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

onobc

@KartikShrivastava, thanks for the updates. LGTM.

The only remaining piece is to add a few tests to cover the newly added methods.

KartikShrivastava · 2024-03-17T17:25:53Z

...-test/src/test/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtilTests.java

+	void messagesAreConsumedWhenContainerIsStoppedAndConsumeMessagesIsCalledWithoutArguments() {
+		PulsarTestContainerSupport.stopContainer();
+		PulsarConsumerTestUtil.<String>consumeMessages();
+		// TODO: Complete this test


I'd like to request some assistance writing this test

Basically I want to test if consumeMessages works properly even if PULSAR_CONTAINER is not running

When I try to get() the consumeMessages I get connection refused error for the localhost pulsar client

Also I'm not sure at what point should I send messages to the topic, if I do it before stopping the container will it still work? since the pulsarClient instance would be different after container is stopped

Yep, this one is tricky to test. We actually can't stop the container as the other tests will fail. The "magic" of the PTCS is that it has the @BeforeAll that starts it for the test class. If we stopped it we would have to restart it and then it gets into a bit of flaky-test-land at that point.

I think instead we can static mock the "isContainerStarted" that it uses to decide what to do.

We are releasing 1.1.0-M2 tomorrow so I am going to take what you have in this current state and add a polish commit on top so that we can get this in. I will include this test adjustment in that commit.

Great work @KartikShrivastava !

Also, if you are wondering why we did not have (or need) the stop container method annotated w/ a @AfterAll to match the start w/ / @BeforeAll it is because Testcontainers does

At the end of the test suite the Ryuk container that is started by Testcontainers core will take care of stopping the singleton container.

However, I think I will add it in the polish commit to avoid future confusion

Note
Adding the @AfterAll to the new stopContainer method had unintended side-effects when used in a Jupiter @Nested test. I removed the annotation but left the method (9bc6bda)

Cool, thanks for taking this up @onobc, really appreciate that.
I've one minor question seeing the improvements you've made: Is there a recommended way or methodology we follow to name test methods in this project? If so, then I would like to learn/read more about it :)

You are welcome @KartikShrivastava .
In general we follow the "when-THIS-then-OUTCOME" but more than anything the principle I follow is consistency w/in a test suite and enough information in the name to understand what it is testing. I will also typically group tests together using @Nested which groups them and allows us to not have ultra long test names. I was tempted to do some grouping on this test but ended up leaving as is.

I also noticed that the negative test cases I named a bit different than the other ones (they are shorter in nature).

So there is no doc that describes this but it would be a good idea for us to add one. I will note this.

Thanks

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java

KartikShrivastava · 2024-03-17T17:30:29Z

Hey folks, I've added the tests for the changes made in PulsarConsumerTestUtil and left few questions I got while testing the new methods
PS: Seems like build failed due to a known flaky test

onobc · 2024-03-17T19:58:59Z

Closing via 2c7e3b2

onobc · 2024-03-17T20:01:58Z

@KartikShrivastava thanks for this great contribution. Take a peek at the follow on commit I did d99101f. You can see how I uses static mocks to test our difficult case. I re-ordered the test methods a bit as well. Great work. Thank you.

KartikShrivastava · 2024-03-18T17:18:45Z

Thanks @onobc!

KartikShrivastava added 3 commits March 10, 2024 21:19

add overloads of consumeMessage for convinence

9aa3134

throw exception if until is called more than once

b2a2707

use different topic names in different unit tests

0d2d69e

jonas-grgt suggested changes Mar 10, 2024

View reviewed changes

fix formatting

20317a5

onobc requested changes Mar 10, 2024

View reviewed changes

KartikShrivastava added 4 commits March 12, 2024 00:39

add new method to check if test container is already running

93ac326

update consumeMessages variants to use one another in fluent style

a387d70

rename calledUntil private variable to untilMethodAlreadyCalled

3f36838

rename test in PulsarConsumerTestUtilTests

74192bc

onobc reviewed Mar 11, 2024

View reviewed changes

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java Outdated Show resolved Hide resolved

onobc requested changes Mar 11, 2024

View reviewed changes

KartikShrivastava added 2 commits March 17, 2024 15:18

fix consumeMessages to use test container broker url

0e26362

add tests for recently added methods

0e1df28

KartikShrivastava marked this pull request as ready for review March 17, 2024 17:21

KartikShrivastava requested review from jonas-grgt and onobc March 17, 2024 17:21

KartikShrivastava commented Mar 17, 2024

View reviewed changes

...ulsar-test/src/main/java/org/springframework/pulsar/test/support/PulsarConsumerTestUtil.java Show resolved Hide resolved

onobc closed this Mar 17, 2024

KartikShrivastava deleted the enhance-consumer-test-util branch March 18, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance consumer test util #601

Enhance consumer test util #601

KartikShrivastava commented Mar 10, 2024 •

edited by onobc

Loading

jonas-grgt left a comment •

edited

Loading

jonas-grgt Mar 10, 2024

onobc Mar 10, 2024

KartikShrivastava Mar 11, 2024

onobc left a comment

onobc Mar 10, 2024

KartikShrivastava commented Mar 11, 2024

onobc left a comment

KartikShrivastava Mar 17, 2024

onobc Mar 17, 2024

onobc Mar 17, 2024

onobc Mar 18, 2024

KartikShrivastava Mar 18, 2024

onobc Mar 18, 2024

KartikShrivastava commented Mar 17, 2024 •

edited

Loading

onobc commented Mar 17, 2024

onobc commented Mar 17, 2024

KartikShrivastava commented Mar 18, 2024

Enhance consumer test util #601

Enhance consumer test util #601

Conversation

KartikShrivastava commented Mar 10, 2024 • edited by onobc Loading

jonas-grgt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onobc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KartikShrivastava commented Mar 11, 2024

onobc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KartikShrivastava commented Mar 17, 2024 • edited Loading

onobc commented Mar 17, 2024

onobc commented Mar 17, 2024

KartikShrivastava commented Mar 18, 2024

KartikShrivastava commented Mar 10, 2024 •

edited by onobc

Loading

jonas-grgt left a comment •

edited

Loading

KartikShrivastava commented Mar 17, 2024 •

edited

Loading