-
Notifications
You must be signed in to change notification settings - Fork 967
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
Conversation
Awesome! 🚀
We usually use awaitility for these kinds of things. AFAIR it should even be automatically imported in the |
...test/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceSyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
|
||
@Test | ||
void testCommandChainedWithThenAccept() throws Throwable { | ||
AsyncConditions conditions = new AsyncConditions(); |
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.
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()
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.
thank you for this suggestion, I believe I have implemented the proposed change
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
…mentation into lettuce-4.0
…d utilize AutoCleanupExtension
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.
This is looking great, thanks! Left just a couple small comments
static RedisCommands<String, String> syncCommands; | ||
|
||
@BeforeAll | ||
public static void setUp() { |
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.
Nit: JUnit 5 test lifecycle methods need not be public
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(); |
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'd be safer to use the AutoCleanupExtension
; the connection would get closed even if the assertion fails.
Related to #7195
A few notes:
The groovy tests used a spock helper classUpdated to use Awaitibilityspock.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