Make DefaultHttpContext more testable

[davidfowl]

Today you have to pass in a feature collection. There should be a default ctor that allows getting and setting all of the properties.

[davidfowl]

I'd like @pranavkm @halter73@rynowak @harshgMSFT to chime in here since they have been writing unit tests agains the new HttpAbstractions. The goal here is the make it easy across the board so that frameworks like Mvc and SignalR can do it as well as end users wanting to test their controllers/hubs etc.

[pranavkm]

cc @rynowak

[rynowak]

In order of importance:

• get/set properties for request/response stuff
• related objects/collections non-null/mutable by default - adding query strings, headers, form-data, etc.
• no arg constructor

Looking though a lot of our test code we could swap out the mocks now that we have a no-arg constructor.

If we have those things I don't think there are major barriers.

The other really common things that are painful are mocking options and service container, but those aren't super related.

[davidfowl]

I think we also need an easy way to mock the query string and form collections. There doesn't seem to be a standard implementation of ReadableStringCollection back by a dictionary that we can use.

[Tratcher]

@rynowak @halter73 Do you have any unit tests we can work on to simplify? Or samples of unit tests you like to write but can't?

[rynowak]

test\Microsoft.AspNet.Mvc.ModelBinding.Test\ValueProviders\FormValueProviderFactoryTests.cs in MVC repo is a good examplar

[Tratcher]

@rynowak Here's the existing CreateContext from that test:

        private static ValueProviderFactoryContext CreateContext(string contentType)
        {
            var collection = Mock.Of<IReadableStringCollection>();
            var request = new Mock<HttpRequest>();
            request.Setup(f => f.GetFormAsync(CancellationToken.None)).Returns(Task.FromResult(collection));

            var mockHeader = new Mock<IHeaderDictionary>();
            mockHeader.Setup(h => h["Content-Type"]).Returns(contentType);
            request.SetupGet(r => r.Headers).Returns(mockHeader.Object);

            var context = new Mock<HttpContext>();
            context.SetupGet(c => c.Request).Returns(request.Object);

            return new ValueProviderFactoryContext(
                context.Object, 
                new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase));
        }

Here's another way of doing that (today, without any changes):

        private static ValueProviderFactoryContext CreateContext(string contentType)
        {
            var httpContext = new DefaultHttpContext();
            httpContext.Request.Headers["Content-Type"] = contentType; // TODO: Content-Type is being added as a property soon, correct?
            httpContext.SetFeature<IFormFeature>(new TestFormFeature());

            return new ValueProviderFactoryContext(
                httpContext,
                new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase));
        }

        private class TestFormFeature : IFormFeature
        {
            public TestFormFeature()
                : this(new ReadableStringCollection(new Dictionary<string, string[]>()))
            {                
            }

            public TestFormFeature(ReadableStringCollection form)
            {
                Form = form;
            }

            public ReadableStringCollection Form { get; set; }

            public Task<IReadableStringCollection> GetFormAsync(CancellationToken cancellationToken)
            {
                return Task.FromResult<IReadableStringCollection>(Form);
            }
        }

If this looks useful then we could make TestFormFeature available somewhere, or we could augment the normal FormFeature to support this directly.

[pranavkm]

TestFormFeature sounds like a better way to go about it. It might be useful to additionally have properties \ methods on it be virtual so they can be mocked.

[rynowak]

If we went up having a TestFormFeature where will it go? Do you anticipate having other things like this? How much of a difference in implementation is there between the 'test' version and the default implementation. There's a benefit to not needing another class - it's much more discoverable, and can help us find gaps/issues in the default implementation.

The HttpContext and headers parts look slammin

[Tratcher]

Lets hold off on the 'where do we put it' discussion until we've decided if it's useful.

The normal FormFeature implementation reads from the request body, which I was told you explicitly did not want to do in unit tests.

For reference, here's the existing FormFeature:
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.PipelineCore/FormFeature.cs

The QueryFeature is very similar:
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.PipelineCore/QueryFeature.cs

I don't think any of the other current features fall into this pattern, but in the future we may have other such features for reading the request body in a certain way.

[rynowak]

There's also good potential here to just merge the constructors that TestFormFeature adds into the default implementation without much friction. I don't have strong feelings either way - either one of these satisfies our needs.

I'm asking if there will be more stuff, because if we end up with more things like this, we might be so inclined to just create a 'TestHttpContext that instantiates and uses 'new' to expose all of the 'test' versions of various data.

[Tratcher]

If we did any Test* types, I'd put it over in Microsoft.AspNet.Testing for folks to share.

I'll play around with putting it into the core library first.

[Tratcher]

How about this?

var form = new Dictionary<string, string[]>(StringComparer.OrdinalIgnoreCase)
{
   { "Foo", new[] { "Bar" } }
};
httpContext.SetFeature<IFormFeature>(new FormFeature(new ReadableStringCollection(form)));

Or do I need to simplify that down one more layer to:

var form = new Dictionary<string, string[]>(StringComparer.OrdinalIgnoreCase)
{
   { "Foo", new[] { "Bar" } }
};
httpContext.SetFeature<IFormFeature>(new FormFeature(form));

No extra types required, just a new constructor or two.

[Tratcher]

BTW, QueryFeature would be exactly the same.

[rynowak]

Looks good to me.

[Tratcher]

Any other test scenarios you want to investigate, or should we call this one done for now?