Add a `exchange()` variant to `RestTestClient` that expects 2xx response

[donalmurtagh]

When using RestClient it's possible to submit a request that's expected to succeed via

restClient.post()
    .uri("/foo/bar")
    .body(requestData)
    .retrieve();

With a RestTestClient the equivalent is

restTestClient.post()
    .uri("/foo/bar")
    .body(requestData)
    .exchange()
    .expectStatus().is2xxSuccessful();

Could we add a correspondingretrieve() method to the RestTestClient API? Not only would it make the code a bit more concise for the very common case of submitting a request that's expected to succeed, but it would simplify migrating from RestClient to RestTestClient

[rstoyanchev]

I get what you're asking but I don't think adding retrieve is a good fit. retrieve vs exchange is about working directly with the response vs using a workflow that also has to check the status because you're not working directly with the response.

The testing case doesn't fit into that. If we added retrieve it would be reasonable to expect that we also call retrieve on the internal RestClient, and therefore that configured status handlers would apply. However, we need to always call exchange to handle the response in a custom way for testing purposes.

What we could do is accept a Consumer<StatusAssertions> at build time that should be applied on every request:

RestTestClient.bindToController(..)
    .alwaysExpectStatus(statusAssertions -> ...)
    .build();

[donalmurtagh]

The testing case doesn't fit into that. If we added retrieve it would be reasonable to expect that we also call retrieve on the internal RestClient, and therefore that configured status handlers would apply. However, we need to always call exchange to handle the response in a custom way for testing purposes.

That's not how I would expect retrieve to be implemented. I would expect it to look like this:

interface RequestHeadersSpec<S extends RequestHeadersSpec<S>> {
    /////////////////////////////////////////////////////////////////
    // other methods of this interface omitted
    /////////////////////////////////////////////////////////////////

    /**
     * Perform the exchange without a request body.
     *
     * @return a spec for decoding the response
     */
    ResponseSpec exchange();

    /**
     * Perform the exchange with an expectation that it will succeed. An exception will be thrown
     * if a HTTP status code outside the 2xx range is returned.
     * 
     * @return a spec for decoding the response
     */
    default ResponseSpec retrieve() {
        return exchange().expectStatus().is2xxSuccessful();
    }
}

Perhaps the choice of a default interface method is not the best, but that's a side issue.

What we could do is accept a Consumer<StatusAssertions> at build time that should be applied on every request:

RestTestClient.bindToController(..)
.alwaysExpectStatus(statusAssertions -> ...)
.build();

I wouldn't find this useful at all. In my case I build a single RestTestClient that is shared throughout my test suite. Some tests expect success and some expect failure, so it doesn't make any sense (for me) to specify the expected outcome at build-time.

[rstoyanchev]

retrieve in RestClient works differently, and that can create different expectations from yours.

Essentially we're talking about a shortcut for exchange().expectStatus().is2xxSuccessful(). Given that a majority of tests are for successful responses, we could add an overload of exchange() called exchangeSuccessfully(). Longer than retrieve, but with code completion it's no different, and it's very clear what it does.

[donalmurtagh]

Essentially we're talking about a shortcut for exchange().expectStatus().is2xxSuccessful(). Given that a majority of tests are for successful responses, we could add an overload of exchange() called exchangeSuccessfully(). Longer than retrieve, but with code completion it's no different, and it's very clear what it does.

Sounds good to me. I agree that exchangeSuccessfully is a better name than retrieve.