TempData & ViewBag Support

[AlexArchive]

Earlier today I had to write a unit test against the TempData property and wondered if there was any support in this library for this - there is not.

The problem that I have is that TempData is basically weakly typed and casting is not elegant_. I love the way this library enables strong typing when possible and thought It *might_ be awesome if there was some support for this.

Because there would already be support for TempData I guess it would make sense to have a ViewBag counterpart too.

I am not entirely convinced as to how valuable this feature would be nor am I confident about what the syntax might look like, but I thought I would throw it out there for discussion anyway.

*It is not so much that I find casting inelegant but rather - I use this library for all my controller tests and I find that altering between this library and standard tests does not look fluent and consequently, neat.

[robdmoore]

In terms of adding ViewBag support and an idea of a syntax for it check out: #4

In terms of a syntax for TempData I guess we need to have an extension method off of the controller since we can't chain that check off the ActionResult or any of the subsequent objects.

Maybe something like (?):

    _controller.ShouldHaveTempDataProperty<string>("key")
    _controller.ShouldHaveTempDataProperty<string>("key", "value")
    _controller.ShouldHaveTempDataProperty<string>("key", s => s.EndsWith("ue"))
    _controller.ShouldHaveTempDataProperty<string>("key", s => { Assert.That(s, Is.EqualTo("value"); })

[AlexArchive]

I strongly recalled reading an issue about view bag support but when I skimmed both the open and closed issues I could not see it - I thought I must be mad! Turns out it was a pull request ugh.

Oh wow. That syntax looks superb Rob.

Unless somebody else wants to take this on, I will work on this soon.

[robdmoore]

Ha! Happens to the best of us :P

I suspect nobody is on this so feel free :)

[AlexArchive]

It occurred to me that we could enable syntax like this:

[Test]
public void Delete_ExistentPost_ReturnsMessage()
{
    // Regular method on ControllerResultTest<T>.
    controller
        .WithCallTo(c => c.Delete(""))
        .ShouldHaveTempDataProperty<string>("Message")
}

This is different from your proposed solution:

[Test]
public void Delete_ExistentPost_ReturnsMessage()
{
    controller.Delete("");

    // Extension method on Controller. 
    controller
        .ShouldHaveTempDataProperty<string>("Message");
}

The benefit of this proposed solution is consistency with the rest of the API.

The drawback however, is that when you want to perform multiple logical assertions, the syntax is less than nice:

[Test]
public void Delete_ExistentPost_RedirectsToIndexWithMessage()
{
    controller
        .WithCallTo(c => c.Delete(""))
        .ShouldRedirectTo(c => c.Index);

   // Regular method on ControllerResultTest<T>.  Eww. 
   controller
        .WithCallTo(c => c.Delete(""))
        .ShouldHaveTempDataProperty<string>("Message")        
}

This is different from your proposed approach:

[Ignore]
public void Delete_ExistentPost_RedirectsToIndexWithMessage()
{
    controller
        .WithCallTo(c => c.Delete(""))
        .ShouldRedirectTo(c => c.Index);

    // Extension method on Controller. 
    controller
        .ShouldHaveTempDataProperty<string>("Message");    
}

I am on the fence about which approach to take. Which do you prefer?

[robdmoore]

I prefer what I suggested just because it means we aren't needing to invoke the method twice. Thoughts?

On 9 Sep 2014, at 10:35 pm, ByteBlast [email protected] wrote:

It occurred to me that we could enable syntax like this:

[Test]
public void Delete_ExistentPost_ReturnsMessage()
{
// Regular method on ControllerResultTest.
controller
.WithCallTo(c => c.Delete(""))
.ShouldHaveTempDataProperty("Message")
}
This is different from your proposed solution:

[Test]
public void Delete_ExistentPost_ReturnsMessage()
{
controller.Delete("");

// Extension method on Controller. 
controller
    .ShouldHaveTempDataProperty<string>("Message");

}
The benefit of this proposed solution is consistency with the rest of the API.

The drawback however, is that when you want to perform multiple logical assertions, the syntax is less than nice:

[Test]
public void Delete_ExistentPost_RedirectsToIndexWithMessage()
{
controller
.WithCallTo(c => c.Delete(""))
.ShouldRedirectTo(c => c.Index);

// Regular method on ControllerResultTest. Eww.
controller
.WithCallTo(c => c.Delete(""))
.ShouldHaveTempDataProperty("Message")
}
This is different from your proposed approach:

[Ignore]
public void Delete_ExistentPost_RedirectsToIndexWithMessage()
{
controller
.WithCallTo(c => c.Delete(""))
.ShouldRedirectTo(c => c.Index);

// Extension method on Controller. 
controller
    .ShouldHaveTempDataProperty<string>("Message");    

}
And so I am torn. What do you think?

