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

Add support for success condition parameter in When. #1237

Closed
ghost opened this issue Feb 26, 2022 · 5 comments
Closed

Add support for success condition parameter in When. #1237

ghost opened this issue Feb 26, 2022 · 5 comments

Comments

@ghost
Copy link

ghost commented Feb 26, 2022

The current MockSequence implementation looks like this:

internal ISetupConditionResult<TMock> For<TMock>(Mock<TMock> mock)
	where TMock : class
{
	var expectationPosition = sequenceLength++;

	return new WhenPhrase<TMock>(mock, new Condition(
		condition: () => expectationPosition == sequenceStep,
		success: NextStep));
}

Do do a custom similar thing we'd use the When method:

public ISetupConditionResult<T> When(Func<bool> condition)
{
	return new WhenPhrase<T>(this, new Condition(condition));
}

But it doesn't expose the success action.
What I'm suggesting is to evolve this signature to:

public ISetupConditionResult<T> When(Func<bool> condition, Action success = null)
{
	return new WhenPhrase<T>(this, new Condition(condition, success));
}

I'd like to use this to setup a custom condition that depends on that success.

Thanks

@stakx
Copy link
Contributor

stakx commented Feb 26, 2022

That success thingy is an internal implementation detail, and ideally it wouldn't exist at all, because it is somewhat problematic from a thread-safety point of view. Therefore, exposing it does not seem a good idea.

Why don't you simply put your logic in a .Callback(...)?

@ghost
Copy link
Author

ghost commented Feb 26, 2022

@stakx so why is Moq using that internal implementation instead on .Callback(...)?
I thought that maybe it is because .Callback is usable only once per setup and you don't want to fill that space during InSequence.

I'm try to make my custom sequencer InMyCustomSequencer so that the developer can do something like:

mock.InMyCustomSequencer(....)
    .Setup(x => x.Method())
    .Returns(10)
    .Callback(() => developerChoosenCallback())

Basically I'm augmenting Moq features available to a developer to cover a particular use case.

@ghost
Copy link
Author

ghost commented Feb 26, 2022

Actually now I see, Callback is not usable from the "sequencer" because the Setup has not been executed yet.
And I don't know which method will the developer choose to setup.
And that's the reason because I'm asking to expose that success parameter.

@stakx
Copy link
Contributor

stakx commented Feb 26, 2022

Not sure what your "sequencers" are exactly, but if they're in any way related or similar to MockSequence, then it's no wonder you ended up with setup.When(...) – right now, it's the only way to implement sequences.

That being said, Moq's sequences are very limited, and conditional setups that they're based on have a whole bunch of warts.

Instead of adding another wart to conditional setups (exposing the internal success callback) and then having to maintain that new public API indefinitely, we should figure out a better way how to support setting up & verifying a sequence of calls, and focus on that. See e.g. #75 about that.

@ghost
Copy link
Author

ghost commented Feb 26, 2022

My goal is to test a service call that internally uses an unit of work multiple times to call repositories.

void SomeServiceMethod()
{
    uow.BeginTran();
    repo1.DoThings();
    repo2.DoOtherThings();
    uow.Commit();

    // do other things

    uow.BeginTran();
    repo3.DoSomething();
    repo4.DoEverything();
    uow.Commit();
}

Of course I we need to mock UnitOfWork and all of the repositories to make a good unit test.

I want to verify that repositories are being called in the same transaction in the correct order.

mockRepo1.InTransaction(mockUnitOfWork).Setup(x => x.DoThings(), transactionIndex: 0).Verifiable();
mockRepo2.InTransaction(mockUnitOfWork).Setup(x => x.DoOtherThings(), transactionIndex: 0).Verifiable();

mockRepo3.InTransaction(mockUnitOfWork).Setup(x => x.DoSomething(), transactionIndex: 1).Verifiable();
mockRepo4.InTransaction(mockUnitOfWork).Setup(x => x.DoEverything(), transactionIndex: 1).Verifiable();

At the moment I'm using reflection to make use of that hidden parameter: I know this is a horrible practice.

/// <summary>
/// Internal <see cref="Moq.Language.Flow.WhenPhrase{T}"/> class used by sequence
/// </summary>
private readonly static Type _whenPhraseGenericType = typeof(Mock).Assembly.GetTypes().First(t => t.FullName == "Moq.Language.Flow.WhenPhrase`1");

/// <summary>
/// Internal <see cref="Condition"/> class used by sequence
/// </summary>
private readonly static ConstructorInfo _conditionCtor = typeof(Mock).Assembly.GetTypes().First(t => t.FullName == "Moq.Condition").GetConstructor(new[] { typeof(Func<bool>), typeof(Action) })!;

internal ISetupConditionResult<TMock> For<TMock>(Mock<TMock> mock, int transactionIndex, bool verifySequence) where TMock : class
{
    while (this._sequenceLengths.Count <= transactionIndex)
    {
        this._sequenceLengths.Add(0);
    }

    int expectationPosition = this._sequenceLengths[transactionIndex]++;

    //var condition = new Condition(() => this._transactionIndex == transactionIndex && (expectationPosition == this._sequenceStep || !verifySequence), NextStep);
    object condition = _conditionCtor.Invoke(new object[] { () => this._transactionIndex - 1 == transactionIndex && (expectationPosition == this._sequenceStep || !verifySequence), NextStep });
    //return new WhenPhrase<TMock>(mock, condition);
    return (ISetupConditionResult<TMock>)_whenPhraseGenericType.MakeGenericType(typeof(TMock)).GetConstructor(new[] { mock.GetType(), condition.GetType() })!.Invoke(new object[] { mock, condition });
}

I get what you're saying though. And I agree with you, there's should be a better way of customizing sequence logics.

I'm going to close this issue, and if I come up with an idea, I'll report it in the issue that you linked.

@ghost ghost closed this as completed Feb 26, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant