Skip to content
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.

Sign up for GitHub

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

Clean up all warnings in Java client test #1213

Merged
merged 8 commits into from
Apr 10, 2024
Merged

Clean up all warnings in Java client test #1213

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e915aa1
Clean up all warnings in Java client test (#163)
acarbonetto Apr 1, 2024
9a58a2c
Clean FfiTest.java of warnings
acarbonetto Apr 3, 2024
4b6140f
Clean RedisClusterClientTest.java of compiler warnings
acarbonetto Apr 3, 2024
960ef67
Clean up more warnings in client tests
acarbonetto Apr 4, 2024
9943e18
Spotless
acarbonetto Apr 4, 2024
8b5fbb9
Fix UT + update IT.
Yury-Fridlyand Apr 10, 2024
a518936
Update suppression.
Yury-Fridlyand Apr 10, 2024
1e13331
Remove suppress warnings
acarbonetto Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
287 changes: 143 additions & 144 deletions java/client/src/test/java/glide/api/RedisClientTest.java
View file
Open in desktop

Large diffs are not rendered by default.

200 changes: 103 additions & 97 deletions java/client/src/test/java/glide/api/RedisClusterClientTest.java
View file
Open in desktop

Large diffs are not rendered by default.

19 changes: 10 additions & 9 deletions java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,16 @@ public void rethrow_error_on_read_when_malformed_packet_received() {
@Test
@SneakyThrows
public void rethrow_error_if_UDS_channel_closed() {
var client = new TestClient(channelHandler);
stopRustCoreLibMock();
try {
var exception =
assertThrows(ExecutionException.class, () -> client.customCommand(new String[0]).get());
assertTrue(exception.getCause() instanceof ClosingException);
} finally {
// restart mock to let other tests pass if this one failed
startRustCoreLibMock(null);
try (var client = new TestClient(channelHandler)) {
stopRustCoreLibMock();
try {
var exception =
assertThrows(ExecutionException.class, () -> client.customCommand(new String[0]).get());
assertTrue(exception.getCause() instanceof ClosingException);
} finally {
// restart mock to let other tests pass if this one failed
startRustCoreLibMock(null);
}
}
}

Expand Down
15 changes: 9 additions & 6 deletions java/client/src/test/java/glide/connectors/resources/ThreadPoolResourceAllocatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@

public class ThreadPoolResourceAllocatorTest {

ThreadPoolResourceAllocator service;

@BeforeEach
public void init() {
var threadPoolResource = ThreadPoolResourceAllocator.getOrCreate(() -> null);
Expand All @@ -30,17 +28,19 @@ public void init() {
public void getOrCreate_returns_default_after_repeated_calls() {
ThreadPoolResource mockedThreadPoolResource = mock(ThreadPoolResource.class);
EventLoopGroup mockedEventLoopGroup = mock(EventLoop.class);
@SuppressWarnings("unchecked")
Copy link
Collaborator

@barshaul barshaul Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why? what's the warning? can't it be fixed other way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Supplier is a paramtrized type (it has <>), but mock returns a raw type of Supplier and can't return Supplier<something>.
Assigning Supplier to a Supplier<...> does not require a cast, but it produces a compiler warning, which we acknoledge and suppress.
If we keep using raw Supplier we have another warning about using raw types, which should be also suppressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use an actual supplier instead of mocks. We would lose the verify check, but that's not really important.

Supplier<ThreadPoolResource> threadPoolSupplier = mock(Supplier.class);

when(mockedThreadPoolResource.getEventLoopGroup()).thenReturn(mockedEventLoopGroup);
when(mockedEventLoopGroup.isShuttingDown()).thenReturn(false);
when(threadPoolSupplier.get()).thenReturn(mockedThreadPoolResource);

ThreadPoolResource theResource = service.getOrCreate(threadPoolSupplier);
ThreadPoolResource theResource = ThreadPoolResourceAllocator.getOrCreate(threadPoolSupplier);
assertEquals(mockedThreadPoolResource, theResource);

// Ensure that supplier only is invoked once to set up the shared resource
ThreadPoolResource theSameResource = service.getOrCreate(threadPoolSupplier);
ThreadPoolResource theSameResource =
ThreadPoolResourceAllocator.getOrCreate(threadPoolSupplier);
assertEquals(mockedThreadPoolResource, theSameResource);
verify(threadPoolSupplier, times(1)).get();

Expand All @@ -52,17 +52,20 @@ public void getOrCreate_returns_default_after_repeated_calls() {
public void getOrCreate_returns_new_thread_pool_after_shutdown() {
ThreadPoolResource mockedThreadPoolResource = mock(ThreadPoolResource.class);
EventLoopGroup mockedEventLoopGroup = mock(EventLoop.class);

@SuppressWarnings("unchecked")
Supplier<ThreadPoolResource> threadPoolSupplier = mock(Supplier.class);

when(mockedThreadPoolResource.getEventLoopGroup()).thenReturn(mockedEventLoopGroup);
when(mockedEventLoopGroup.isShuttingDown()).thenReturn(true);
when(threadPoolSupplier.get()).thenReturn(mockedThreadPoolResource);

ThreadPoolResource theResource = service.getOrCreate(threadPoolSupplier);
ThreadPoolResource theResource = ThreadPoolResourceAllocator.getOrCreate(threadPoolSupplier);
assertEquals(mockedThreadPoolResource, theResource);

// Ensure that supplier only is invoked once to set up the shared resource
ThreadPoolResource theSameResource = service.getOrCreate(threadPoolSupplier);
ThreadPoolResource theSameResource =
ThreadPoolResourceAllocator.getOrCreate(threadPoolSupplier);
assertEquals(mockedThreadPoolResource, theSameResource);
verify(threadPoolSupplier, times(2)).get();

Expand Down
10 changes: 5 additions & 5 deletions java/client/src/test/java/glide/ffi/FfiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public void redisValueToJavaValue_Okay() {
}

@ParameterizedTest
@ValueSource(longs = {0L, 100L, 774L, Integer.MAX_VALUE + 1, Integer.MIN_VALUE - 1})
@ValueSource(longs = {0L, 100L, 774L, Integer.MAX_VALUE + 1L, Integer.MIN_VALUE - 1L})
public void redisValueToJavaValue_Int(Long input) {
long ptr = FfiTest.createLeakedInt(input);
Object longValue = RedisValueResolver.valueFromPointer(ptr);
Expand Down Expand Up @@ -98,8 +98,8 @@ public void redisValueToJavaValue_Map() {
long[] values = {1L, 2L, 3L};
long ptr = FfiTest.createLeakedMap(keys, values);
Object mapValue = RedisValueResolver.valueFromPointer(ptr);
assertTrue(mapValue instanceof HashMap);
HashMap<Object, Object> result = (HashMap<Object, Object>) mapValue;
assertTrue(mapValue instanceof HashMap<?, ?>);
HashMap<?, ?> result = (HashMap<?, ?>) mapValue;
assertAll(
() -> assertEquals(1L, result.get(12L)),
() -> assertEquals(2L, result.get(14L)),
Expand Down Expand Up @@ -134,8 +134,8 @@ public void redisValueToJavaValue_Set() {
long[] array = {1L, 2L, 2L};
long ptr = FfiTest.createLeakedLongSet(array);
Object longSetValue = RedisValueResolver.valueFromPointer(ptr);
assertTrue(longSetValue instanceof HashSet);
HashSet<Long> result = (HashSet<Long>) longSetValue;
assertTrue(longSetValue instanceof HashSet<?>);
HashSet<?> result = (HashSet<?>) longSetValue;
assertAll(
() -> assertTrue(result.contains(1L)),
() -> assertTrue(result.contains(2L)),
Expand Down
20 changes: 10 additions & 10 deletions java/client/src/test/java/glide/managers/ConnectionManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,21 @@ public class ConnectionManagerTest {

ChannelHandler channel;

private static String HOST = "aws.com";
private static int PORT = 9999;
private static final String HOST = "aws.com";
private static final int PORT = 9999;

private static String USERNAME = "JohnDoe";
private static String PASSWORD = "Password1";
private static final String USERNAME = "JohnDoe";
private static final String PASSWORD = "Password1";

private static int NUM_OF_RETRIES = 5;
private static int FACTOR = 10;
private static int EXPONENT_BASE = 50;
private static final int NUM_OF_RETRIES = 5;
private static final int FACTOR = 10;
private static final int EXPONENT_BASE = 50;

private static int DATABASE_ID = 1;
private static final int DATABASE_ID = 1;

private static int REQUEST_TIMEOUT = 3;
private static final int REQUEST_TIMEOUT = 3;

private static String CLIENT_NAME = "ClientName";
private static final String CLIENT_NAME = "ClientName";

@BeforeEach
public void setUp() {
Expand Down
17 changes: 6 additions & 11 deletions java/client/src/test/java/glide/utils/RustCoreMock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.nio.file.Files;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
import lombok.SneakyThrows;
import redis_request.RedisRequestOuterClass.RedisRequest;
Expand Down Expand Up @@ -67,13 +68,6 @@ public static Response.Builder OK() {
}
}

public abstract static class GlideMockConnectAll extends GlideMockProtobuf {
@Override
public Response connection(ConnectionRequest request) {
return Response.newBuilder().build();
}
}

/** Thread pool supplied to <em>Netty</em> to perform all async IO. */
private final EventLoopGroup group;

Expand Down Expand Up @@ -113,7 +107,7 @@ private RustCoreMock() {
new ChannelInitializer<DomainSocketChannel>() {

@Override
protected void initChannel(DomainSocketChannel ch) throws Exception {
protected void initChannel(@NonNull DomainSocketChannel ch) {
ch.pipeline()
// https://netty.io/4.1/api/io/netty/handler/codec/protobuf/ProtobufEncoder.html
.addLast("frameDecoder", new ProtobufVarint32FrameDecoder())
Expand Down Expand Up @@ -155,7 +149,8 @@ private class UdsServer extends ChannelInboundHandlerAdapter {
private final AtomicBoolean anybodyConnected = new AtomicBoolean(false);

@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg)
throws Exception {
var buf = (ByteBuf) msg;
var bytes = new byte[buf.readableBytes()];
buf.readBytes(bytes);
Expand All @@ -165,7 +160,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
return;
}
var handler = (GlideMockProtobuf) messageProcessor;
Response response = null;
Response response;
if (!anybodyConnected.get()) {
var connection = ConnectionRequest.parseFrom(bytes);
response = handler.connection(connection);
Expand All @@ -180,7 +175,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
}

@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
cause.printStackTrace();
ctx.close();
failed.setPlain(true);
Expand Down
Loading