—
Reply to this email directly or view it on GitHub.

[AlexArchive]

I was leaning that way too so yeah, all good.

I will start on this now.

[AlexArchive]

How do you want to handle multiple tests?

_controller.ShouldHaveTempDataProperty<string>("a")
_controller.ShouldHaveTempDataProperty<string>("b")

_controller
    .ShouldHaveTempDataProperty<string>("a")
    .ShouldHaveTempDataProperty<string>("b")

_controller
    .ShouldHaveTempDataProperty<string>("a")
    .AndShouldHaveTempDataProperty<string>("b")

[mwhelan]

Probably with an And, so that you can just reuse assertions rather than creating And versions of each. Fluent Assertions uses And that way. Here is an example from Fluent Assertions:

action
   .ShouldThrow<RuleViolationException>()
   .WithMessage("change the unit of an existing ingredient", ComparisonMode.Substring)
   .And.Violations.Should().Contain(BusinessRule.CannotChangeIngredientQuanity);

So your example above would be:

_controller
    .ShouldHaveTempDataProperty<string>("a")
    .And.ShouldHaveTempDataProperty<string>("b")

[robdmoore]

That seems reasonable.

On 10 Sep 2014, at 12:45 am, Michael Whelan [email protected] wrote:

Probably with an And, so that you can just reuse assertions rather than creating And versions of each. Fluent Assertions uses And that way. Here is an example from Fluent Assertions:

action
.ShouldThrow()
.WithMessage("change the unit of an existing ingredient", ComparisonMode.Substring)
.And.Violations.Should().Contain(BusinessRule.CannotChangeIngredientQuanity);
So your example above would be:

_controller
.ShouldHaveTempDataProperty("a")
.And.ShouldHaveTempDataProperty("b")
—
Reply to this email directly or view it on GitHub.

[AlexArchive]

I do not understand the purpose of the And. here :c Can somebody explain?

Especially because elsewhere in the library we do this:

controller
    .WithCallTo(c => c.Index())
    .ShouldRenderDefaultView()
    .WithModel<string>()
    .AndModelError("foo")
    .AndModelError("bar");

[robdmoore]

Maybe just make the new extension return the controller and leave it at that?

ByteBlast [email protected] wrote:

I do not understand the benefit of the And. here :c Can somebody explain?

Especially because elsewhere in the library we do this:

controller
.WithCallTo(c => c.Index())
.ShouldRenderDefaultView()
.WithModel()
.AndModelError("foo")
.AndModelError("bar");
—
Reply to this email directly or view it on GitHub.

[AlexArchive]

That would conform to the syntax I described earlier:

_controller
    .ShouldHaveTempDataProperty<string>("a")
    .ShouldHaveTempDataProperty<string>("b")

Especially easy to implement but not very fluent, wouldn't you say?

[robdmoore]

You could have an AndShouldHaveTempDataProperty method that is a synonym too?

On 10 Sep 2014, at 6:32 am, ByteBlast [email protected] wrote:

That would conform to the syntax I described earlier:

_controller
.ShouldHaveTempDataProperty("a")
.ShouldHaveTempDataProperty("b")
Not terribly fluent, wouldn't you say?

—
Reply to this email directly or view it on GitHub.

[AlexArchive]

That would enable syntax like this though, would it not?:

_controller
    .AndShouldHaveTempDataProperty<string>("a")
    .ShouldHaveTempDataProperty<string>("b")

We could perhaps return a TempDataTestResult that has the AndShouldHaveTempDataProperty method. What do you think?

[robdmoore]

Yep that works too :)

[mwhelan]

I don't have much experience with this library, so I'm not qualified to comment too much on the API. I'm sure you both will come up with something great. I think that the And syntax works well with fluent interfaces though, and it's worth sharing a bit of code for consideration. Fluent Assertions has this simple class, which is used to wrap a lot of the return values, and would save getting into constructs like TempDataTestResult.

public class AndConstraint<T>
{
    private readonly T parentConstraint;

    /// <summary>
    /// Initializes a new instance of the <see cref="T:System.Object"/> class.
    /// </summary>
    public AndConstraint(T parentConstraint)
    {
        this.parentConstraint = parentConstraint;
    }

    public T And
    {
        get { return parentConstraint; }
    }
}

If you take @byteblast 's ShouldHaveTempDataProperty and adapt it to return an AndConstraint rather than void (or presumably Controller eventually).

