Reimplement `SetupSequence` to make it thread-safe by stakx · Pull Request #476 · devlooped/moq

stakx · 2017-10-06T19:03:35Z

~~This is an early draft and shouldn't be merged just yet.~~

It does appear to resolve #467, however since it's a complete reimplementation of SetupSequence which works in a completely different way than the previous implementation, it needs much more thorough testing of edge cases, as well as a code cleanup.

This resolves #467.

stakx

This is a draft and needs more work.

stakx · 2017-10-06T19:12:34Z

+{
+	// keeping the fluent API separate from `SequenceMethodCall` saves us from having to
+	// define a generic variant `SequenceMethodCallReturn<TResult>`, which would be much more
+	// work that having a generic fluent API counterpart `SequenceMethodCall<TResult>`.


This demonstrates a pattern that should probably be applied more in Moq. Moq's "plumbing" classes (e.g. MethodCall, MethodCallReturn<TResult>, Mock) should be made largely non-generic and kept strictly separate from the generic classes that are part of the fluent API. These latter types only flow the context of a fluent API "phrase" (i.e. a chain of method calls starting with mock.Setup… and ending with the end-of-line semicolon). That way, we could probably get rid of some code duplication.

stakx · 2017-10-06T19:13:47Z

+
+		public ISetupSequentialAction Throws<TException>()
+			where TException : Exception, new()
+			=> this.Throws(new TException());


Perhaps delayed instantiation should be preferred over newing up exception objects way in advance?

stakx · 2017-10-06T19:15:02Z

 		/// building the intermediate hierarchy of mock objects if necessary.
 		/// </summary>
-		private static Interceptor GetInterceptor(Expression fluentExpression, Mock mock)
+		internal static Interceptor GetInterceptor(Expression fluentExpression, Mock mock)


Making this and the following helper methods internal was a quick hack to see if the reimplementation was feasible at all. This needs to be cleaned up. Do these methods actually have to be located in the Mock class?

stakx · 2017-10-06T19:15:28Z

+using System.Reflection;
 using Moq.Language;
+using Moq.Language.Flow;
+using System.Linq;


(Detail:) Sort usings properly.

stakx · 2017-10-06T19:16:03Z

@@ -0,0 +1,65 @@
+using System;


(Detail:) Add missing license text to all new files!

stakx · 2017-10-06T19:25:54Z

+			{
+				// we get here if there are more invocations than configured responses.
+				// if the setup method does not have a return value, we don't need to do anything;
+				// if it does have a return value, we produce the default value.


Verify (via additional unit tests) that the new implementation acts the same way as the old implementation when the configured responses have been exhausted by too many invocations.

The new implementation acts differently; see explanation in #467 (comment). I personally favour the new behaviour as it brings SetupSequence more in line with how other Setup methods work: unless a setup is a conditional setup (.When(...)), then it overrides any previous setup for the same invocation.

stakx · 2017-10-06T19:26:42Z

+				}
+				else if (call.Method.ReturnType.GetTypeInfo().IsValueType)
+				{
+					call.ReturnValue = Activator.CreateInstance(call.Method.ReturnType);


AFAIK other parts of Moq do this in a more complicated fashion, because of nullable value types, COM types, etc. Check whether this is sufficient.

stakx · 2017-10-06T19:27:13Z

+			}
+
+			[Fact]
+			public async Task Test()


Give this test a more sensible name.

stakx · 2017-10-06T19:27:38Z

+				for (int i = 1; i <= length; i++)
+				{
+					seqSetup = seqSetup
+						.Returns(i);


Unnecessary line break, reformat.

stakx · 2017-10-06T19:28:55Z

+				var orderedInvocations = invocationsQueue.OrderBy(x => x).ToArray();
+
+				// The assertion prints the real invocations, you will see duplicates there!
+				Assert.Equal(


@icalvo's original repro code used MSTest's CollectionAssert.AreEqual. Make sure that this is indeed xUnit.NET's way of comparing two sequences.

stakx

Minor points from previous issue have been addressed. Need to add additional unit tests now that try to detect changes in behaviour.

stakx · 2017-10-09T18:32:28Z

+		/// </summary>
+		[SuppressMessage("Microsoft.Design", "CA1006:DoNotNestGenericTypesInMemberSignatures", Justification = "By Design")]
+		public ISetupSequentialResult<TResult> SetupSequence<TResult>(
+			Expression<Func<T, TResult>> expression)


Unnecessary line break, remove.

stakx · 2017-10-09T18:34:20Z


+		[SuppressMessage("Microsoft.Design", "CA1006:DoNotNestGenericTypesInMemberSignatures", Justification = "By Design")]
+		internal static SetupSequencePhrase<TResult> SetupSequence<T, TResult>(Mock<T> mock, Expression<Func<T, TResult>> expression)
+			where T : class


Does this method really have to be generic? Why not make the parameters be of types Mock and LambdaExpression? The public method in Mock<T> can make sure that everything is appropriately typed.

stakx · 2017-10-09T18:34:48Z

+		}
+
+		internal static SetupSequencePhrase SetupSequence<T>(Mock<T> mock, Expression<Action<T>> expression)
+			where T : class


Same here: Does this method really have to be generic? Why not make the parameters be of types Mock and LambdaExpression? The public method in Mock<T> can make sure that everything is appropriately typed.

stakx · 2017-10-09T18:36:24Z

+		public static ISetupSequentialResult<TResult> SetupSequence<TMock, TResult>(
+			this Mock<TMock> mock,
+			Expression<Func<TMock, TResult>> expression)
+			where TMock : class


TMock should be just T, but changing a generic type parameter's name is possibly a (minor) breaking change. Leave as is for now.

stakx · 2017-10-09T18:36:39Z

+		[Obsolete("Please use instance method Mock<T>.SetupSequence instead.")]
+		[SuppressMessage("Microsoft.Design", "CA1006:DoNotNestGenericTypesInMemberSignatures", Justification = "By Design")]
+		public static ISetupSequentialAction SetupSequence<TMock>(this Mock<TMock> mock, Expression<Action<TMock>> expression)
+			where TMock : class


Same here: TMock should be just T, but changing a generic type parameter's name is possibly a (minor) breaking change. Leave as is for now.

stakx

All points have been addressed, this is good to go, except that we might want to add new verbs for SetupSequence such that the default response for an exhausted sequence can be defined.

stakx · 2017-10-19T17:08:24Z

+
+		public override void Execute(ICallContext call)
+		{
+			base.Execute(call);


Done. Added a unit test that ensures Verify can correctly check a sequence's invocation count.

stakx · 2017-10-19T17:08:37Z

+			{
+				// we get here if there are more invocations than configured responses.
+				// if the setup method does not have a return value, we don't need to do anything;
+				// if it does have a return value, we produce the default value.


The new implementation acts differently; see explanation in #467 (comment). I personally favour the new behaviour as it brings SetupSequence more in line with how other Setup methods work: unless a setup is a conditional setup (.When(...)), then it overrides any previous setup for the same invocation.

stakx self-assigned this Oct 6, 2017

stakx commented Oct 6, 2017

View reviewed changes

stakx force-pushed the setupsequence-thread-safety branch 2 times, most recently from d43a177 to 5646a64 Compare October 9, 2017 18:28

stakx commented Oct 9, 2017

View reviewed changes

stakx force-pushed the setupsequence-thread-safety branch 5 times, most recently from 5b366a9 to 3e5564e Compare October 19, 2017 17:03

stakx commented Oct 19, 2017

View reviewed changes

stakx added the breaking change label Oct 19, 2017

stakx changed the title ~~Reimplement the whole machinery around SetupSequence to make it thread-safe (Draft)~~ Reimplement SetupSequence to make it thread-safe Oct 20, 2017

stakx force-pushed the setupsequence-thread-safety branch from 3e5564e to 6bedd9f Compare October 21, 2017 15:11

stakx added 2 commits October 21, 2017 21:20

Add test showing SetupSequence isn't thread-safe

2b8f364

Add tests to document SetupSequence's current behavior

4fa402e

stakx force-pushed the setupsequence-thread-safety branch from 6bedd9f to 9139dfc Compare October 21, 2017 19:21

stakx added 3 commits October 21, 2017 21:28

Reimplement machinery surrounding SetupSequence

6281f4e

Breaking change: SetupSequence overrides preexisting setups

6c3f91f

Update the changelog

c87cdcf

stakx force-pushed the setupsequence-thread-safety branch from 9139dfc to c87cdcf Compare October 21, 2017 19:29

stakx merged commit 50ba5bf into devlooped:develop Oct 21, 2017

stakx deleted the setupsequence-thread-safety branch October 21, 2017 19:34

Jevonius mentioned this pull request Mar 26, 2018

WindsorContainer UsingFactoryMethod broken on upgrade to 4.8.0+ #608

Closed

devlooped locked and limited conversation to collaborators Sep 7, 2024

Uh oh!

Conversation

stakx commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stakx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stakx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stakx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

stakx commented Oct 6, 2017 •

edited

Loading