Skip to content

Convert Lettuce 4.0 tests from groovy to java #9419

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

Merged
merged 9 commits into from
Sep 15, 2023

Conversation

jaydeluca
Copy link
Member

@jaydeluca jaydeluca commented Sep 8, 2023

Related to #7195

A few notes:

  • The groovy tests were spinning up a new redis container for every test. I have changed things a bit where for each class all tests will share the same container with the exception of the two tests per class that invoke commands that result in the redis container crashing. Those tests spin up their own containers.
    • I tested the speed difference of the new tests vs old and the new tests run in under 4s on my computer where the previous tests take ~12 seconds 💪
  • The groovy tests used a spock helper class spock.util.concurrent.AsyncConditions, which I ended up re-creating, but if there's another existing class or strategy I should use in place of that, please let me know and I can make those adjustments Updated to use Awaitibility

@jaydeluca jaydeluca requested a review from a team September 8, 2023 10:56
@mateuszrzeszutek
Copy link
Member

  • I tested the speed difference of the new tests vs old and the new tests run in under 4s on my computer where the previous tests take ~12 seconds 💪

Awesome! 🚀

  • The groovy tests used a spock helper class spock.util.concurrent.AsyncConditions, which I ended up re-creating, but if there's another existing class or strategy I should use in place of that, please let me know and I can make those adjustments

We usually use awaitility for these kinds of things. AFAIR it should even be automatically imported in the testing-common module.


@Test
void testCommandChainedWithThenAccept() throws Throwable {
AsyncConditions conditions = new AsyncConditions();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of that you could use either an AtomicReference or a CompletableFuture that you would set from the redisFuture.thenAccept() callback; and then assert on that ref/future using Awaitility.await()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for this suggestion, I believe I have implemented the proposed change

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, thanks! Left just a couple small comments

static RedisCommands<String, String> syncCommands;

@BeforeAll
public static void setUp() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: JUnit 5 test lifecycle methods need not be public

Suggested change
public static void setUp() {
static void setUp() {

equalTo(SemanticAttributes.NET_PEER_NAME, host),
equalTo(SemanticAttributes.NET_PEER_PORT, port),
equalTo(SemanticAttributes.DB_SYSTEM, "redis"))));
testConnection.close();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be safer to use the AutoCleanupExtension; the connection would get closed even if the assertion fails.

@mateuszrzeszutek mateuszrzeszutek merged commit f7b2de7 into open-telemetry:main Sep 15, 2023
@jaydeluca jaydeluca deleted the lettuce-4.0 branch September 24, 2023 15:32
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.

2 participants