Matcher sObjectWith doesn't tell you why the match failed

[cropredyHelix]

While matcher sObjectWith is pretty useful, if you are verifying several fields, it doesn't tell you which one failed and why. This is a step backwards from traditional asserts and makes debugging more difficult

Example 1

((fflib_SobjectUnitOfWork) mocks.verify(mockUow,
           mocks.times(1).description('some modestly helpful text')))
        .registerDirty(fflib_Match.sObjectWith(new map<SObjectField,Object> {
                        OrderItem.ID => mockOrders.OrderItems[0].Id,
                        OrderItem.Foo__c => expectedFoo,
                        OrderItem.Bar__c => expectedBar}));

This generates

fflib_ApexMocks.ApexMocksException: Expected : 1, Actual: 0 -- 
Wanted but not invoked: fflib_SObjectUnitOfWork__sfdc_ApexStub.registerDirty(SObject). 
some modestly helpful text.

So, what is a developer to do?

• Is the ID a mismatch?
• Is the Foo__c a mismatch?
• Is the Bar__c a mismatch?

The developer has to go the system.debug route or futz around with argument capture which is heavy syntax.

The most useful behavior would be for sObjectWith to look through all of the fields and concatenate all the mismatches into a string for display in the exception message. Thus, the developer could track down and fix all of the offending items, not discover them 1x1.

Root cause is the method return for a fflib_MatcherDefinitions.someMatcher. These methods simply return true or false and hence lose (valuable) information.

I can think of some hack solutions - like sObjectWith deferring the return of false until after it has emitted a System.debug(LoggingLevel.FATAL,the concatenation of mismatches) so at least there is a fast way to find the issues without recompiling anything; but better would be to surface in the exception message.

[dfruddffdc]

Good point. A Boolean doesn't tell you much beyond yay or nay. I'm reluctant to modify the fflib_IMatcher interface, as a breaking change for anyone whose defined a custom matcher before. Or to splinter into Matcher v1 and v2.

I wonder if the best solution would be to store a list of match failure reasons somewhere else, e.g.

// In fflib_Match
public static final String[] matchFailureReasons = new String[]{};

Then log the contents of the array out when throwing the verify exception.

[cropredyHelix]

"I wonder if the best solution would be to store a list of match failure reasons somewhere else, e.g. //In fflib_IMatch public static final String[] matchFailureReasons = new String[]{}

Then log the contents of the array out when throwing the verify exception."

That is workable but I was afraid to suggest it as it didn't seem to pass the otherwise O-O implementation of the ApexMocks package. I imagined something like a registerMatcherFailDetail(someObj) that would be a callback from the code that does the verify exception to grab the matcher verify detail.

On Fri, Jan 26, 2018 at 7:56 AM, David Frudd notifications@github.com wrote:

Good point. A Boolean doesn't tell you much beyond yay or nay. I'm reluctant to modify the fflib_IMatcher interface, as a breaking change for anyone whose defined a custom matcher before. Or to splinter into Matcher v1 and v2.

I wonder if the best solution would be to store a list of match failure reasons somewhere else, e.g. ```java //In fflib_IMatch public static final String[] matchFailureReasons = new String[]{};

Then log the contents of the array out when throwing the verify exception.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/financialforcedev/fflib-apex-mocks/issues/58#issuecomment-360823790, or mute the thread https://github.com/notifications/unsubscribe-auth/AYdgA0UrfBsHyW5MmJtYXPm4JGYdbUyCks5tOfW3gaJpZM4QcoPh .

-- Eric Kintzer Salesforce Architect 1 Circle Star Way, Floor 2, San Carlos, CA 94070 650 533 5619 (m) www.helix.com | We're hiring https://www.helix.com/careers

[cropredyHelix]

Thought some more about this

If you call uow.registerXXX(someSobject) twice and then use verify Matcher SObjectWith with a map of sobject [0]'s field tokens => expected values, the matcher will record in this static variable match failures for sobject[1] and vice-versa. So, the display of match failures will actually show failures to match both sobjects. This is expected but perhaps counterintuitive, especially if the map of tokens contains the ID field (as is likely for registerDirty or registerDelete). Thus the display of the verify error would need to be clearly formatted

"Did not match map of sobject[0] - fields that failed to match map of sobject[1] - fields that failed to match "

This could get unwieldy if the list of Sobjects passed to uow.registerXXX is large and/or the map size is large as a match failure will fail every sobject

Perhaps some cleverness in the matcher to avoid reporting field-by-field failures if the ID field doesn't match? Hence reducing the unwieldy-ness down to just registerNew (and even that could be somewhat mitigated if the Name field is included in the SobjectWith map of field tokens=> expected values)

On Fri, Jan 26, 2018 at 9:47 AM, Eric Kintzer eric.kintzer@helix.com wrote:

"I wonder if the best solution would be to store a list of match failure reasons somewhere else, e.g. //In fflib_IMatch public static final String[] matchFailureReasons = new String[]{}

Then log the contents of the array out when throwing the verify exception."

That is workable but I was afraid to suggest it as it didn't seem to pass the otherwise O-O implementation of the ApexMocks package. I imagined something like a registerMatcherFailDetail(someObj) that would be a callback from the code that does the verify exception to grab the matcher verify detail.

On Fri, Jan 26, 2018 at 7:56 AM, David Frudd notifications@github.com wrote:

Good point. A Boolean doesn't tell you much beyond yay or nay. I'm reluctant to modify the fflib_IMatcher interface, as a breaking change for anyone whose defined a custom matcher before. Or to splinter into Matcher v1 and v2.

I wonder if the best solution would be to store a list of match failure reasons somewhere else, e.g. ```java //In fflib_IMatch public static final String[] matchFailureReasons = new String[]{};

