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.

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

Add WebTestClient and TestHttpResponse to enable more fluent testing of HTTP responses #4740

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

tomatophobia
Copy link
Contributor

@tomatophobia tomatophobia commented Mar 10, 2023

Motivation:

  • Related issue: Add WebTestClient  #4339
  • In the existing code that tests HTTP responses, there was some redundant code present.
  • It would be helpful to have a method that enables more fluent testing.

Modifications:

  • Add TestHttpResponse, which is similar to AggregatedHttpResponse, and includes additional assertion methods.
    • One of the assertion methods in TestHttpResponse, assertStatus, allows you to assert the status of the response and returns the TestHttpResponse object, enabling you to test other aspects of the response more fluently.
  • Add WebTestClient that returns TestHttpResponse by default

Results:

  • Closes Add WebTestClient  #4339
  • Users can now use multiple assertion methods to test responses more fluently.
  • The code below contrasts testing a REST API using WebClient versus testing it using WebTestClient.
@RegisterExtension
static ServerExtension server = ...;

@Test
void usingWebClient() {
    final WebClient client = server.webClient();
    AggregatedHttpResponse res;

    res = client.get("/rest/1").aggregate().join();
    assertThat(res.status()).isEqualTo(HttpStatus.OK);
    assertThat(res.headers().contains(...)).isTrue();
    assertThat(res.content().toStringUtf8()).isEqualTo(...);
    assertThat(res.trailers().isEmpty()).isTrue();
}

@Test
void usingWebTestClient() {
    final WebTestClient client = server.webTestClient();

    client.get("/rest/1")
          .assertStatus().isOk()
          .assertHeaders().contains(...)
          .assertContent().stringUtf8IsEqualTo(...)
          .assertTrailers().isEmpty();
}

@tomatophobia tomatophobia changed the title Add WebTestClient and TestHttpResponse Add WebTestClient and TestHttpResponse to enable more fluent testing of HTTP responses Mar 10, 2023
@minwoox minwoox added this to the 1.24.0 milestone Mar 23, 2023
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

I didn't have time to fully review this PR. I will see in detail soon.

@ikhoon ikhoon modified the milestones: 1.24.0, 1.25.0 Jun 2, 2023
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.
The overall direction looks good although we need minor changes and polish Javadoc.

@ikhoon
Copy link
Contributor

ikhoon commented Jul 12, 2023

I suggest you apply the WebTestClient for some existing code to see how well the current design covers our existing tests.

As you know, we usually use AssertJ. Maybe in the follow-up PR, we can add armeria-assertj module that our team mostly uses for Armeria testing.

var testClient = AssertjWebTestClient.of(server.webClient());

@tomatophobia
Copy link
Contributor Author

tomatophobia commented Jul 12, 2023

I suggest you apply the WebTestClient for some existing code to see how well the current design covers our existing tests.

As you know, we usually use AssertJ. Maybe in the follow-up PR, we can add armeria-assertj module that our team mostly uses for Armeria testing.

var testClient = AssertjWebTestClient.of(server.webClient());

(I'm sorry, but I couldn't leave a reply due to a bug on GitHub, so I left a comment instead.)

I completely agree with your opinion. I will contribute as a follow-up to this PR. 😄 👍

In fact, initially I wrote the code using AssertJ according to the developer guide, but due to dependency issues, I ended up using the JUnit API.

I have a question at this point. junit5 module of armeria depends on libs.junit5.jupiter.api but it doesn't depend on libs.assertj. Is it strange to add the dependency on libs.assertj to the junit5 module in the current situation? Do you prefer creating a new armeria-assertj module as you mentioned in the comment?

