jbogard / ContosoUniversityCore

MIT License
591 stars 151 forks source link

Validation not tested. #27

Open ngbrown opened 6 years ago

ngbrown commented 6 years ago

There are tests for each of the commands, but none of them end up exercising the validation on the commands. That is the FluentValidation or the ComponentModel.DataAnnotation attributes that are on some command properties.

For example, if the following test was added to the ContosoUniversityCore.IntegrationTests.Features.Department.CreateTest class:

[Fact]
public async Task Should_not_create_invalid_department()
{
    var command = new Create.Command
    {
        Budget = 10m,
        Name = "Engineering",
        StartDate = DateTime.Now.Date,
        Administrator = null,
    };

    // really this should fail somehow...
    await SendAsync(command);

    var created = await ExecuteDbContextAsync(db => db.Departments.Where(d => d.Name == command.Name).SingleOrDefaultAsync());

    created.ShouldBeNull();
}

The test would fail because the command's validator rules are not exercised. Specifically this one:

        public class Validator : AbstractValidator<Command>
        {
            public Validator()
            {
                //...
                RuleFor(m => m.Administrator).NotNull();
            }
        }

It seems they are only evaluated on model binding, but no tests exercise that path as it would require spinning up an in-memory ASP.Net server.

How do you normally test the validations? It seems that command validation testing is a central part of the tests that should be in place.

jbogard commented 6 years ago

https://jimmybogard.com/domain-command-patterns-validation/

ngbrown commented 6 years ago

I had read that. I'm not making a comment on the handlers, more on the how to test the validation in the handlers.

I think there are two alternatives; add some controller tests to ensure binding validation runs or modify SliceFixture.SendAsync to also run the validation.

~I'm not 100% sure, but you might find you need to add this line to each action in the controllers:~

if (!ModelState.IsValid) return this.BadRequest(ModelState);

Edit: That's probably taken care of by the ValidatorActionFilter but I don't see where it is tested.

jbogard commented 6 years ago

So in real life, not this contrived application, we use a MediatR pipeline to execute command/domain validation. The request validation is just attributes, declarative, and therefore tests don't really add much value imo.

On Wed, Feb 21, 2018 at 12:52 PM, Nathan Brown notifications@github.com wrote:

I had read that. I'm not making a comment on the handlers, more on the how to test the validation in the handlers.

I think there are two alternatives; add some controller tests to ensure binding validation runs or modify SliceFixture.SendAsync to also run the validation.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jbogard/ContosoUniversityCore/issues/27#issuecomment-367430976, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMhHvC8w8rODSr2a6pz0kPahqF45Kks5tXGXagaJpZM4SOC_R .