Then log the contents of the array out when throwing the verify exception.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/financialforcedev/fflib-apex-mocks/issues/58#issuecomment-360823790, or mute the thread https://github.com/notifications/unsubscribe-auth/AYdgA0UrfBsHyW5MmJtYXPm4JGYdbUyCks5tOfW3gaJpZM4QcoPh .

-- Eric Kintzer Salesforce Architect 1 Circle Star Way, Floor 2, San Carlos, CA 94070 650 533 5619 <(650)%20533-5619> (m) www.helix.com | We're hiring https://www.helix.com/careers

-- Eric Kintzer Salesforce Architect 1 Circle Star Way, Floor 2, San Carlos, CA 94070 650 533 5619 (m) www.helix.com | We're hiring https://www.helix.com/careers

[ClayChipps]

@cropredyHelix Have you had any more experience overcoming this shortcoming? I have started implementing mocks and this is the biggest pitfall I've encountered so far. I know the discussion is a bit stale but looking to potentially contribute to improve this.

[cropredyHelix]

@dfruddffdc So, more thinking ...

Assume a new Class; methods are static because all the knowledge of diagnostics lies inside inner classes in fflib_MatcherDefinition and there's no reference back to an object of type fflib_ApexMocks within those inner classes.

public class fflib_MatcherDiagnostics {
   static String[] diagnostics = new List<String>();

  public static void add(String diagnostic) {diagnostics.add(diagnostic);}
  public static String toString() {return String.join(diagnostics;}
  public static void clear() {diagnostics.clear();}
}

fflib_MethodVerifier is the place to display the matcher diagnostics

protected void throwException(
        fflib_QualifiedMethod qm,
        String inOrder,
        Integer expectedCount,
        String qualifier,
        Integer methodCount,
        String customAssertMessage)
    {
        String assertMessage = 'Wanted but not invoked: ' + qm + '.';

        if(customAssertMessage != null)
        {
            assertMessage = assertMessage + ' ' + customAssertMessage + '.';
        }

                assertMessage += fflib_MatcherDiagnostics.toString();  // get diagnostics, if any
        String message = '{0}Expected : {1}{2}, Actual: {3} -- {4}';

        List<String> errorParameters = new List<String>
        {
            inOrder, expectedCount + '', qualifier, methodCount + '', assertMessage
        };

        throw new fflib_ApexMocks.ApexMocksException(String.format(message, errorParameters));
    }

The static list of diagnostics is cleared before every verify

public void verifyMethodCall(fflib_InvocationOnMock mockInvocation, fflib_VerificationMode verificationMode)
    {
        validateMode(verificationMode);
                fflib_MatcherDiagnostics.clear();  // clear diags from any previous verify
        verify(mockinvocation.getMethod(), mockinvocation.getMethodArgValues(), verificationMode);
    }

and finally, in the fflib_MatcherDefinition inner classes, for those where relevant to capture diagnostics (example ListIsEmpty

    public class ListIsEmpty implements fflib_IMatcher
    {
        public Boolean matches(Object arg)
        {

            if (arg != null && arg instanceof List<Object>) {
                             if (((List<Object>)arg).isEmpty()) {
                                  return true;
                             }
                             fflib_MatcherDiagnostics.add('ListIsEmpty does not match: ' + String.valueOf(arg));  // incoming arg
                             return false;
                       }
                      return false;
        }
    }

so, if ListIsEmpty returns false on an actual list, the throwException method will fetch the diagnostics and include that in testmethod output so the developer can see that, rather than an expected empty list, the actual list used by the code-under-test will be displayed along with any custom assert message. This will save time in debugging as often, by seeing the real method args, once can immediately understand how one's code is in error (or possibly, the testmethod isn't constructed correctly)

Needless to sav, I'm not nuts about using static variables but I don't see how else to communicate detailed matching errors back to the place where the exception is thrown with the wanted/received + custom assert message

[dfruddffdc]

Actually, I've fixed this in FinancialForce's internal Apex Mocks repo. We should have done a better job keeping the forked repos in sync, but I fear they have now diverged too far to make porting over the changes trivial.

The key to solving the problem was that we only need to log out the actual and expected args when the verify fails. And all of the information is stored internally in the fflib_ApexMocks instance.

This is lifted straight from the internal release notes.

Better messages when verify fails

Prior to this release, assert messages looked like this:

• Expected : 1, Actual: 0 -- Wanted but not invoked:

