Adding a `UniversalMocker.validateAllMocks()` api to prevent user error

[surajp]

I am thinking of adding a UniversalMocker.validateAllMocks() that can be used to validate if all mocks have been properly used. Since the api is more verbose than needed, in favor of readability, there is a potential for developers to unintentionally leave method calls incomplete. (eg: mock.assertThat().method('methodname')) . This call would insulate developers against such errors. Comments welcome

[Thieveryshoe]

Having used a mocking framework like this before, I highly recommend something like that. When you're teaching someone the framework or teaching mocking in general, forgetting a method in the chain is a common occurrence. And again, from experience, once you're familiar with the framework, forgetting a method from time to time does happen and it's a time sink figuring out why a test isn't working as expected without any sort of compiler support.

[jamessimone]

I think it's tough because if they don't call this validateAllMocks method, the developer is in the same boat as before - the test passes and there's no way to get immediate feedback on the invalid state of the mock. If they do call it, assuming the format for the API stays the same as it is now, validateAllMocks ends up feeling super verbose. But perhaps you both are thinking about a radically different publicly-facing API that will slim down the below.

Are you thinking that the full API will then look something like:

mock.assertThat().method(mockedMethodName).wasCalled(1, UniversalMocker.Times.EXACTLY).validateAllMocks() ?

[surajp]

I think it's tough because if they don't call this validateAllMocks method, the developer is in the same boat as before - the test passes and there's no way to get immediate feedback on the invalid state of the mock. If they do call it, assuming the format for the API stays the same as it is now, validateAllMocks ends up feeling super verbose. But perhaps you both are thinking about a radically different publicly-facing API that will slim down the below.

Are you thinking that the full API will then look something like:

mock.assertThat().method(mockedMethodName).wasCalled(1, UniversalMocker.Times.EXACTLY).validateAllMocks() ?

No, I was thinking of a single static method call at the end of each test method. So, call UniversalMethod.validateAllMocks() in exactly that fashion at the end of each test method

[jamessimone]

well, at least it's on a new line then!

[Thieveryshoe]

Run into the same problem of, have to call the static method. But it's a start and better than nothing. Once we have a bit of a more finalized product, a code snippet could be included in the repo that has the call built in?

[surajp]

Run into the same problem of, have to call the static method. But it's a start and better than nothing. Once we have a bit of a more finalized product, a code snippet could be included in the repo that has the call built in?

I'm not sure what you mean by a snippet that has the call built in. I assume you mean we just add that line to the sample code and Readme? Anyway, I see this really being an issue only with asserting the method calls. For setting up mocks, if you don't finish the api call chain, your tests should fail. And for inspecting the method arguments, if you don't complete the api calls, you won't get the values back. And as for asserting method call counts, with the fix that shoe suggested, it'd be really unusual that developer leaves the api calls incomplete, since they will always remember to specify how many times they expect the method to have been called, which is where the chain ends now, as opposed to an extra .times() call that we had before?

[jamessimone]

I think shoe was just saying adding the extra call to the Readme.

The API is definitely in a better state where the chain ends now! I agree that it would be unusual for the dev to leave the API calls incomplete since they have to specify the number of times in a strongly-typed fashion.

[Thieveryshoe]

I suspect a test will look something like:

@IsTest
private static it_should_test() {
  //...
  UniversalMocker.validateAllMocks();
  mock.assertThat().method(mockedMethodName).wasCalled(1, UniversalMocker.Times.EXACTLY);
}

I'm assuming we could provide a VS Code snippet that includes the validateAllMocks call.

[surajp]

UniversalMocker.validateAllMocks();

@IsTest
private static it_should_test() {
  //...
  mock.assertThat().method(mockedMethodName).wasCalled(1, UniversalMocker.Times.EXACTLY);
//...
UniversalMocker.validateAllMocks();
}

The validateAllMocks call will be at the end of the test method.

[Thieveryshoe]

UniversalMocker.validateAllMocks();

@IsTest
private static it_should_test() {
  //...
  mock.assertThat().method(mockedMethodName).wasCalled(1, UniversalMocker.Times.EXACTLY);
//...
UniversalMocker.validateAllMocks();
}

The validateAllMocks call will be at the end of the test method.

Won't the test fail in the .assertThat() call before the validateAllMocks() call is made?

[surajp]

Won't the test fail in the .assertThat() call before the validateAllMocks() call is made?

The test will fail if the method was not called 1 time.

The validateAllMocks() call will fail if developer does not complete a specific api chain. for eg: if they just do mock.assertThat().method(mockedMethodName) and leave it at that

[justin-lyon]

What if in order to execute the developer is required to execute a .run() method?

If all chains must end with a single method call, you have the context in the implementation of run to validate this

Rather than trying to solve for all at once, put the onus on the Dev, through the public API

[surajp]

What if in order to execute the developer is required to execute a .run() method?

If all chains must end with a single method call, you have the context in the implementation of run to validate this

Rather than trying to solve for all at once, put the onus on the Dev, through the public API

That is a good suggestion, and I can see the potential. However, that would also cause the api to be more verbose than is necessary, imo. What does everyone else think?

[jamessimone]

I think I like a smaller method like run that then removes the need for a second, static method to be called.

The unfortunate fact of the matter is that, at the end of the day, there is no clean way to be more straightforward with providing feedback to the developer - ie throwing an error unless the “validate” method, whatever it is called, is used - so unless we can get around that through some brilliant internals, I’ll favor a terse, fluent API.

[Thieveryshoe]

I think I like a smaller method like run that then removes the need for a second, static method to be called.

The unfortunate fact of the matter is that, at the end of the day, there is no clean way to be more straightforward with providing feedback to the developer - ie throwing an error unless the “validate” method, whatever it is called, is used - so unless we can get around that through some brilliant internals, I’ll favor a terse, fluent API.

I'm in agreement here. Unless we can get it exactly the way we'd like it, it's better to not implement a run command and keep the chain as terse as possible.

[surajp]

To summarize, it sounds like @jamessimone and @jlyon87 are in favor of a run method at the end of each api chain, whereas @Thieveryshoe and I are not sold on it entirely. Hope I got that right. So, I am going to release the class in its current form, and park this till we can get inputs from some others as well, since I feel this is less of a problem than it was before since we fixed the times() call

[jamessimone]

Works for me!