bvolpato commented on code in PR #29262: URL: https://github.com/apache/beam/pull/29262#discussion_r1382423428
########## sdks/java/io/rrio/src/test/java/org/apache/beam/io/requestresponse/EchoHTTPCallerTestIT.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.io.requestresponse; + +import static org.apache.beam.sdk.io.common.IOITHelper.readIOTestPipelineOptions; +import static org.apache.beam.sdk.util.Preconditions.checkStateNotNull; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import com.google.protobuf.ByteString; +import java.net.URI; +import org.apache.beam.testinfra.mockapis.echo.v1.Echo.EchoRequest; +import org.apache.beam.testinfra.mockapis.echo.v1.Echo.EchoResponse; +import org.apache.beam.testinfra.mockapis.echo.v1.EchoServiceGrpc; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; +import org.checkerframework.checker.nullness.qual.NonNull; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Tests for {@link EchoHTTPCallerTestIT} on a deployed {@link EchoServiceGrpc} instance's HTTP + * handler. See {@link EchoITOptions} for details on the required parameters and how to provide + * these for running integration tests. + */ +@RunWith(JUnit4.class) +public class EchoHTTPCallerTestIT { + + private static @MonotonicNonNull EchoITOptions options; + private static @MonotonicNonNull EchoHTTPCaller client; + private static final ByteString PAYLOAD = ByteString.copyFromUtf8("payload"); + + @BeforeClass + public static void setUp() throws UserCodeExecutionException { + options = readIOTestPipelineOptions(EchoITOptions.class); + if (options.getHttpEndpointAddress().isEmpty()) { + throw new RuntimeException( + "--httpEndpointAddress is missing. See " + EchoITOptions.class + "for details."); + } + client = EchoHTTPCaller.of(URI.create(options.getHttpEndpointAddress())); + + EchoRequest request = createShouldExceedQuotaRequest(); + + // Quota allocation needs at least 1 so we invoke a few times to guarantee exceeding quota. + // We only throw exceptions that are not UserCodeQuotaException. + try { + EchoResponse ignored = client.call(request); + client.call(request); + client.call(request); + client.call(request); + } catch (UserCodeExecutionException ignored) { + } + } + + @Test + public void givenValidRequest_receivesResponse() throws UserCodeExecutionException { + EchoRequest request = createShouldNeverExceedQuotaRequest(); + EchoResponse response = client.call(request); + assertEquals(response.getId(), request.getId()); + assertEquals(response.getPayload(), request.getPayload()); + } + + @Test + public void givenExceededQuota_shouldThrow() { + assertThrows(UserCodeQuotaException.class, () -> client.call(createShouldExceedQuotaRequest())); + } + + @Test + public void givenNotFound_shouldThrow() { + UserCodeExecutionException error = + assertThrows( + UserCodeExecutionException.class, + () -> + client.call( + EchoRequest.newBuilder() + .setId("i-dont-exist-quota-id") + .setPayload(PAYLOAD) + .build())); + + assertTrue(error.getMessage().contains("404 Not Found")); Review Comment: Just curious if we have/can use something like Mockito? We could use more idiomatic tests, for example ``` assertThat(error.getMessage(), containsString("404 Not Found")); ``` It looks like the test is the same, but failure reports would be (just an example): > String is 429 Too Many Requests, expected to contain 404 Not Found Instead of: > Expected true is false ########## sdks/java/io/rrio/src/test/java/org/apache/beam/io/requestresponse/EchoHTTPCallerTestIT.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.io.requestresponse; + +import static org.apache.beam.sdk.io.common.IOITHelper.readIOTestPipelineOptions; +import static org.apache.beam.sdk.util.Preconditions.checkStateNotNull; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import com.google.protobuf.ByteString; +import java.net.URI; +import org.apache.beam.testinfra.mockapis.echo.v1.Echo.EchoRequest; +import org.apache.beam.testinfra.mockapis.echo.v1.Echo.EchoResponse; +import org.apache.beam.testinfra.mockapis.echo.v1.EchoServiceGrpc; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; +import org.checkerframework.checker.nullness.qual.NonNull; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Tests for {@link EchoHTTPCallerTestIT} on a deployed {@link EchoServiceGrpc} instance's HTTP + * handler. See {@link EchoITOptions} for details on the required parameters and how to provide + * these for running integration tests. + */ +@RunWith(JUnit4.class) +public class EchoHTTPCallerTestIT { + + private static @MonotonicNonNull EchoITOptions options; + private static @MonotonicNonNull EchoHTTPCaller client; + private static final ByteString PAYLOAD = ByteString.copyFromUtf8("payload"); + + @BeforeClass + public static void setUp() throws UserCodeExecutionException { + options = readIOTestPipelineOptions(EchoITOptions.class); + if (options.getHttpEndpointAddress().isEmpty()) { + throw new RuntimeException( + "--httpEndpointAddress is missing. See " + EchoITOptions.class + "for details."); + } + client = EchoHTTPCaller.of(URI.create(options.getHttpEndpointAddress())); + + EchoRequest request = createShouldExceedQuotaRequest(); + + // Quota allocation needs at least 1 so we invoke a few times to guarantee exceeding quota. + // We only throw exceptions that are not UserCodeQuotaException. + try { + EchoResponse ignored = client.call(request); + client.call(request); + client.call(request); + client.call(request); + } catch (UserCodeExecutionException ignored) { + } Review Comment: It feels like it's part of some initialization, but feels like we're testing some behavior here. Let's make sure that this is covered (with assertions) somewhere else ########## sdks/java/io/rrio/build.gradle: ########## @@ -34,15 +38,18 @@ dependencies { testImplementation project(path: ":sdks:java:core", configuration: "shadowTest") testImplementation project(path: ":sdks:java:io:common", configuration: "testRuntimeMigration") + testImplementation project(path: ":beam-test-infra-mock-apis") + // Vendered grpc library not fully compatible with proto autogenerated code Review Comment: nit typo Vendored ########## sdks/java/io/rrio/src/test/java/org/apache/beam/io/requestresponse/EchoITOptions.java: ########## @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.io.requestresponse; + +import org.apache.beam.sdk.options.Default; +import org.apache.beam.sdk.options.Description; +import org.apache.beam.sdk.options.PipelineOptions; +import org.apache.beam.testinfra.mockapis.echo.v1.EchoServiceGrpc; + +/** + * Shared options for running integration tests on a deployed {@link EchoServiceGrpc}. See <a + * href="https://github.com/apache/beam/tree/master/.test-infra/mock-apis#integration";>https://github.com/apache/beam/tree/master/.test-infra/mock-apis#integration</a> + * for details on how to acquire values required by {@link EchoITOptions}. + * + * <p>To provide these values to your integration tests: + * + * <pre> + * ./gradlew :sdks:java:io:rrio:integrationTest -DintegrationTestPipelineOptions='[ + * "--grpcEndpointAddress=", + * "--httpEndpointAddress=" + * ]' + * </pre> + */ +public interface EchoITOptions extends PipelineOptions { + @Description("The gRPC address of the Echo API endpoint, typically of the form <host>:<port>.") + String getgRPCEndpointAddress(); Review Comment: Can you test this explicitly to make sure this works as intended? It feels to me that it should be `getGRPCEndpointAddress` to follow Java conventions, but I am not sure if the PipelineOptions is case sensitive. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
