Beginnings of #25.

[AlexArchive]

Note that this PR is intended to fail.

While working on #25 I discovered that inflecting an expression is not quite as straightforward as I had hoped. It turns out that calling ToString on an expression produces some funky results. I added some failing tests (hence the failing build) to demonstrate the problem.

I have done some preliminary research, but I cannot find anything of use.

Any thoughts on how to overcome this obstacle?

[robdmoore]

When you say funky results what do you mean? From what I can tell from the build failures it looks ok (the strings being asserted against just need to be changed?).

[AlexArchive]

(Sorry for the miscommunication.)

Well it prints AndAlso instead of && for one.

More importantly, there is no telling what the error message might look like if the predicate involves compiler generated code (as is the case when you capture an outer variable).

Consider evaluating the Exception.Message using the debugger to see what I mean.

I want to avoid exception messages like this one:

Expected view model {"Property1":"test","Property2":3} to pass the given condition (m => m.Property1 == null AndAlso m.Property2 == valueTestStack.FluentMVCTesting.Tests.ViewResultTestShould+<>c__DisplayClass7.capturedOuterVariable), but it failed.

[robdmoore]

Ahh I see. Darn.

Looks like we do need something a bit fancier.

As a starting point we could look at how Shouldly does it: https://github.com/shouldly/shouldly/blob/e47f958a20b1fb247e262bb503683cc21f80c13a/src/Shouldly/Internals/StringHelpers.cs#L28

[AlexArchive]

😢

Maybe I am being thick but I do not see the relationship between what we need here and what Shouldy does. Could you elucidate?

I have been doing some research and the only robust solution I can find is this one. It would need tailoring for our needs and I am happy to do that.

Thoughts?

[robdmoore]

With that Inspect method, if you go lambda.Inspect() it will return a string representation of it.

It probably won't be as fancy as the one you linked to, but it might do the job given we aren't expecting people to use really complex lambdas.

Either way, whatever you decide to introduce I think should be in a sub-namespace (maybe Implementation?) and be marked internal. Happy for you to unit test it if you feel the need, or I'm also happy to just test it via the public API that needs it (that way we can swap out the implementation later if we decide to).

[AlexArchive]

I think the Inspect method suffers from the same problem.

[AlexArchive]

I am having some trouble approaching this using TDD. I would really love your input.

I want to build a class called ExpressionInspector using TDD. The class will have a method called Inspect that when called with an Expression, will return the textual representation of the expression. We will call this class from the view model test methods.

The trouble is there are so many conditions to test for. For instance, I need to test a bunch of binary operators: ==, <, >= and so on. I also need to test each of these binary operators with different operand types like string and int. I do not think there is a good reason to test every possible input, but in the case of binary operators for example, I do think there should be a few tests.

Do you have any insight as to how to approach this in a clean way?

Personally I find naming tests like this to be difficult. Could you please list a few example test names that meet the test naming convention?

Thanks so much man.

[robdmoore]

I'd identify a list of predicates that we want to support and then pop each of them into a test. So I think the following are probably good enough to start - we can add others later if we or other people notice them and report it:

ExpressionInspectorShould
    Correctly_parse_equality_comparison_with_two_properties
    Correctly_parse_equality_comparison_with_constant
    Correctly_parse_equality_comparison_with_closure_variable
    Correctly_parse_inequality_comparison
    Correctly_parse_lessthan_comparison
    Correctly_parse_greaterthan_comparison
    Correctly_parse_lessthanorequalto_comparison
    Correctly_parse_greaterthanorequalto_comparison
    Correctly_parse_boolean_value
    Correctly_parse_boolean_value_with_notoperator
    Correctly_parse_or_statement
    Correctly_parse_and_statement
    Correctly_parse_combination_of_or_and_and_statements

Each test might look something like:

