Extend httpResponseMessage.Should().HaveStatusCode() to dump request body on failure

[bart-degreed]

After updating to the latest FA, I found that our custom extension method conflicts with the one added in v6.4.0 here.

The difference is that ours dumps the request body on HTTP status code mismatch, which is helpful in analyzing what's going on without debugging the test.

For an assertion like:

httpResponse.Should().HaveStatusCode(HttpStatusCode.OK);

when that returns 404, we get the following test output:

Xunit.Sdk.XunitException
Expected the enum to be HttpStatusCode.OK {value: 200} because response body returned was:
{
  "links": {
    "self": "http://localhost/workItems/2147483647"
  },
  "errors": [
    {
      "id": "23e70a0e-23dd-4c18-9fde-62a45eff9e39",
      "status": "404",
      "title": "The requested resource does not exist.",
      "detail": "Resource of type 'workItems' with ID '2147483647' does not exist.",
      "meta": {
        "stackTrace": [
          "JsonApiDotNetCore.Errors.ResourceNotFoundException: Exception of type 'JsonApiDotNetCore.Errors.ResourceNotFoundException' was thrown.",
          "   at void JsonApiDotNetCore.Services.JsonApiResourceService<TResource, TId>.AssertPrimaryResourceExists(TResource resource) in C:/Source/Repos/JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs:line 521",
          "   at async Task<TResource> JsonApiDotNetCore.Services.JsonApiResourceService<TResource, TId>.GetPrimaryResourceByIdAsync(TId id, TopFieldSelection fieldSelection, CancellationToken cancellationToken) in C:/Source/Repos/JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs:line 488",
          "   at async Task JsonApiDotNetCore.Services.JsonApiResourceService<TResource, TId>.DeleteAsync(TId id, CancellationToken cancellationToken) in C:/Source/Repos/JsonApiDotNetCore/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs:line 454",
          "   at async Task<IActionResult> JsonApiDotNetCore.Controllers.BaseJsonApiController<TResource, TId>.DeleteAsync(TId id, CancellationToken cancellationToken) in C:/Source/Repos/JsonApiDotNetCore/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs:line 352",
          "   at async Task<IActionResult> JsonApiDotNetCore.Controllers.JsonApiController<TResource, TId>.DeleteAsync(TId id, CancellationToken cancellationToken) in C:/Source/Repos/JsonApiDotNetCore/src/JsonApiDotNetCore/Controllers/JsonApiController.cs:line 107",
          "   at async ValueTask<IActionResult> Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor+TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, object controller, object[] arguments)",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeActionMethodAsync()+Awaited(?)",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()+Awaited(?)",
          "   at void Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)",
          "   at Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()+Awaited(?)",
          "   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextExceptionFilterAsync()+Awaited(?)"
        ]
      }
    }
  ]
}, but found HttpStatusCode.NotFound {value: 404}.
   at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args)
   at FluentAssertions.Primitives.EnumAssertions`2.Be(TEnum expected, String because, Object[] becauseArgs)
   at TestBuildingBlocks.HttpResponseMessageExtensions.HttpResponseMessageAssertions.HaveStatusCode(HttpStatusCode statusCode) in C:\Source\Repos\JsonApiDotNetCore\test\TestBuildingBlocks\HttpResponseMessageExtensions.cs:line 32
   at JsonApiDotNetCoreTests.IntegrationTests.ReadWrite.Deleting.DeleteResourceTests.Cannot_delete_unknown_resource() in C:\Source\Repos\JsonApiDotNetCore\test\JsonApiDotNetCoreTests\IntegrationTests\ReadWrite\Deleting\DeleteResourceTests.cs:line 66
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 264
--- End of stack trace from previous location ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:line 90

I know the message is a bit messy, but it contains all the info we need: the actual status code, the expected status code, and the returned response body.

Our extension method code is the following, feel free to reuse parts as it makes sense.

using System.Net;
using FluentAssertions;
using FluentAssertions.Primitives;
using JetBrains.Annotations;

namespace TestBuildingBlocks;

[PublicAPI]
public static class HttpResponseMessageExtensions
{
    public static HttpResponseMessageAssertions Should(this HttpResponseMessage instance)
    {
        return new HttpResponseMessageAssertions(instance);
    }

    public sealed class HttpResponseMessageAssertions : ReferenceTypeAssertions<HttpResponseMessage, HttpResponseMessageAssertions>
    {
        protected override string Identifier => "response";

        public HttpResponseMessageAssertions(HttpResponseMessage subject)
            : base(subject)
        {
        }

        // ReSharper disable once UnusedMethodReturnValue.Global
        [CustomAssertion]
        public AndConstraint<HttpResponseMessageAssertions> HaveStatusCode(HttpStatusCode statusCode)
        {
            if (Subject.StatusCode != statusCode)
            {
                string responseText = Subject.Content.ReadAsStringAsync().Result;
                Subject.StatusCode.Should().Be(statusCode, string.IsNullOrEmpty(responseText) ? null : $"response body returned was:\n{responseText}");
            }

            return new AndConstraint<HttpResponseMessageAssertions>(this);
        }
    }
}

[bart-degreed]

/cc @mu88

[mu88]

Thank you for your input, @bart-degreed !

I understand your preference for providing the request body as well. From the perspective of a multi-purpose library like Fluent Assertions, I would ask: why the request body and not all the headers as well? So it is more a product decision :)

Let's bring @dennisdoomen and @jnyrup into the discussion.

[dennisdoomen]

I think it's a great idea. We would welcome a PR for that.

[jnyrup]

While the output looks good, introducing a blocking call would go against our statement of "fully embrace asynchronous code".

One of them is the way we supported async code. In the past, we would invoke asynchronous code by wrapping it in a synchronously blocking call. Unfortunately this resulted in occasional deadlocks, which we fixed by temporarily clearing the SynchronizationContext. This worked but felt like an ugly workaround. So in v6, we decided to fully embrace asynchronous code and make some of the assertion APIs async itself. This means that using Should().Throw() will no longer magically work on async code and you need to use ThrowAsync() instead.

https://www.continuousimprover.com/2021/08/fluent-assertions-v6.html

[mu88]

Regarding the output format, I'd prefer if the actual and expected status code are close together not separated by dozens of lines in case the request body is large. That somehow breaks the beauty and simplicity of Fluent Assertions.

[dennisdoomen]

Ah yes, I didn't even consider that.

[jnyrup]

Are there other pieces of information in the HttpResponseMessage that would be valuable to include in the failure message that doesn't need an async/blocking call?

[mu88]

There are headers and cookies - but I think it really depends on the situation what you need

[dennisdoomen]

Maybe we can use ReadAsStream

[bart-degreed]

Information that might be useful is the full request URL including query string, request/response body, incoming and outgoing cookies, headers, and the response status code. When writing production logs, I'd include all that information, at the cost of information overload, because if I don't, it becomes difficult or even impossible to obtain that information.

In test assertions, however, I'd like to quickly see just the essential information instead. The response body typically explains why something failed. If you want to go fancy, I'd suggest adding an overload that takes a [Flags] enum with all of these sources, including All and None shortcuts, but I think just dumping the request body should cover most cases and therefore be the default behavior. When I write HTTP tests, I always assert on the status code first, because if that doesn't match the expectation, subsequent assertions on headers, cookies, and the body aren't likely to succeed. If the status code matches, I'd then assert on headers, which already dumps the dictionary contents if it doesn't match the expectation.

I wouldn't mind if this were an async method. And like I said, the message template can be improved, so it would be nice to put the actual/expected status codes close to each other and produce a sentence that's proper English.

[bart-degreed]

Another enhancement could be to check the Content-Type header first, and if it's binary, then dump the body in hex. Another enhancement could be to truncate the body if it exceeds some threshold.

[bkoelman]

The HTTP version is missing from my list of possibly useful information above, as well as the sender IP, but these are unlikely to be relevant in tests.

[bart-degreed]

If you like, feel free to copy https://github.com/bkoelman/DogAgilityCompetitionManagement/blob/5847fc641b9e81fb732f87294baa108024c2dcd2/src/Circe/Protocol/ByteArrayExtensions.cs. It renders binary data in hex format, something like this:

[bkoelman]

It would be nice to move this into a sub-namespace in the next major release. Currently it's cumbersome to use my own extension method on HttpResponseMessage, the only way to overcome the clash is to use a namespace alias in every file.

[dennisdoomen]

Or even a separate package, e.g. FluentAssertions.Http, FluentAssertions.Data

[bart-degreed]

A separate package already exists at https://github.com/balanikas/FluentAssertions.Http. Makes me wonder why this was integrated.

[jnyrup]

A separate package already exists at https://github.com/balanikas/FluentAssertions.Http. Makes me wonder why this was integrated.

FluentAssertions.Http had it's last update three years ago.
I haven't checked if it even works with FA 6.0+

We also conflicted with the much more maintained https://www.nuget.org/packages/FluentAssertions.Web
We had a discussion with the maintainer of that to overcome the inclusion of HttpResponseMessage.

We did discussed some concerns in #1737, but mostly about the dependency on System.Net.Http and not what havoc adding a Should(this HttpResponseMessage) could cause.

It seems that creating custom extensions for HttpResponseMessage is already so well-established that it has become difficult to create assertions for it in the core lib that satisfies the needs of everyone.
This is a misjudgement on our side, but please remember that every new Should() is potentially breaking someones workflow.

I would be fine splitting this off to a separate library for vNext.
For those that already uses the built-in behavior it should be as simple as adding an extra nuget package.

[dennisdoomen]

Yep, something we can fix in v7.

[mu88]

So a different NuGet package (e. g. FluentAssertions.HttpAssertions) from your side? Or trying to reanimate the existing package FluentAssertions.Http?

[dennisdoomen]

That remains to be seen. We're postponing v7 as long as possible.

[0xced]

I just stumbled on this too and was somewhat disappointed that the default assertion message does not include the HTTP body. Like Bart described, displaying the body (which contains an error message) would have saved me a trip in the debugger.

I'll be using FluentAssertions.Web in the meantime which has excellent output messages on failure.

[bart-vmware]

Is this still in scope for v7? What matters most to me is that the existing implementation gets moved into a separate namespace, to resolve the conflict with our own implementation.

[dennisdoomen]

I haven't looked at this for a while and can't promise anything for v7. I guess the first discussion that we need to have is what path to take. The most logical path is remove direct support entirely and steer existing users to https://github.com/adrianiftode/FluentAssertions.Web