Riptide Client Testing #301

lukasniemeier-zalando · 2017-11-24T09:11:47Z

Introducing @RiptideClientTest - a new spring-boot meta-annotation to support testing riptide-spring-boot-starter generated Riptide Http clients.

Currently testing is a bit tedious and repetitive. It can be shorter:

@Component
public class GatewayService {

    @Autowired
    @Qualifier("example")
    private Http http;

    void remoteCall() {
        http.get("/bar").dispatch(status(), on(OK).call(pass())).join();
    }
}

@RunWith(SpringRunner.class)
@RiptideClientTest(GatewayService.class)
public class RiptideTest {

    @Autowired
    private GatewayService client;

    @Autowired
    private MockRestServiceServer server;

    @Test
    public void shouldAutowireMockedHttp() throws Exception {
        server.expect(requestTo("https://example.com/bar")).andRespond(withSuccess());
        client.remoteCall()
        server.verify();
    }
}

@RiptideClientTest is based on @RestClientTest.

Currently @RiptideClientTest and its facilities are tied to riptide-spring-boot-starter. In order to allow usage with riptide-core only, it may make sense to extract the components to a dedicated artifact. Instead of modifying the RiptideRegistar directly, one would need to make sure that properly named AsyncClientHttpRequestFactory are registered beforehand (expose Registry#generateBeanName?). Added benefit is that we wouldn't need any provided testing dependencies anymore. On the other hand I am not sure whether anybody wants to / needs to use this without the starter...

lukasniemeier-zalando · 2017-11-24T09:12:01Z

...n/java/org/zalando/riptide/spring/testing/RiptideMockRestServiceServerAutoConfiguration.java

+    public MockRestServiceServer mockRestServiceServer(final RequestExpectationManager manager) throws Exception {
+        final Constructor<MockRestServiceServer> ctr = MockRestServiceServer.class.getDeclaredConstructor(RequestExpectationManager.class);
+        ctr.setAccessible(true);
+        return ctr.newInstance(manager);


Hello Darkness My Old Friend

whiskeysierra · 2017-11-24T09:18:21Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/RiptideSettings.java

@@ -23,6 +23,11 @@
    private Defaults defaults = new Defaults();
    private GlobalOAuth oauth = new GlobalOAuth();
    private Map<String, Client> clients = new LinkedHashMap<>();
+    private Boolean mocked = false;
+
+    public void setMocked(final Boolean mocked) {


You should get this for free.

Top Level of the Settings currently only has setters generated.

You mean getters, right? Isn't the constructor enough?

I did not get it working without this setter somehow. He does not find a proper "writable" property path.

whiskeysierra · 2017-11-24T09:18:30Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/RiptideSettings.java

@@ -23,6 +23,11 @@
    private Defaults defaults = new Defaults();
    private GlobalOAuth oauth = new GlobalOAuth();
    private Map<String, Client> clients = new LinkedHashMap<>();
+    private Boolean mocked = false;


Why not boolean?

Putting this here technically allows users to do this:

riptide: mocked: true

We can unbox - no particular reason to use Boolean

whiskeysierra · 2017-11-24T09:20:08Z

...-spring-boot-starter/src/main/java/org/zalando/riptide/spring/testing/RiptideClientTest.java

+     *
+     * @return the components to test
+     */
+    @AliasFor(annotation = RestClientTest.class, attribute = "value")


Any other attributes that are worth having as an alias?

I haven't used any other on @RestClientTest, but we may decided to just forward all of it?

whiskeysierra · 2017-11-24T09:20:15Z

...-spring-boot-starter/src/main/java/org/zalando/riptide/spring/testing/RiptideClientTest.java

+@RestClientTest
+@AutoConfigureMockRestServiceServer(enabled = false) // see RiptideClientAutoConfiguration
+@TestPropertySource(properties = {
+        "riptide.mocked: true"


There it is.

Also referencing your comment above: any other method to inject the "mock mode" is obviously fine - this has been just the fastest way 😁

whiskeysierra · 2017-11-24T09:21:53Z

...ot-starter/src/main/java/org/zalando/riptide/spring/testing/ExpectingHttpRequestFactory.java

+import java.io.IOException;
+import java.net.URI;
+
+public class ExpectingHttpRequestFactory implements AsyncClientHttpRequestFactory, ClientHttpRequestFactory {


Can't we use the existing implementation?

I think it is private. Yeah I know I am already using the private constructor of the server, but this guy here is 10 straight forward lines of code

whiskeysierra · 2017-11-24T09:24:34Z

Summary of this PR contains RiptideGatewayTest rather than RiptideClientTest.

whiskeysierra · 2017-11-24T09:33:03Z

Instead of modifying the RiptideRegistar directly, one would need to make sure that properly named AsyncClientHttpRequestFactory are registered beforehand (expose Registry#generateBeanName?). Added benefit is that we wouldn't need any provided testing dependencies anymore. On the other hand I am not sure whether anybody wants to / needs to use this without the starter...

I think that would be cleaner, even without the dedicated artifact. Leaving the Registrar purely for production would be easier to understand for future changes.

jhorstmann · 2017-11-24T10:18:20Z

Are OAuth access tokens also mocked? Would be nice to avoid any external calls in the tests.

whiskeysierra · 2017-11-24T10:23:34Z

Are OAuth access tokens also mocked? Would be nice to avoid any external calls in the tests.

Not explicitly. The AccessTokens instance will try to generate tokens, but they will never be used, since the interceptor that would inject them is not being used (it's for the real Apache HTTP client).

whiskeysierra · 2017-11-24T10:24:27Z

The test setup could just register a AccessTokens bean that does nothing.

…Riptide clients

lukasniemeier-zalando · 2017-11-27T19:17:10Z

I've just pushed an alternative implementation that relies on the original trick from the README and does not use a property to modify the behavior of the RiptideRegistar.

The example above changes slightly, as there have to be one mocked server per client:

@Component
public class GatewayService {

    @Autowired
    @Qualifier("example")
    private Http http;

    void remoteCall() {
        http.get("/bar").dispatch(status(), on(OK).call(pass())).join();
    }
}

@RunWith(SpringRunner.class)
@RiptideClientTest(GatewayService.class)
public class RiptideTest {

    @Autowired
    private GatewayService client;

    @Autowired
    @Qualifier("example") // this is now needed
    private MockRestServiceServer server;

    @Test
    public void shouldAutowireMockedHttp() throws Exception {
        server.expect(requestTo("https://example.com/bar")).andRespond(withSuccess());
        client.remoteCall()
        server.verify();
    }
}

One important (?) thing to note is that the RestTemplate won't be redirected to the mock request factory. In order to do so we'd have to register another dedicated MockRestServiceServer - I guess this is not worth it as then we'd have multiple MockRestServiceServer beans with the same client qualifier.

lukasniemeier-zalando · 2017-11-27T19:21:54Z

The test setup could just register a AccessTokens bean that does nothing.

👍

lukasniemeier-zalando · 2017-11-27T19:39:44Z

The test setup could just register a AccessTokens bean that does nothing.

👍

Actually this is not needed. The AccessTokens integration works via interceptors of the HttpClient of the request factory. And the latter we mock.

whiskeysierra · 2017-11-27T21:06:44Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/TestRiptideRegistrar.java

+    }
+
+    private String registerAsyncRestTemplate(final String id) {
+        return registry.registerIfAbsent(id, AsyncRestTemplate.class, () -> {


That will incorrectly replace the AsyncRestTemplate that we would register in the DefaultRegistrar. It would miss plugins as well as the base url.

whiskeysierra · 2017-11-27T21:07:12Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/TestRiptideRegistrar.java

+        });
+    }
+
+    private String registerMockRestServiceServer(final String id, final String templateId) {


Why don't we register this once as a singleton?

... and share it across clients?

Hm I remember that we talked about having this as a singleton vs. per-client and somehow I was under the impression that I gave in to have it per-client 😬 I believe the singleton route is more intuitive either way!

whiskeysierra · 2017-11-28T08:09:10Z

riptide-spring-boot-starter/src/test/java/org/zalando/riptide/spring/testing/TestService.java

+    @Autowired
+    public TestService(@Qualifier("example") final Http http, @Qualifier("example") final RestTemplate restTemplate) {
+        this.http = http;
+        this.restTemplate = restTemplate;


whiskeysierra · 2017-11-28T08:10:14Z

Actually this is not needed. The AccessTokens integration works via interceptors of the HttpClient of the request factory. And the latter we mock.

Access tokens are never used, but the default implementation will spawn a thread and poll tokens in the background. That could be disabled.

whiskeysierra · 2017-11-28T08:12:50Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/RiptideClientTest.java

+@Documented
+@Inherited
+@RestClientTest
+@AutoConfigureMockRestServiceServer(enabled = false) // will be registered per client


Comment is out-dated. Can you describe here why we need this?

Ups will do

whiskeysierra · 2017-11-28T08:17:02Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/TestRiptideRegistrar.java

+        settings.getClients().forEach((id, client) -> registerAsyncClientHttpRequestFactory(id, templateId, serverId));
+    }
+
+    private String registerMockAsyncRestTemplate() {


Couldn't you register the template and the server using traditional @Bean methods in the RiptideTestAutoConfiguration?

Technically yes, but I haven't found a cool way to still reference these beans in here on registering the request factories. Just by some explicit bean name? Any idea? I feel those 2 + (1 * n) beans should go together.

Explicit bean name should work. I'd argue that the configuration binds those 2+n beans together. The Registrar is just our way to dynamically register the n beans.

whiskeysierra · 2017-11-28T08:59:37Z

...ring-boot-starter/src/main/java/org/zalando/riptide/spring/RiptideTestAutoConfiguration.java

+        }
+
+        @Bean(name = SERVER_BEAN_NAME)
+        MockRestServiceServer mockRestServiceServer(@Qualifier("_mockAsyncRestTemplate") final AsyncRestTemplate template) {


The qualifier could use the TEMPLATE_BEAN_NAME.

Ups forgot to replace

whiskeysierra · 2017-11-28T09:00:12Z

riptide-spring-boot-starter/src/main/java/org/zalando/riptide/spring/TestRiptideRegistrar.java

+    private final Registry registry;
+    private final RiptideSettings settings;
+
+    private final String asyncRestTemplateName;


I'd be fine with accessing the same constant.

whiskeysierra · 2017-11-28T14:04:45Z

👍

lukasniemeier-zalando · 2017-11-28T14:30:32Z

In a follow-up I will add a paragraph to the READMEs

RohanNg · 2019-06-17T12:56:28Z

Hi,

We had a use case where in remoteCall multiple requests are dispatched to a http endpoint. We have a problem writing test because the ordering of the http call cannot be relaxed, while it is desirable to ignore the ordering.

This could be probably be done by MockRestServiceServer.bindTo(restTemplate).ignoreExpectOrder(true).build(). However we don't know how to get the produced server instance into use.

Could you provide some hint how to do that?

whiskeysierra · 2019-06-17T13:58:20Z

No easy way to do that right now. Options that I see:

Use reflection to manipulate the expectationManager and replace it with an UnorderedRequestExpectationManager
Open a pull request and introduce an option (property?) to change the behavior of this line:

riptide/riptide-spring-boot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/RiptideTestAutoConfiguration.java

Line 39 in 0f53529

return createServer(restTemplate);
Change the line mentioned above, introduce a @ConditionalOnBean(MockRestServiceServer.class) and then override it in your auto configuration. Properly the cleanest solution: ✔️

lukasniemeier-zalando requested a review from whiskeysierra November 24, 2017 09:11

lukasniemeier-zalando commented Nov 24, 2017

View reviewed changes

lukasniemeier-zalando changed the title ~~Introduce @RiptideClientTest to support testing of starter generated …~~ Riptide Client Testing Nov 24, 2017

whiskeysierra reviewed Nov 24, 2017

View reviewed changes

Introduce @RiptideClientTest to support testing of starter generated …

f4cdb44

…Riptide clients

lukasniemeier-zalando force-pushed the gateway-testing branch from d494498 to d85f8b4 Compare November 27, 2017 19:13

Alternative implementation

780bf90

lukasniemeier-zalando force-pushed the gateway-testing branch from d85f8b4 to 780bf90 Compare November 27, 2017 19:24

whiskeysierra reviewed Nov 27, 2017

View reviewed changes

Single instance of MockRestServiceServer

ee3e97e

whiskeysierra reviewed Nov 28, 2017

View reviewed changes

lukasniemeier-zalando added 2 commits November 28, 2017 09:41

Remove unused code.

478b1d3

Register Template and Server via regular Configuration bean definition.

763a58d

whiskeysierra reviewed Nov 28, 2017

View reviewed changes

Use static references instead of injection

4177bf3

whiskeysierra merged commit aca9f62 into master Nov 28, 2017

whiskeysierra deleted the gateway-testing branch November 28, 2017 14:05

lukasniemeier-zalando mentioned this pull request Jan 5, 2018

Document Testing via @RiptideClientTest #317

Closed

Riptide Client Testing #301

Riptide Client Testing #301

Conversation

lukasniemeier-zalando commented Nov 24, 2017 • edited

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

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

whiskeysierra commented Nov 24, 2017

whiskeysierra commented Nov 24, 2017

jhorstmann commented Nov 24, 2017

whiskeysierra commented Nov 24, 2017

whiskeysierra commented Nov 24, 2017

lukasniemeier-zalando commented Nov 27, 2017

lukasniemeier-zalando commented Nov 27, 2017

lukasniemeier-zalando commented Nov 27, 2017

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

whiskeysierra commented Nov 28, 2017

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

Choose a reason for hiding this comment

whiskeysierra commented Nov 28, 2017

lukasniemeier-zalando commented Nov 28, 2017

RohanNg commented Jun 17, 2019 • edited

whiskeysierra commented Jun 17, 2019

lukasniemeier-zalando commented Nov 24, 2017 •

edited

RohanNg commented Jun 17, 2019 •

edited