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

Ability to evaluate "verify" after a certain period of time #931

Closed
Seikilos opened this issue Sep 24, 2019 · 10 comments
Closed

Ability to evaluate "verify" after a certain period of time #931

Seikilos opened this issue Sep 24, 2019 · 10 comments

Comments

@Seikilos
Copy link

Would this enhancement align with the roadmap of moq?

Sometimes we need to verify a call to a method but need to consider timings introduced by the implementation.

A modified example from the docs how it could look like
mock.Verify(foo => foo.DoSomething("ping"),After(2000,100), Times.AtLeast(2));

This would verify that a call to DoSomething was done at least twice within the period of 2 seconds. To avoid blocking the code longer than necessary and because this could be implemented as an Thread.Sleep(2000); before the verify, the second parameter is the time interval in which the verification is evaluated. If condition is true after 200 ms, it completes earlier.

@stakx
Copy link
Contributor

stakx commented Sep 24, 2019

Would this enhancement align with the roadmap of moq?

I don't think so.

To me it seems like a bad idea (in general) to introduce time dependencies into tests, unless you really have to. I'm saying that because you usually get no guarantees from the OS nor from the test runner how your test is scheduled for execution. For example, it could be that it gets run on a low-priority thread with lots of interruptions, and will therefore take longer than you expect. If you choose a timeout that's too small, you'll end up with a fragile test that sometimes fails for no good reason. The easiest way to prevent that will be to choose a timeout that's too large, making your test run unnecessarily slow. I believe that tests should be written in a time-independent manner whenever possible, and Moq's API should reflect that.

I dare say it's only in a small minority of all tests that time matters. For these, like you say, it would be simple enough to have an await Task.Delay(...) wherever you need it. There's no need to put that delay in the core library.

@stakx stakx closed this as completed Sep 24, 2019
@bormm
Copy link

bormm commented Oct 11, 2021

@stakx:
In theory that's true but in many real world enterprise applications you have something like server timeouts to handle, switch to fallback server and so one. For these you have some kind of timing requirements that you have to verify. A delay will stop the test always, so especially for the reasons you have given (no guarantees from the OS) it make sense to have a always matching timeout, but allow the test to complete earlier. With a fix delay that's not working and test need always the maximum allowed time.

A example from a other mock framework: https://www.javadoc.io/doc/org.mockito/mockito-core/2.2.9/org/mockito/verification/VerificationWithTimeout.html

@stakx
Copy link
Contributor

stakx commented Oct 11, 2021

@bormm: Yes, there may possibly be some cases where you really do want a timeout in your test. I still think this should be the exception, rather than the general case. And like I said, in those few cases, nothing stops you from simply putting an await Task.Delay(timeout); right before the calls to mock.Verify(...);. Given that possibility, and that time dependencies are probably not something you should generally have in your tests, I see no need to cover timeouts in Moq's API. Just because other libraries (such as Mockito) have that capability doesn't necessarily mean it's a good thing to have in Moq. Or am I misunderstanding your point?

@stakx
Copy link
Contributor

stakx commented Oct 11, 2021

P.S.: The only thing that might make this feature slightly more attractive would be if mock.Verify(..., timeout); would continue as soon as verification fails, succeeds, or the timeout is up, thereby not blocking test execution longer than necessary (as with a fixed delay using await Task.Delay(...);). Is this what this is about?

@bormm
Copy link

bormm commented Oct 12, 2021

@stakx Exactly. As I wrote, your suggested "Task.Delay(timeout);" will delay the test in any case.
I working more than 24 years as a professional developer and I also hate timeouts. But every application/component that has to interact with something out of the own process, does need timeouts. You can't wait forever if the connection is there, you can't wait forever if the response is coming. Even on the local system: You can't wait on a event forever, the other process is maybe crashed or was killed.

Of course often timeouts are too large or too small. Its always some compromise. But what I more dislike is to add fixed sleeps in the code. In regular code its a absolute no-no. It shouldn't be needed to simulate testing some timing condition.

So it would be very good if Moq provides some mechanism that allows to verify that something happens in at least some time, but does not wait always for that time.

@stakx
Copy link
Contributor

stakx commented Oct 12, 2021

I see.

I am still wondering whether this needs to be in the core library... my gut feeling still objects to it. This might easily get overused and lead more novice users down a wrong path.

It should be possible to implement most of this capability in client code, or offer it in a contrib NuGet package. As a possible starting point, a naive implementation might look as follows:

async static Task Verify<T>(this Mock<T> mock, ..., TimeSpan timeout)
{
    const int GRANULARITY = 50;
    int elapsed = 0;
    while (elapsed < timeout.TotalMilliseconds)
    {
        try
        {
            mock.Verify(...);
            return;
        }
        catch
        {
            // perhaps collect all exceptions here, to be re-thrown after a timeout
            await Task.Delay(GRANULARITY);
            elapsed += GRANULARITY;
        }
    }
    throw new Exception("Timeout during verification");
}

Granted, that's a bit ugly, and not entirely correct semantically since Verify has some visible side effects re: mock state, but it might be good enough for many use cases.

@bormm
Copy link

bormm commented Oct 12, 2021

To be realistic: This request had no comment for 2 years.
Possible reasons for this:

  1. Nobody has this issue
  2. Nobody was thinking about how to solve it correctly
  3. They read this here and just added a sleep in the code and done

Anyway, it seems not very urgent and resources are always low.
I appreciate your feedback and the solution you offered now looks very usable and I will add that to my code for now. I don't like to call functions when I know it will throw all the time, but its ok for testing and hidden in the function. Much better than using a sleep. Thank you for that. Maybe consider to add it to the core functionality if more people asking for it.
I will give a feedback if I integrated it and if I was needed to change something.

@bormm
Copy link

bormm commented Oct 12, 2021

I created a extension class and put it here. Works good for me and allows doing things like that:

            var timeSpan = TimeSpan.FromSeconds(40);
            var asyncVerification = new List<Task>()
            {
                client.AsyncVerify(f => f.functionA(), Times.Once(), timeSpan),
                client.AsyncVerify(f => f.functionB(It.IsAny<Type>(), true), Times.AtLeastOnce(), timeSpan),
            };

            await Task.WhenAll(asyncVerification);

Any additional hint is welcome.
I don't know if it make sense to provide a nupkg for that, if there is so less requests for that.

@johannesmeyer
Copy link

I have the same problem. I know there are workarounds. Additionally to the general wait suggested above one could use a Setup().Callback() and set a TaskCompletionSource in the callback. The test can than wait for this TaskCompletionSource with a timeout. But this is really cumbersome. The timeout for verify like suggested in the extension from @bormm would be great to have in Moq itself. There is a reason why it was added to Mockito. In a world of distributed services it is quite normal to have such timeouts.

@kandaker
Copy link

To the point about others not having this need, FYI that I very much need it. I have a bunch of Task.Delay() calls in my tests. They'd be faster with a mechanism like what you describe. I implemented my own for now client-side. It'd be a great addition as part of Moq.

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

5 participants