(related #4740 (comment))

Copy link
Contributor Author

@tomatophobia tomatophobia left a comment

Choose a reason for hiding this comment

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

The newly added exception test API is as follows.

Comment on lines 108 to 121
if (this == obj) {
return true;
}

if (!(obj instanceof TestHttpResponse)) {
return false;
}

final TestHttpResponse that = (TestHttpResponse) obj;

return informationals().equals(that.informationals()) &&
headers().equals(that.headers()) &&
content().equals(that.content()) &&
trailers().equals(that.trailers());
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if (this == obj) {
return true;
}
if (!(obj instanceof TestHttpResponse)) {
return false;
}
final TestHttpResponse that = (TestHttpResponse) obj;
return informationals().equals(that.informationals()) &&
headers().equals(that.headers()) &&
content().equals(that.content()) &&
trailers().equals(that.trailers());
if (this == obj) {
return true;
}
if (!(obj instanceof TestHttpResponse)) {
return false;
}
final TestHttpResponse that = (TestHttpResponse) obj;
return delegate.equals(that.unwrap());

Before using delegate.equals(), I first cast obj to TestHttpResponse, and then unwrap() it before using it as an argument. I made this change because when using obj without casting it to TestHttpResponse, the result was always false if the runtime type of obj was TestHttpResponse.

@ikhoon
Copy link
Contributor

ikhoon commented Jul 27, 2023

This PR seems ready to review by other maintainers. Let's get some opinions about the current direction. PTAL

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Attention: Patch coverage is 30.23810% with 293 lines in your changes are missing coverage. Please review.

Project coverage is 73.82%. Comparing base (b8eb810) to head (b97719a).
Report is 165 commits behind head on main.

❗ Current head b97719a differs from pull request most recent head 36daaa1. Consider uploading reports for the commit 36daaa1 to get more accurate results

Files Patch % Lines
...a/testing/junit5/client/TestBlockingWebClient.java 8.64% 73 Missing and 1 partial ⚠️
...lient/TestBlockingWebClientRequestPreparation.java 26.43% 64 Missing ⚠️
...ng/junit5/client/TestBlockingWebClientBuilder.java 0.00% 39 Missing ⚠️
...meria/testing/junit5/client/HttpHeadersAssert.java 26.53% 36 Missing ⚠️
.../armeria/testing/junit5/client/HttpDataAssert.java 30.76% 18 Missing ⚠️
...rmeria/testing/junit5/client/HttpStatusAssert.java 25.00% 18 Missing ⚠️
...armeria/testing/junit5/client/ThrowableAssert.java 69.23% 11 Missing and 1 partial ⚠️
...testing/junit5/client/DefaultTestHttpResponse.java 56.52% 10 Missing ⚠️
...testing/junit5/client/AbortedTestHttpResponse.java 40.00% 9 Missing ⚠️
...ng/junit5/client/DefaultTestBlockingWebClient.java 57.89% 8 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4740      +/-   ##
============================================
- Coverage     73.95%   73.82%   -0.14%     
- Complexity    20115    20917     +802     
============================================
  Files          1730     1819      +89     
  Lines         74161    77163    +3002     
  Branches       9465     9794     +329     
============================================
+ Hits          54847    56963    +2116     
- Misses        14837    15569     +732     
- Partials       4477     4631     +154     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ikhoon ikhoon modified the milestones: 1.25.0, 1.26.0 Jul 30, 2023
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Wow that's a lot of code 😅 Looks good overall 👍

I'm wondering if it's worth just creating an assertj module at this point. What do you think others?

Nevermind this comment 😅 Talked with the team and we decided not to move this to a separate module for this iteration

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks good overall, left some minor comments

TestHttpResponse response() {
return response;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of adding a base assertion which accepts a vararg of consumers?

for (val c: consumers) {
  assertAll(() -> consumer.accept(actual()));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I'm sorry, but I can't understand the intent behind the suggestion. Could you please clarify in which situations this code is being used?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of something like the following so that users could fall back in case they can't find an assertion that is sufficient for their use-case.

@SafeVarargs
public final TestHttpResponse satisfies(ThrowableConsumer<T>... consumers) {
    for (ThrowableConsumer<T> consumer: consumers) {
        assertAll(() -> consumer.accept(actual));
    }
    return response();
}

/**
* Asserts that the {@link String} representation of actual {@link HttpData} contains the given value.
*/
public TestHttpResponse stringContains(Charset charset, String expected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind also formatting the other methods so that the verb comes first? (following how junit, assertj are naming assertions)

Suggested change
public TestHttpResponse stringContains(Charset charset, String expected) {
public TestHttpResponse containsString(Charset charset, String expected) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! When I named the stringContains method, my intention was not to convey "the HTTP content contains the parameter string" but rather "when representing HTTP content as a string, it contains the provided string as an argument." If I were to name it more explicitly, it might be something like toStringAndItWillContain. The background behind choosing such names was due to the ability to represent HTTP content in various ways, such as UTF-8, ASCII, and others. Initially, I thought of stringUtf8Contains, and then I came up with stringContains.

As you mentioned, it does seem awkward. How about remove the stringUtf8Contains and stringAsciiContains methods and simply use shorter method namescontains(String) and contains(Charset charset, String expected)?

Copy link
Contributor

Choose a reason for hiding this comment

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

my intention was not to convey "the HTTP content contains the parameter string" but rather "when representing HTTP content as a string, it contains the provided string as an argument."

I see, ideally I think we would as a StringAssert class then and chain like the following:

assertContent().asString().isEqualTo(...);

I think this can be done separately though - what do you think of focusing on byte equality since HttpData is essentially a byte array?

Users probably still find this useful - i.e.

assertContent().isEqualTo("".getBytes());

@minwoox minwoox modified the milestones: 1.26.0, 1.27.0 Oct 11, 2023
@ikhoon ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Sorry it took a while - did another round. I think this PR is almost done, let me know if anything doesn't make sense 🙇

* Use the factory methods in {@link TestBlockingWebClient} if you do not have many options to override.
* Please refer to {@link ClientBuilder} for how decorators and HTTP headers are configured
*/
public final class TestBlockingWebClientBuilder extends AbstractWebClientBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question) I'm not sure how useful this class is given that we can always use TestBlockingWebClient#of(WebClient).

What do you think of removing this class for now and adding it later if there is demand for such a fluent builder?

* }
* }</pre>
*/
public interface TestBlockingWebClient extends ClientBuilderParams, Unwrappable {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of modifying s.t. TestBlockingWebClient extends BlockingWebClient instead?

I think we could achieve this by:

  • Rename BlockingWebClientRequestPreparation to DefaultBlockingWebClientRequestPreparation and make BlockingWebClientRequestPreparation an interface
  • Make TestBlockingWebClient extend BlockingWebClient
  • Make TestHttpResponse extend AggregatedHttpResponse

We should be able to maintain these APIs more easily this way

@@ -370,6 +371,24 @@ public RestClient restClient(Consumer<WebClientBuilder> webClientCustomizer) {
return delegate.restClient(webClientCustomizer);
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Question) Instead of creating new methods, could we just return TestBlockingWebClient from ServerExtension#blockingWebClient directly?

TestHttpResponse response() {
return response;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of something like the following so that users could fall back in case they can't find an assertion that is sufficient for their use-case.

@SafeVarargs
public final TestHttpResponse satisfies(ThrowableConsumer<T>... consumers) {
    for (ThrowableConsumer<T> consumer: consumers) {
        assertAll(() -> consumer.accept(actual));
    }
    return response();
}

/**
* Asserts that the {@link String} representation of actual {@link HttpData} contains the given value.
*/
public TestHttpResponse stringContains(Charset charset, String expected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

my intention was not to convey "the HTTP content contains the parameter string" but rather "when representing HTTP content as a string, it contains the provided string as an argument."

I see, ideally I think we would as a StringAssert class then and chain like the following:

assertContent().asString().isEqualTo(...);

I think this can be done separately though - what do you think of focusing on byte equality since HttpData is essentially a byte array?

Users probably still find this useful - i.e.

assertContent().isEqualTo("".getBytes());

* Asserts that the actual {@link HttpHeaders} contains the given name and {@link String} value.
* The {@code name} and the {@code value} cannot be null.
*/
public TestHttpResponse contains(CharSequence name, String value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: What do you think of

  • Removing the other contains* variants since they don't seem to be providing much value - the object is string-ified internally anyways
  • Accepting Object for the general case
Suggested change
public TestHttpResponse contains(CharSequence name, String value) {
public TestHttpResponse contains(CharSequence name, Object value) {

private final TestHttpResponse response;

AbstractResponseAssert(T actual, TestHttpResponse response) {
requireNonNull(actual, "actual");
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Although I think it may be useful to let users check nullability via assertions, I don't think it's necessary at this stage. I'd like to focus on getting this PR merged first.

* Asserts that the message of the actual {@link Throwable} starts with the given message.
* The {@code message} cannot be null.
*/
public ThrowableAssert hasMessageStartingWith(String message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of removing hasMessageStartingWith|hasMessageNotContaining|hasMessageEndingWith assertions?

I imagine adding a StringAssert later on

e.g.

assertCause().message().startsWith()

}

@Override
public ThrowableAssert assertCause() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Matching the class name

Suggested change
public ThrowableAssert assertCause() {
public ThrowableAssert assertThrowable() {

private final Throwable cause;

ThrowableAssert(Throwable cause) {
requireNonNull(cause, "cause");
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I think the nullibility of this class can be relaxed, but I'd rather we not do this in this already large PR

@jrhee17 jrhee17 modified the milestones: 1.28.0, 1.29.0 Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add WebTestClient
4 participants