Reimplement `SetupSequence` to make it thread-safe #476

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

Source/Language/Flow/SetupSequencePhrase.cs

+{
+	// 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

Source/Language/Flow/SetupSequencePhrase.cs

+
+		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

Source/Mock.cs

@@ -768,7 +768,7 @@ private static Expression GetPropertyExpression(Type mockType, PropertyInfo prop
 		/// Gets the interceptor target for the given expression and root mock, 
 		/// 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

Source/SequenceExtensions.cs

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


(Detail:) Sort usings properly.

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

Source/Language/Flow/SetupSequencePhrase.cs

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


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

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

Source/SequenceMethodCall.cs

+			{
+				// 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

Source/SequenceMethodCall.cs

+				}
+				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

UnitTests/Regressions/IssueReportsFixture.cs

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


Give this test a more sensible name.

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

UnitTests/Regressions/IssueReportsFixture.cs

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


Unnecessary line break, reformat.

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

UnitTests/Regressions/IssueReportsFixture.cs

+				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

Source/Mock.Generic.cs

+		/// </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

Source/Mock.cs

@@ -667,6 +669,44 @@ private static Expression GetValueExpression(object value, Type type)
 			return Expression.Convert(Expression.Constant(value), type);
 		}

+		[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

Source/Mock.cs

+		}
+
+		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

Source/Obsolete/SequenceExtensions.cs

+		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

Source/Obsolete/SequenceExtensions.cs

+		[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

Source/SequenceMethodCall.cs

+
+		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

Source/SequenceMethodCall.cs

+			{
+				// 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement `SetupSequence` to make it thread-safe #476

Reimplement `SetupSequence` to make it thread-safe #476

stakx commented Oct 6, 2017 •

edited

Loading

stakx left a comment

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 19, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx Oct 6, 2017

stakx left a comment

stakx Oct 9, 2017

stakx Oct 9, 2017

stakx Oct 9, 2017

stakx Oct 9, 2017

stakx Oct 9, 2017

stakx left a comment

stakx Oct 19, 2017

stakx Oct 19, 2017

Reimplement SetupSequence to make it thread-safe #476

Reimplement SetupSequence to make it thread-safe #476

Conversation

stakx commented Oct 6, 2017 • edited Loading

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reimplement `SetupSequence` to make it thread-safe #476

Reimplement `SetupSequence` to make it thread-safe #476

stakx commented Oct 6, 2017 •

edited

Loading