Removing some clutter from ApexMocks syntax

[mischkes]

Hey guys,

I recently started working with apex-mocks and I love it. The only thing I didn't like and where it falls a bit short with regards to Mockito is the amount of clutter in the code. Some is due to the Salesforce restrictions (no generics...) but some seemed unnecessary.

So I played arround with it a bit and found three changes that improve this. The results can be seen here: https://github.com/mischkes/fflib-apex-mocks/tree/feature/poc-for-less-clutter#a-more-complicated-case

We are already using this code in production, but it is not yet library-grade. If you are interested I would be happy to improve the code and make some pull requests.

The changes are:

Create a Utility class that encapsulates ApexMocks and Match

• See https://github.com/mischkes/fflib-apex-mocks/blob/feature/poc-for-less-clutter/src/classes/fflib_Mock.cls
• this is actually the same way Mockito is doing it
• I would advise Projects using fflib-apex-mocks to rename this file (eg. to 'Mocks' as in the example or even 'Mk') to further reduce clutter. This is easily possible as there are no dependencies on fflib_Mock, so no other class has to be changed.
• This removes the clutter from constructing fflib_ApexMocks
• This removes clutter from writing fflib_Match
• This removes more clutter for more complex use-cases, e.g. by the use of fflib_Mock.Answer)
• This also creates a single-place-of use, which makes it easier for Newbies. "Just look at fflib_Mock"
• This also hides away some of the public Methods intended for use internally, like handleMethodCall or mockNonVoidMethod
• This change requires almost no change to the existing code. Currently I'm subclassing fflib_Match from fflib_Mock for convenienve, but event that could be wrapped. Which might be a good Idea to hide some internal methods like getAndClearMatchers.

Make start/stopStubbing unnecessary

• there is/was probably some use-case why those methods makes sense, but I didn't see it
• the main change was to always return a value in fflib_ApexMocks.mockNonVoidMethod, and move the actual parameter storing to fflib_ApexMocks.when
• This remove the clutter from the startStubbing() / stopStubbing() methods
• this is a minor breaking change, as mocking a method that was previously mocked to throw an exception will throw this exception during mocking. Mockito uses doReturn() statements for this case, which could be implemented
• this also removes the curent possibility to check for incomplete stubbing. I guess it could still be done by setting a flag inside when() and clearing it on then()

Record call history inside methods

• this change is probably mostly a matter of taste
• I never really liked the way complex argument checks are done in Mockito, with the ArgumentCaptor and all. And ApexMocks is following Mockito here.
• I really like the way jasmine is handling this and implemented it in the same way: simply storing the call history in the method (here: the Method mock object)
• this removes the clutter to create the captor, then verifying, and then getting the object from the captor
• this was also a minor change in the code

All in all these changes remove approx. 50% of the necessary mocking code, which I think is really a lot.

[dfruddffdc]

Great work - really appreciate the summary and your reasoning. I agree with almost everything above. I love the improved encapsulation and reduced clutter.

A few follow up points...

I would advise Projects using fflib-apex-mocks to rename this file (eg. to 'Mocks' as in the example or even 'Mk') to further reduce clutter. This is easily possible as there are no dependencies on fflib_Mock, so no other class has to be changed.

The fflib_ prefix is intentional, to avoid naming collisions when injected into another code-base as part of the Salesforce happy soup. I suspect a namespaced Unlocked Package will be the best way to give the best of both worlds.
Of course there's less chance of a collision with everything stored instead in a single utility class.

Make start/stopStubbing unnecessary - there is/was probably some use-case why those methods makes sense, but I didn't see it

I believe this prevents us from incrementing the call count, capturing arguments, throwing exceptions or invoking answers in the test setup phase. If we can still achieve all of these without explicit startStubbing and stopStubbing calls, I'm happy.

I really like the way jasmine is handling this

It's a core philosophical decision to be as close to Mockito as possible, to lower the barrier to entry and allow us to port Mockito features following their syntax. This might prove contentious but I'm ok with this in theory.

@agarciaodeian - any thoughts?

[mischkes]

The fflib_ prefix is intentional, to avoid naming collisions when injected into another code-base as part of the Salesforce happy soup. I suspect a namespaced Unlocked Package will be the best way to give the best of both worlds.

yes I understand this and would not remove the prefix. Using the single wrapper class just gives projects the option to rename fflib_Mock to something else.

Make start/stopStubbing unnecessary - there is/was probably some use-case why those methods makes sense, but I didn't see it

I believe this prevents us from incrementing the call count, capturing arguments, throwing exceptions or invoking answers in the test setup phase. If we can still achieve all of these without explicit startStubbing and stopStubbing calls, I'm happy.

That might be true. However, all your unit-tests where still running fine after my code change. Only the case where a method was mocked to throw an exception and then mocked a second time did no longer work, since the second invocation threw an exception.

Thanks for your reply. Do let me know if you are interested in parts of the change, then I will create a pull request with the polished code. I believe all changes could be added without causing any breaking changes.

[mischkes]

Since I did not hear from @dfruddffdc and @agarciaodeian again I guess this is not something you want to follow right now. So I will just fork the project, which makes a lot of sense anyway. Most of the code-generation stuff seems deprecated and in a fork I can simply remove it 😇