[Test]
public void Correctly_parse_equality_comparison()
{
    Func<TestViewModel, bool> predicate = v => v.Property1 == v.Property2;
    const string expected = "v.Property1 == v.Property2";

    var actual = ExpressionInspector.Inspect(predicate);

    Assert.That(actual, Is.EqualTo(expected);
}

Only other thing I'm thinking is whether we should get the inspector to put in the values of the properties in there somehow as well to make the statement even more useful for debugging...

One other thing to think about is whether we even care about the ExpressionInspector at all? In reality it's an implementation detail and we only really care about the effect on the error message when using a predicate for looking at the model. We might decide to write the tests from that level instead in which case the test would look similar to above except the Act part will have a .WithModel(predicate) instead of the call to ExpressionInspector.

[AlexArchive]

Thank you man.

I am going to have a stab at this tonight.

[AlexArchive]

Sorry my lackadaisical response earlier, I had only just woken up.

Only other thing I'm thinking is whether we should get the inspector to put in the values of the properties in there somehow as well to make the statement even more useful for debugging...

We spoke about this previously and I concur, we absolutely should. I am going to focus on inflecting the expression tree first though.

One other thing to think about is whether we even care about the ExpressionInspector at all?

I completely see where you are coming from and I think what you suggest is the conceptually sound thing to do. I know that using InternalsVisibleTo is a hack but in practice, I find it easier to work this way because the fixture is more focused. Is that OK?

[robdmoore]

I'm happy for you to run with whichever option suits you :)

On 15 Oct 2014, at 12:42 am, ByteBlast notifications@github.com wrote:

Sorry my lackadaisical response earlier, I had only just woken up.

Only other thing I'm thinking is whether we should get the inspector to put in the values of the properties in there somehow as well to make the statement even more useful for debugging...

We spoke about this previously and I concur, we absolutely should. I am going to focus on inflecting the expression tree first though.

One other thing to think about is whether we even care about the ExpressionInspector at all?

I completely see where you are coming from and I think what you suggest is the conceptually sound thing to do. I know that using InternalsVisibleTo is a hack but in practice, I find it easier to work this way because the fixture is more focused. Is that OK?

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

[robdmoore]

Btw Have you watched Ian coopers TDD Where did it go wrong talk?

On 15 Oct 2014, at 12:42 am, ByteBlast notifications@github.com wrote:

Sorry my lackadaisical response earlier, I had only just woken up.

Only other thing I'm thinking is whether we should get the inspector to put in the values of the properties in there somehow as well to make the statement even more useful for debugging...

We spoke about this previously and I concur, we absolutely should. I am going to focus on inflecting the expression tree first though.

One other thing to think about is whether we even care about the ExpressionInspector at all?

I completely see where you are coming from and I think what you suggest is the conceptually sound thing to do. I know that using InternalsVisibleTo is a hack but in practice, I find it easier to work this way because the fixture is more focused. Is that OK?

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

[AlexArchive]

Yes I watched it a couple of months ago. Honestly, back then it went over my head. I should watch it again now I have some more experience.

Is there a particular part of the recording I should watch that is relevant to this PR do you think?

[robdmoore]

Just his general assertion about testing from the public API rather than implementation detail

On 16 Oct 2014, at 2:09 am, ByteBlast notifications@github.com wrote:

Yes I watched it a couple of months ago. Honestly, back then it went over my head. I should watch it again now I have some more experience.

Is there a particular part of the recording I should watch that is relevant to this PR do you think?

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

[AlexArchive]

I do not think it will be possible to remove the extraneous parentheses that surround expressions because it is too difficult to differentiate said extraneous parentheses from meaningful ones.

source code text 
-> what the inspector output will look like

text == ""  || text.Length > 1
 -> ((text == "") || (text.Length > 1))

text.Contains("fml")
->  text.Contains("fml")

Should I roll with this? I really do not think the ideal output (i.e. one with no extraneous parentheses) will be attainable without writing a considerably complicated parser (even using the CodeDomExpressionVisitor.) Maybe in the future we can leverage Roslyn to do this.

[robdmoore]

It looks great mate. Ship it :) we can always improve if in the future if needed.

On 18 Oct 2014, at 6:02 am, ByteBlast notifications@github.com wrote:

I do not think it will be possible to remove the extraneous parentheses that surround expressions because it is too difficult to differentiate said extraneous parentheses from meaningful ones.

source code text
-> what the inspector output will look like

text == "" || text.Length > 1
-> ((text == "") || (text.Length > 1))

text.Contains("fml")
-> text.Contains("fml")
Should I roll with this? I really do not think the ideal output (i.e. one with no extraneous parentheses) will be attainable without writing a considerably complicated parser (even using the CodeDomExpressionVisitor.) Maybe in the future we can leverage Roslyn to do this.

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

[AlexArchive]

What you are looking at here is probably the simplest possible passable implementation. There are some limitations but those limitations are not likely to to be prevalent imo.

I intend for the tests to serve as regression tests when I implement something like this in the (hopefully-near) future.

I suspect this implementation is pretty robust because it is mostly predicated on vendor code (like Json.Encode), I would still be thankful if you would review it.

[AlexArchive]

There is a discrepancy with the InternalsVisibleTo attribute that causes the OldMvc builds to fail.

I was hoping you could take a look at it and then advise?

Thanks in advance.

[AlexArchive]

Bump.

[robdmoore]

Taking a look now

[robdmoore]

Add the following to TestStack.FluentMVTesting\Properties\AssemblyInfo.cs:

[assembly: InternalsVisibleTo("TestStack.FluentMVCTesting.Mvc3.Tests")]
[assembly: InternalsVisibleTo("TestStack.FluentMVCTesting.Mvc4.Tests")]

[AlexArchive]

Thanks. Implemented 👍

[AlexArchive]

Hey man.

I hope you get time to review this soon 👍

[robdmoore]

Sorry mate - thought you were going to merge it since I already looked. My bad.