public static AndConstraint<T> ShouldHaveTempDataProperty<T>(this T controller, string key, object value = null) where T : Controller
{
    var actual = controller.TempData[key];

    if (actual == null)
    {
        throw new TempDataAssertionException(string.Format(
            "Expected TempData to have a non-null value with key \"{0}\", but none found.", key));
    }

    if (value == null)  return new AndConstraint<T>(controller);

    if (actual.GetType() != value.GetType())
    {
        throw new TempDataAssertionException(string.Format(
            "Expected value to be of type {0}, but instead was {1}.",
            value.GetType().FullName,
            controller.TempData[key].GetType().FullName));
        }

    if (!value.Equals(actual))
    {
        throw new TempDataAssertionException(string.Format(
            "Expected value for key \"{0}\" to be \"{1}\", but instead found \"{2}\"", key, value, actual));
    }
    return new AndConstraint<T>(controller);
}

then this syntax becomes possible, which I think is quite "fluent".

[Test]
public void Check_for_multiple_existent_temp_data_properties_and_check_values()
{
    const string key1 = "key1";
    const int value1 = 10;
    _controller.TempData[key1] = value1;

    const string key2 = "key2";
    const int value2 = 20;
    _controller.TempData[key2] = value2;

    _controller
        .ShouldHaveTempDataProperty(key1, value1)
        .And.ShouldHaveTempDataProperty(key2, value2);
}

This approach could potentially be used elsewhere in the API - breaking changes of course! :-) Here, I've added a ModelErrorFor method to replace AndModelErrorFor. (Apologies if the code is rough. I've just done enough to wrap the return values and make the tests pass).

public interface IModelTest<TModel>
{
    IModelErrorTest<TModel> AndModelErrorFor<TAttribute>(Expression<Func<TModel, TAttribute>> memberWithError);
    IModelTest<TModel> AndNoModelErrorFor<TAttribute>(Expression<Func<TModel, TAttribute>> memberWithNoError);
    IModelErrorTest<TModel> AndModelError(string errorKey);
    void AndNoModelErrors();

    AndConstraint<IModelErrorTest<TModel>> ModelErrorFor<TAttribute>(Expression<Func<TModel, TAttribute>> memberWithError);
}

public class ModelTest<TModel> : IModelTest<TModel>
{
    public AndConstraint<IModelErrorTest<TModel>> ModelErrorFor<TAttribute>(Expression<Func<TModel, TAttribute>> memberWithError)
    {
        var member = ((MemberExpression)memberWithError.Body).Member.Name;
        if (!_controller.ModelState.ContainsKey(member) || _controller.ModelState[member].Errors.Count == 0)
            throw new ViewResultModelAssertionException(string.Format("Expected controller '{0}' to have a model error for member '{1}', but none found.", _controller.GetType().Name, member));
        var test = new 
            ModelErrorTest<TModel>(this, member, _controller.ModelState[member].Errors);
        return new AndConstraint<IModelErrorTest<TModel>>(test);
    }
    ...
}

Which would allow these use cases:

[Test]
public void Chain_calls_to_check_model_error_in_property()
{
    var returnVal = new AndConstraint<IModelErrorTest<TestViewModel>>(Substitute.For<IModelErrorTest<TestViewModel>>());
    _modelTest.ModelErrorFor(Arg.Any<Expression<Func<TestViewModel, string>>>()).Returns(returnVal);

    Assert.That(_modelErrorTest.ModelErrorFor(m => m.Property1), Is.EqualTo(returnVal));
    _modelErrorTest
        .ModelErrorFor(m => m.Property1)
        .And.ModelErrorFor(m => m.Property2);
}

I think this avoids the issue @byteblast raised with having And methods before Should methods, which I agree is not ideal. You would only ever get And after your first call to a method, as it is being returned from that method.

The use of an And constraint shouldn't imply that there could also be an Or constraint. It is really syntactic sugar to chain multiple assertions and there is no And/Or logic going on between assertions.

Thoughts?

[robdmoore]

While the code behind it is more complex, I actually prefer having less .'s needed by combining into one method e.g. AndModelErrorFor. I think as a consumer of the fluent API it makes it less painful to use.

[mwhelan]

Yes, that is similar to Shouldly vs Fluent Assertions, and it's a personal preference thing. I think the current API is good the way it is, from what I've seen. This approach starts to have advantages if you were to introduce the AndShouldHaveTempDataProperty suggested above, where you get into the odd situation of having the And method before the Should method, as it constrains the order. Perhaps then, if you just stick to the one ShouldHaveTempDataProperty that would be consistent with the multiple calls to AndModelError style of the current API listed above.

[AlexArchive]

@mwhelan, I think you make a good argument but in the end I have to agree with @robdmoore - less .s is desirable.

[mwhelan]

No worries :-)