  AccountServiceWorker__sfdc_ApexStub.appendToName(String, List<Account>).

• Now they look like this:

  EXPECTED COUNT: 1
  ACTUAL COUNT: 0
  METHOD: IAccountServiceWorker__sfdc_ApexStub.appendToName(String, List<Account>)
  ---
  ACTUAL ARGS: ("HOT", [{"attributes":{"type":"Account"},"Name":"One"},{"attributes":{"type":"Account"},"Name":"Two"}])
  ---
  EXPECTED ARGS: [equals "Hot"], [list with SObjectType Account and at least one record with fields {"Name":"Three"}]

Notes:

Describing actual args

• In the first instance, Apex Mocks will attempt to JSON.serialize the actual arguments. For SObjects and other standard Salesforce classes, this means the values will not be truncated.
• If JSON.serialize fails, then Apex Mocks will coerce the Object to a String by concatenating it to another String (i.e. using its toString() method).

Note: Some objects cannot be JSON serialized, e.g. those with circular references.

Describing expected args

• If you are not using matchers, then the expected arguments will be listed - using JSON.serialize where possible, toString() otherwise.
• If you are using matchers, the matchers will be listed - using toString().
• You may wish to override toString to provide a better description of your custom matchers.

[dfruddffdc]

The changes needed are:

• Change the verify methods to gather this info and log it out in the Apex Mocks exception
• Add fflib_ApexMocks toString() method
  • otherwise stringifying the matchers can cause spurious internal errors, where the matchers and fflib_ApexMocks have circular references to each other - that was tricky to get to the bottom of!!
• (OPTIONAL - I will do this) Change all internal matchers to override toString
  • So that they give descriptive explanations of expected values
• (OPTIONAL - I won't do this) Move all matcher code into a subdirectory classes/match
  • To make things a bit tidier
• (OPTIONAL - I won't do this) - Break monolithic fflib_MatcherDefinitions into separate files
  • To make things a bit tidier
  • Internally, now broken down into fflib_MatcherDefinitionsNumerics, fflib_MatcherDefinitionsStrings etc.

I'll get this code copied over this week.

The advantage to this approach is that it's a non-breaking change. If anyone has created custom matchers, usually their default toStrings are good enough to diagnose failed asserts. Though they can optionally update the toStrings to fall in line with the convention.

E.g. the DateTimeAfter matcher's toString

public override String toString()
{
    if (inclusive)
    {
        return '[on or after ' + JSON.serialize(fromDateTime) + ']';
    }
    else
    {
        return '[after ' + JSON.serialize(fromDateTime) + ']';
    }
}

[cropredyHelix]

@dfruddffdc - this is great! I think I see how this will work -- the verify methods call toString in the matchers and each matcher keeps local variables to record matcher inputs v expected and such local variable is used to construct the return value of toString()?

I can't wait to try this.

[dfruddffdc]

FYI https://github.com/dfruddffdc/fflib-apex-mocks/tree/feature/better-verification-messages

• [x] Change the verify methods to gather this info and log it out in the Apex Mocks exception
• [x] Add fflib_ApexMocks toString() method
• [x] Change all internal matchers to override toString
• [x] Update readme

More tomorrow...

[ClayChipps]

@ImJohnMDaniel @daveespo Let's close this since #96 was merged?