Refactor Makes “Perfect”

I was recently doing a unit test of some Typescript logic for a Form in Dynamics 365 for Customer Engagement (Hereafter referred to as CRM).  Our CRM instance maintains a list of Localities and when a user selects a State/County it should perform different actions based on the number of localities found for the state/county combination:

• 1 found: Set the Lookup to the single Locality 
• 1+ found: Filter the Localities Lookup by state and county
• 0 found: Filter the Localities Lookup only by state and allow the user to choose.

In writing the test case for the first lookup, this is what I came up with at first:

it("should set locality if a single locality exists for the state/county", async (done) => {
    spyOn(restLibMock.CrmWebApiLib, "getList").and.callFake((r) => CrmWebApiLibStubs.getList.defaultValuesUsingSelect(r, 1));
    new Squire()
        .mock(restLibMock.path, restLibMock)
        .require(["dfnd_/scripts/lead/CustomerStage"], async (m: { CustomerStage: typeof CustomerStage }) => {
            await m.CustomerStage.setLocalityFilter();
            expect(CommonLib.getValue(LeadCommon.fields.locality)).toBeTruthy("because the locality should have been set because only one locality exists for the given city/state");
            done();
        });
});

It works, but it sucks:

• It’s really confusing (I’m going to explain it line by line, and you still probably won’t understand it)
• A lot will need to be duplicated to handle the other 2 cases.

First thing it does is setup a fake for the CrmWebApiLib module in restLibMock using a Jasmine Spy.  The fake “defaultValuesUsingSelect” just returns an result where the fields selected are populated with their own values, and the 1 just means that one item should be returned:

spyOn(restLibMock.CrmWebApiLib, "getList").and.callFake((r) => CrmWebApiLibStubs.getList.defaultValuesUsingSelect(r, 1));

The next 3 lines are setup for injecting the mock as CrmWebApiLib module, into CustomerStage.  It accepts a callback with a parameter that has a CustomerStage Property that has the mock CrmWebApiLib injected:

new Squire()
    .mock(restLibMock.path, restLibMock)
    .require(["scripts/lead/CustomerStage"], async (m: { CustomerStage: typeof CustomerStage }) => {

The final lines are the actual test that needs to be performed.  Call setLocalityFilter, and verify that that Locality attribute has had it’s value set:

await m.CustomerStage.setLocalityFilter();
expect(CommonLib.getValue(LeadCommon.fields.locality)).toBeTruthy("because the locality should have been set because only one locality exists for the given city/state");
done();

Even if you do understand how this works now, I’d be willing to bet the “future you” who looks at it in 5 days, will require significant time/energy to re-figure out what it’s doing.  No bueno.

Here is the refactored version with the helper method:

it("should set locality if a single locality exists for the state/county", async (done) => {
    const sut = await getCustomerStageWithMockedCrmWeb(1);
    await sut.setLocalityFilter();
    expect(CommonLib.getValue(LeadCommon.fields.locality)).toBeTruthy("because the locality should have been set because only one locality exists for the given city/state");
    done();
});

function getCustomerStageWithMockedCrmWeb(numResults: number): Promise<typeof CustomerStage> {
    return new Promise((resolve) => {
        spyOn(restLibMock.CrmWebApiLib, "getList").and.callFake((r) => CrmWebApiLibStubs.getList.defaultValuesUsingSelect(r, numResults));
        new Squire()
            .mock(restLibMock.path, restLibMock)
            .require(["scripts/lead/CustomerStage"], (m: { CustomerStage: typeof CustomerStage }) => {
                resolve(m.CustomerStage);
            });
    });
}

The actual test method is now dead simple, Get the SUT (Subject Under Test), set the locality filter, and assert that the locality was set and the new helper method “getCustomerStageWithMockedCrmWeb” can now be reused in the other three calls with minimal coding required/repeated.  If “future you” looked at that code in 5 months, it would immediately make sense.    Refactor, Refactor, Refactor.