Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconstruct SetupSet and VerifySet expressions from delegates #767

Merged
merged 10 commits into from
Mar 6, 2019

Conversation

stakx
Copy link
Contributor

@stakx stakx commented Mar 4, 2019

This converts the current SetupSet machinery with a more accurate one based on a new service component called the ExpressionReconstructor. It takes a delegate and returns a mostly equivalent expression.

The differences between the new component and the old code are these:

  • The old code executed the setup/verification expression delegate on the actual mock; the new code uses a completely separate and more light-weight recording proxy. (This has become possible thanks to Make System.Object members interceptable by *any* interceptor #764.)

  • The new code has much better logic for reconstructing argument matchers, no matter where on a fluent / recursive call they happened. This is done mostly through "timestamping" all invocations, combined with a back-tracking algorithm for spreading them across suitable parameters left-to-right.

This PR also does some additional work to ensure custom matchers will work with SetupSet and VerifySet.

@stakx stakx added this to the 4.11.0 milestone Mar 4, 2019
@stakx stakx added the draft label Mar 4, 2019
Copy link
Contributor Author

@stakx stakx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few baby changes I noticed, plus the MatchExpression-related improvements should probably be extracted into a separate PR to keep this one more focused.

src/Moq/ActionObserver.cs Outdated Show resolved Hide resolved
src/Moq/ActionObserver.cs Outdated Show resolved Hide resolved
@stakx
Copy link
Contributor Author

stakx commented Mar 4, 2019

Roughly around the time of these commits, I've started noticing that test runs in Visual Studio's Test Explorer no longer work reliably: There's the odd NullReferenceException that happens on full test runs, but not when a failing test is debugged. This appears to be caused by tests in ActionObserverFixture; whatever tests get executed along with those will fail with it, if executed in a separate batch, they'll succeed.

This needs some further looking into; could actually be a bug, and not just a Test Explorer malfunction.

@stakx
Copy link
Contributor Author

stakx commented Mar 4, 2019

Note to self: Some code paths in the main interception pipeline are still geared to the old "execute and observe" approach that is largely being deprecated here... however Mock.Of still relies on it.

It would be good to refactor Mock.Of to use the new expression Split-ting based, recursive setup machinery prior to this PR getting merged.

Once that's happened, we can remove obsolete code paths from the interception pipeline, plus trim down AmbientObserver to a mere MatcherObserver (as noone will be registering invocations there anymore).

@stakx stakx force-pushed the expression-reconstruction branch 2 times, most recently from fef32b6 to 1ae8514 Compare March 4, 2019 22:44
Copy link
Contributor Author

@stakx stakx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more things to do before this can be merged.

src/Moq/ActionObserver.cs Outdated Show resolved Hide resolved
src/Moq/ActionObserver.cs Outdated Show resolved Hide resolved
src/Moq/ActionObserver.cs Show resolved Hide resolved
src/Moq/ActionObserver.cs Show resolved Hide resolved
Guard.NotNull(setterExpression, nameof(setterExpression));
var expression = ExpressionReconstructor.Instance.ReconstructExpression(setterExpression);

var setup = Mock.SetupSet(mock, expression, this.condition);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That kind of code is duplicated a lot in other overloads. If ReconstructExpression were not generic, this code pattern could be pushed down into the Mock class, and this fluent API method (as well as others) would turn into one-liners.

src/Moq/Obsolete/Mock.Legacy.cs Show resolved Hide resolved
src/Moq/Protected/ProtectedMock.cs Show resolved Hide resolved
tests/Moq.Tests/ActionObserverFixture.cs Show resolved Hide resolved
@stakx
Copy link
Contributor Author

stakx commented Mar 4, 2019

Roughly around the time of these commits, I've started noticing that test runs in Visual Studio's Test Explorer no longer work reliably: There's the odd NullReferenceException that happens on full test runs, but not when a failing test is debugged. This appears to be caused by tests in ActionObserverFixture; whatever tests get executed along with those will fail with it, if executed in a separate batch, they'll succeed.

This turned out to be an actual code issue caused by static member initialization order between base and derived class; I juggled around the instance and Instance bits of ActionObserver and ExpressionReconstructor, should be fine now. The same change might have to be done for DelegateCompiler et. al.

Moq needs to work around LINQ expression tree limitations of the .NET
compilers by using delegates instead in some places (e.g. `SetupSet`).
The current approach for turning those back into setups is not ideal.

This commit adds a new component to Moq: the `ExpressionReconstructor`
abstract type, along with an early version of a concrete implementa-
tion (`ActionObserver`). The latter is based on a similar principle as
`AmbientObserver`, but it is completely divorced from the `Mock` type.
It uses an independent interceptor type whose sole raison d'être is
to record invocations and to proxy return types.

This is what we'll be able to do with it:

 1. Eventually install it in `SetupSet`, `VerifySet` and a few other
    places so that these can operate on LINQ expression trees, like
    the rest of the Moq API.

 2. Then remove those code paths from the `Mock` interception pipeline
    that are specific to `AmbientObserver` and performed the same
    function that `ExpressionReconstructor` can do in a more focused
    manner.

These changes might also give us easier-to-understand code and
possibly better runtime performance.
Reconstructing argument matchers and placing them in the right place
is achieved as follows:

 * We execute our delegate while an `AmbientObserver` is active. (This
   is where invoked matchers register themselves, and from whence we
   can retrieve them.)

 * We augment `AmbientObserver` such that it timestamps each observa-
   tiion with a sequential number.

 * The recorder proxies timestamp their own creation (using the same
   sequence), as well as the moment when they receive an invocation.

 * Now we deduce that all matchers observed by the `AmbientObserver`
   between a recorder proxy's creation and invocation time "belong" to
   that recorder's invocation. That's because an expression such as
   the following gets evaluated in this order:

    |    X.Y(a, b, c)
    |
    |
    |    X               1. The proxy on which an invocation occurs
    |                       is associated with a recorder creation.
    v        a  b  c     2. Arguments are evaluated.
   time   .Y(       )    3. A proxy member gets invoked.

 * Finally, we try to distribute matchers over the invoked method's
   parameters. Only positions are considered where a non-`default`
   argument value was received (as matchers by convention return `def-
   ault`). These slots are filled with available matchers "from left
   to right" using a simple back-tracking algorithm.

This is still a little rough around the edges. Let's refine later.
So far, matchers only got placed where a parameter received its own
default value. This won't work in a case such as this:

    object Property { get; set; }

    obj => obj.Property = It.IsAny<int>();

because this will invoke `set_Property(object)` with the value 0, i.e.
the default value of the matcher but not of the property type.

If we change the matcher parameter selection rule such that parameters
get chosen whose argument is not equal to a *matcher's* default value.
Using the recently added `ExpressionReconstructor` for `SetupSet` and
`VerifySet` means they can now operate on expressions, too; and they
can plug into the new recursive setup/verification algorithm like the
other API methods.

However, some additional work is needed here to get custom matchers
working as expected. It would be inaccurate of `ExpressionReconstruc-
tor` to just embed a custom matcher's `RenderExpression` into the ex-
pression, as that property is only meant to be used for diagnostic
purposes (e.g. in error messages). Instead, the `Match` itself should
be embedded in the expression. Fortunately there's already `MatchEx-
pression` for just that purpose; it just needs to be improved a little
along with `ExpressionComparer` and `ExpressionStringBuilder`. An ex-
pression containing `MatchExpression`s should never be compiled and
executed. To ensure that, we declare this node type as irreducible.
@stakx stakx force-pushed the expression-reconstruction branch from 1ae8514 to 7c505e0 Compare March 6, 2019 18:01
@stakx stakx merged commit c7bfbdd into devlooped:master Mar 6, 2019
@stakx stakx deleted the expression-reconstruction branch March 6, 2019 22:22
@stakx
Copy link
Contributor Author

stakx commented Mar 6, 2019

To be perfectly honest, I do expect this to cause a few problems in the beginning (when people start updating from <4.11.0 to >=4.11.0), and thus some patching will likely be required. However, for now I think I've done my best.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant