Vjeko.com — ideas in the cloud

Directions EMEA 2023 demo – decoupling base app

· Testing

Here we go. As promised, I’ll start with the demo I delivered at my “Mythbusting code coverage” session at Directions EMEA 2023 in Lyon.

Over the course of this year, I have talked about testability at 12 sessions at 7 events, 6 public one-day and two-day workshops, and 4 private direct-to-partner one-day workshops. At all those sessions and workshops, almost all of my demos are “made up”: they are code examples written by me to illustrate the concept I talk about in an easy-to-follow way. I think they work quite well to show you what my goal is and what and how I am doing what I am doing.

And yet, I have faced criticism that goes along the lines of “all nice, but this is oversimplified fake examples, and it’s easy to create those examples for just about any concept you want; why don’t you do it on a real-life example, it’s probably not going to be possible.”

Well, I disagree, and that’s why for this event I have decided to add one more example to my example suite, and show how you can apply all the concepts I talk about to a piece of existing base app code, all while not breaking anything and not having to change any other area of the code that depends on that refactored code. Here you can find that example, explained step by step.

While I would love to show all the stuff I talk about on a really complex example like Sales-Post codeunit, I needed a piece of code which is short enough to be able to demo in 10-15 minutes max, and yet a good representation of the problems that hinder true unit testing – the tight coupling that most of AL code is so full of.

I found this example in codeunit 1405 “Purch. Inv. Header – Edit”. I took that code minus namespaces, because at the time of me demoing it and writing this post, there is an issue with code coverage tools that shift the reported lines off for any namespace or using declarations. I have taken all of this code, and created a one-on-one copy of it.

All of the code I demoed during the session is available on my GitHub, here: https://github.com/vjekob/emea2023/

For your convenience here’s the entire codeunit:

codeunit 50102 PurchInvHeaderEdit
{
    Permissions = TableData "Purch. Inv. Header" = rm;
    TableNo = "Purch. Inv. Header";

    trigger OnRun()
    var
        PurchInvHeader: Record "Purch. Inv. Header";
    begin
        PurchInvHeader.Copy(Rec);
        PurchInvHeader.ReadIsolation(IsolationLevel::UpdLock);
        PurchInvHeader.Find();
        PurchInvHeader."Payment Reference" := Rec."Payment Reference";
        PurchInvHeader."Payment Method Code" := Rec."Payment Method Code";
        PurchInvHeader."Creditor No." := Rec."Creditor No.";
        PurchInvHeader."Ship-to Code" := Rec."Ship-to Code";
        PurchInvHeader."Posting Description" := Rec."Posting Description";
        OnBeforePurchInvHeaderModify(PurchInvHeader, Rec);
        PurchInvHeader.TestField("No.", Rec."No.");
        PurchInvHeader.Modify();
        Rec.Copy(PurchInvHeader);

        UpdateVendorLedgerEntry(Rec);

        OnRunOnAfterPurchInvHeaderEdit(Rec);
    end;

    local procedure UpdateVendorLedgerEntry(PurchInvHeader: Record "Purch. Inv. Header")
    var
        VendorLedgerEntry: Record "Vendor Ledger Entry";
    begin
        if not GetVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader) then
            exit;
        VendorLedgerEntry."Payment Method Code" := PurchInvHeader."Payment Method Code";
        VendorLedgerEntry."Payment Reference" := PurchInvHeader."Payment Reference";
        VendorLedgerEntry."Creditor No." := PurchInvHeader."Creditor No.";
        VendorLedgerEntry.Description := PurchInvHeader."Posting Description";
        OnBeforeUpdateVendorLedgerEntryAfterSetValues(VendorLedgerEntry, PurchInvHeader);
        Codeunit.Run(Codeunit::"Vend. Entry-Edit", VendorLedgerEntry);
    end;

    local procedure GetVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header"): Boolean
    begin
        if PurchInvHeader."Vendor Ledger Entry No." = 0 then
            exit(false);
        VendorLedgerEntry.ReadIsolation(IsolationLevel::UpdLock);
        exit(VendorLedgerEntry.Get(PurchInvHeader."Vendor Ledger Entry No."));
    end;

    [IntegrationEvent(false, false)]
    local procedure OnBeforePurchInvHeaderModify(var PurchInvHeader: Record "Purch. Inv. Header"; PurchInvHeaderRec: Record "Purch. Inv. Header")
    begin
    end;

    [IntegrationEvent(false, false)]
    local procedure OnBeforeUpdateVendorLedgerEntryAfterSetValues(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header")
    begin
    end;

    [IntegrationEvent(false, false)]
    local procedure OnRunOnAfterPurchInvHeaderEdit(var PurchInvHeader: Record "Purch. Inv. Header")
    begin
    end;
}

Cool, so what is it that I have to say about this piece of code? What’s wrong with it and how can it be improved?

Well, it’s simple enough, there are only three functions all seem pretty simple. Let me start by explaining what the code does and how, and then I’ll move on to the test I wrote for it.

This codeunit allows you to modify an existing posted purchase invoice header by assigning new values to some fields. It first loads the header from the database, then assigns new values, then modifies them. Then, it looks up if there is a vendor ledger entry related to that invoice, and if it is, the same changes are written to that entry as well Finally, it calls another codeunit (a dependency) to perform some more vendor-ledger-entry-related work. All simple enough.

The problems in this code are exposed once you want to test it. So here’s how I test it:

codeunit 50103 "Test PurchaseInvHeaderEdit"
{
    Subtype = Test;

    var
        Assert: Codeunit Assert;
        LibraryPurchase: Codeunit "Library - Purchase";
        LibrarySales: Codeunit "Library - Sales";
        LibraryERM: Codeunit "Library - ERM";
        LibraryUtility: Codeunit "Library - Utility";

    [Test]
    procedure TestEdit()
    var
        PurchaseHeader: Record "Purchase Header";
        PurchInvHeader, PurchInvHeader2 : Record "Purch. Inv. Header";
        PaymentMethod: Record "Payment Method";
        Customer: Record Customer;
        ShipToAddress: Record "Ship-to Address";
        VendorLedgEntry: Record "Vendor Ledger Entry";
        PurchInvHeaderEdit: Codeunit PurchInvHeaderEdit;
        PostedInvoiceNo: Code[20];
    begin
        // [GIVEN] A purchase invoice
        LibraryPurchase.CreatePurchaseInvoice(PurchaseHeader);

        // [GIVEN] A customer
        LibrarySales.CreateCustomer(Customer);

        // [GIVEN] A ship-to address
        LibrarySales.CreateShipToAddress(ShipToAddress, Customer."No.");

        // [GIVEN] Purchase header is for the customer
        PurchaseHeader.Validate("Sell-to Customer No.", Customer."No.");
        PurchaseHeader.Modify(true);

        // [GIVEN] Post the invoice
        PostedInvoiceNo := LibraryPurchase.PostPurchaseDocument(PurchaseHeader, true, true);

        // [GIVEN] Posted purchase invoice header
        PurchInvHeader.Get(PostedInvoiceNo);

        // [GIVEN] A payment method
        LibraryERM.CreatePaymentMethod(PaymentMethod);

        // [GIVEN] Changing some fields
        PurchInvHeader."Payment Reference" := LibraryUtility.GenerateRandomText(50);
        PurchInvHeader."Payment Method Code" := PaymentMethod.Code;
        PurchInvHeader."Creditor No." := LibraryUtility.GenerateRandomText(20);
        PurchInvHeader."Ship-to Code" := ShipToAddress.Code;
        PurchInvHeader."Posting Description" := LibraryUtility.GenerateRandomText(100);

        // [WHEN] Running purchase invoice header edit
        PurchInvHeaderEdit.Run(PurchInvHeader);

        // [THEN] Purchase invoice record is updated
        PurchInvHeader2.Get(PostedInvoiceNo);
        Assert.AreEqual(PurchInvHeader."Payment Reference", PurchInvHeader2."Payment Reference", 'Payment Reference not updated');
        Assert.AreEqual(PurchInvHeader."Payment Method Code", PurchInvHeader2."Payment Method Code", 'Payment Method Code not updated');
        Assert.AreEqual(PurchInvHeader."Creditor No.", PurchInvHeader2."Creditor No.", 'Creditor No. not updated');
        Assert.AreEqual(PurchInvHeader."Ship-to Code", PurchInvHeader2."Ship-to Code", 'Ship-to Code not updated');
        Assert.AreEqual(PurchInvHeader."Posting Description", PurchInvHeader2."Posting Description", 'Posting Description not updated');

        // [THEN] Vendor ledger entry updated
        VendorLedgEntry.Get(PurchInvHeader2."Vendor Ledger Entry No.");
        Assert.AreEqual(PurchInvHeader2."Payment Reference", VendorLedgEntry."Payment Reference", 'Payment Reference not updated');
        Assert.AreEqual(PurchInvHeader2."Payment Method Code", VendorLedgEntry."Payment Method Code", 'Payment Method Code not updated');
        Assert.AreEqual(PurchInvHeader2."Creditor No.", VendorLedgEntry."Creditor No.", 'Creditor No. not updated');
        Assert.AreEqual(PurchInvHeader2."Posting Description", VendorLedgEntry.Description, 'Posting Description not updated');
    end;
}

Good. So again, what exactly is the problem, isn’t this how we are supposed to write our tests? GIVEN this and that stuff in the database, WHEN calling some BC feature, THEN this and that must be validated.

The code I want to test needs a posted purchase invoice. So, my first given is a purchase invoice. Since I want to test all aspects of the codeunit, one of which is changing of the ship-to address for the customer to which I would eventually ship the stuff I purchased with this purchase invoice, I also need a customer. So my second given is a customer, and my third given is a ship-to address, and then my third given is that this purchase invoice is earmarked for this customer. My fifth given is a posted purchase invoice, so I post it My sixth given is the posted purchase invoice itself, so I read it from the database. I will be changing the payment method on the posted invoice, so my next given is a new payment method. My final given is the changes I want to do, so I prepare the changes I want to write to the posted purchase invoice header and related vendor ledger entry. In total, I have eight givens, that together take quite some time to run, a lot database reads and writes are involved, and a lot of rolling back once tests are finished.

All of these givens are fine if I am writing an integration test. However, I don’t want to write an integration test. I want to just test this codeunit and what it does, and I hate the fact that I need to do all of this preparation just to make sure that I can edit a posted purchase invoice header and a vendor ledger entry. But I don’t have this choice. Unfortunately, the functional code I have in front of me that I want to test has so many problems that hinder any attempt to write proper and reliable unit tests for it.

Another point that I want to make is that most of the givens I have are in fact fixtures. Fixtures are something everyone does in every language in every environment on every project where tests are done in every corner of the world. Per se, fixtures aren’t bad. However, it’s bad to overly rely on fixtures to write tests. Especially unit tests. Ideally, unit tests should have zero fixtures. The moment you feel a need for a fixture in your test, you are probably doing something wrong in the code you are testing.

So let’s dissect our functional code and see what’s wrong about it and what it is that we can improve. I’ll start with the OnRun trigger:

trigger OnRun()
    var
        PurchInvHeader: Record "Purch. Inv. Header";
    begin
        PurchInvHeader.Copy(Rec);
        PurchInvHeader.ReadIsolation(IsolationLevel::UpdLock);
        PurchInvHeader.Find();

        PurchInvHeader."Payment Reference" := Rec."Payment Reference";
        PurchInvHeader."Payment Method Code" := Rec."Payment Method Code";
        PurchInvHeader."Creditor No." := Rec."Creditor No.";
        PurchInvHeader."Ship-to Code" := Rec."Ship-to Code";
        PurchInvHeader."Posting Description" := Rec."Posting Description";

        PurchInvHeader.TestField("No.", Rec."No.");
        PurchInvHeader.Modify();

        Rec.Copy(PurchInvHeader);

        UpdateVendorLedgerEntry(Rec);
    end;

For sake of simplicity and clarity, I took the event invocations out so I can focus on functionality. Also, for same reasons, I have added and removed some line breaks to show obvious stages this code goes through.

The biggest problem of this code, and in fact most of AL code out there – and that’s why this simple example is a good representation of all AL code out there – is that everything is so tightly coupled: data in the database, business logic, dependencies., all of it is in the same place. Tight or loose coupling has nothing to do with the number of lines of code or complexity – you can have it tightly coupled in two lines of code, and you can have it loosely coupled in hundred of lines, this coupling and complexity that stems from it have nothing to do with how short or long your code is.

First of all, this code retrieves the header to modify by reading it from the database. That’s my problem #1. For me to test this piece of code, I need to write something to the database first, and since it’s a posted document, writing that to the database involves a lot of data and processing which will make my tests slow. The first thing I’ll want to do is to delegate this work into a function of its own, like this:

internal procedure FindPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header")
    begin
        PurchInvHeader.Copy(FromPurchInvHeader);
        PurchInvHeader.ReadIsolation(IsolationLevel::UpdLock);
        PurchInvHeader.Find();
    end;

Trust me on this one: you can never go wrong with delegating. Anything you can delegate, you should delegate, always. So that’s why I did it. It’s a logical unit of code that does one thing, has a very narrow, specific task to do, and it deserves to have a function of its own.

The next chunk of code I want to delegate is – you guessed it right – this:

internal procedure EditPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header")
    begin
        PurchInvHeader."Payment Reference" := FromPurchInvHeader."Payment Reference";
        PurchInvHeader."Payment Method Code" := FromPurchInvHeader."Payment Method Code";
        PurchInvHeader."Creditor No." := FromPurchInvHeader."Creditor No.";
        PurchInvHeader."Ship-to Code" := FromPurchInvHeader."Ship-to Code";
        PurchInvHeader."Posting Description" := FromPurchInvHeader."Posting Description";
    end;

Wait, but am I not missing something. That PurchInvHeader.Modify piece of code, shouldn’t I do it here as well, I mean this is where I am editing the purchase invoice header, why not putting that piece of code here into this function too? Well, that’s the point of it all. I don’t want data access code intermingled and mish-mashed with my business logic. This is probably the most important point I am going to make in this post, but don’t, just don’t, ever, put data access code into the same function where your business logic is. No matter how simple business logic is (and in this example above, it’s ridiculously simple), don’t add that modify, or insert, or delete, next to the place where you assign values. That code does not belong together. Call me crazy if you want, but give me the benefit of the doubt, and read on.

Obviously, I need to do this now:

internal procedure ModifyPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header")
    begin
        PurchInvHeader.TestField("No.", FromPurchInvHeader."No.");
        PurchInvHeader.Modify();
    end;

Now my trigger code looks like this:

trigger OnRun()
    var
        PurchInvHeader: Record "Purch. Inv. Header";
    begin
        FindPurchInvHeader(PurchInvHeader, Rec);
        EditPurchInvHeader(PurchInvHeader, Rec);
        ModifyPurchInvHeader(PurchInvHeader, Rec);

        Rec.Copy(PurchInvHeader);

        UpdateVendorLedgerEntry(Rec);
    end;

Reads like a book. Or a tweet. So simple, very clear. I call this improvement already. It’s necessarily spectacularly more testable, but at least, now, I can test three different things in isolation of each other: I can test if I can retrieve a record from the database based on the input, I can test if my business logic (assignment of values from one table to another in this case) works correctly without having to have that record actually written to the database (and this is already a massive, massive improvement!!!), and finally, I can test if I can write this modified record to the database.

And here we get to the deepest why of why I really delegated it this way. Re-read my previous paragraph. Then read it again if it’s not crystally obvious on the first read. Then do it again if necessary (at least repeat-until is something we in AL are quite good at).

I don’t need to test all those things.

Why in the earth would I want to test if I can read a record from the database, and if I can write it back to the database? The database is my dependency, Microsoft, actually two separate teams at Microsoft, have tested the bejesus out of database reading and writing, and there is nothing for you to test here, Trust me, if Record.Get or Record.Modify or Record.Find or any other Record.Whatever method doesn’t work correctly that’s not your problem, that’s not something your test code should be ever concerned about. Both the Get and Modify in our code here are unconditional – you want them to fail at runtime if they can’t find the record you need, and you want them to succeed if they can find the record you need. And that’s what they will do, and you don’t need to test it. Ever.

This was in fact the topic of my session yesterday: code coverage. Someone once “accused” me of my “100% coverage policy” (and this is something I will also cover in a blog post of its own here), but I don’t have that policy actually. I do, actually. Now I confused you. Yes, I want to test all 100% of code that needs testing. But I also want to test 0% of code that needs no testing. And what I want above else is a clear separation of that code.

And this is what I have done up until this point. I have three isolated method, each of which has a single concern, and two of those concerns are not my concern. Only one is – the business logic. So I can test that in complete isolation of anything else, and of the database.

But that’s not all of it, there are more things going on in this codeunit, so let’s move on.

The next step for me is to refactor this one:

local procedure UpdateVendorLedgerEntry(PurchInvHeader: Record "Purch. Inv. Header")
    var
        VendorLedgerEntry: Record "Vendor Ledger Entry";
    begin
        if not GetVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader) then
            exit;
        VendorLedgerEntry."Payment Method Code" := PurchInvHeader."Payment Method Code";
        VendorLedgerEntry."Payment Reference" := PurchInvHeader."Payment Reference";
        VendorLedgerEntry."Creditor No." := PurchInvHeader."Creditor No.";
        VendorLedgerEntry.Description := PurchInvHeader."Posting Description";
        OnBeforeUpdateVendorLedgerEntryAfterSetValues(VendorLedgerEntry, PurchInvHeader);
        Codeunit.Run(Codeunit::"Vend. Entry-Edit", VendorLedgerEntry);
    end;

Funnily enough, Microsoft has already done the “read this from the database” part here. The GetVendorLedgerEntry function does that, so that part I don’t have to delegate into another function.

The next block, though, I do. So here it goes:

internal procedure EditVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header")
    begin
        VendorLedgerEntry."Payment Method Code" := PurchInvHeader."Payment Method Code";
        VendorLedgerEntry."Payment Reference" := PurchInvHeader."Payment Reference";
        VendorLedgerEntry."Creditor No." := PurchInvHeader."Creditor No.";
        VendorLedgerEntry.Description := PurchInvHeader."Posting Description";
    end;

Again, it’s the same reasoning. This is the business logic of this codeunit, and I want it both delegated into a separate place and decoupled from the flow so I can test it in isolation, directly.

And I also want this delegated:

internal procedure RunVendorEntryEdit(var VendorLedgerEntry: Record "Vendor Ledger Entry")
    begin
        Codeunit.Run(Codeunit::"Vend. Entry-Edit", VendorLedgerEntry);
    end;

This is an external dependency to my code, and I never want to test this. There is nothing for me to gain by ever testing this function now, or involving this function in any tests I’ll make once I am done refactoring, This is the first step.

Why is it that I don’t want this codeunit involved in my tests? Well, it should already be obvious. This is not a part of business logic of the codeunit I am currently working with – it’s a dependency. Yes, for my codeunit to get its job done, it needs to invoke this “Vend. Entry-Edit” codeunit to do its work, but when testing, I want to test my codeunit in isolation of this. If you can decouple your code from its dependencies, you always should. So even if it’s a single line of code in this RunVendorEntryEdit function, it’s still an improvement. Maybe it’s not obvious just yet, but bear with me. As I said, this is the first step.

And finally I can refactor the UpdateVendorLedger entry function to call these two delegates, so here it goes:

local procedure UpdateVendorLedgerEntry(PurchInvHeader: Record "Purch. Inv. Header")
    var
        VendorLedgerEntry: Record "Vendor Ledger Entry";
    begin
        if not GetVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader) then
            exit;

        EditVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader);
        RunVendorEntryEdit(VendorLedgerEntry);
    end;

And that’s it. Now all of the code is spread around. I can test most of my business logic directly, isolated from both database and dependencies (and database is a dependency, that’s why I am so eager to delegate database access to functions of their own whenever i can).

If you did all these changes, you can now run the existing test, and you’ll see that these changes didn’t break anything. Everything still works correctly, just as it did before you took it apart and restructured. But if you look at the code, you can see that it’s now much better structured, follows the separation of concerns principle, follows the single-responsibility principle, is more readable than before. And – arguably – it’s more testable. How exactly?

The core of the business logic of this codeunit now happens in two functions: EditPurchInvHeader and EditVendorLedgerEntry, and I can test these two functions directly without having to write anything to the database. No givens necessary. No fixtures. These two functions are now isolated from the process, decoupled from it, so they can be tested directly, in isolation of database and all other dependencies. And that’s a massive improvement. I am not showing you any tests just yet, because I am not done refactoring.

Then we have these functions: FindPurchInvHeader, ModifyPurchInvHeader, GetVendorLedgerEntry, and RunVendorEntryEdit. I don’t want to test them, ever. There is nothing in those functions that I care about testing, nothing of interest. These are just my dependencies. I always treat database reads and writes as dependencies and I try to isolate them from my code so I both don’t have to test them, and I can test the rest without invoking any of database access.

This has profound impact on my test code, because – from my experience – most of the time spent writing test code is spent on preparing the fixtures, the givens. More often than not it’s a trial and error, or a cat and a mouse game of running the test, getting a failure, reading the error to understand which next “given” you need, adding that “given” and then doing it again and again until you get the test passing. The biggest problems with givens is that mostly they aren’t obvious. Often those givens are there not because your code truly needs them, they are there because one of dependencies (or your dependencies’ dependencies deep down the dependency chain need them). With this approach you’ve just seen here, either no givens are need (which is true in most of situations) or if they are needed, they are really your givens, stuff that your code needs, not some other code you don’t want to test, but you have to, because it’s so tightly coupled. When everything is decoupled, testing becomes far, far simpler.

Take a look:

[Test]
    procedure TestEditPurchInvHeader()
    var
        PurchInvHeader, PurchInvHeader2 : Record "Purch. Inv. Header";
        PurchInvHeaderEdit: Codeunit PurchInvHeaderEdit;
    begin
        // Assemble
        PurchInvHeader."Payment Reference" := LibraryUtility.GenerateRandomText(50);
        PurchInvHeader."Payment Method Code" := LibraryUtility.GenerateRandomText(10);
        PurchInvHeader."Creditor No." := LibraryUtility.GenerateRandomText(20);
        PurchInvHeader."Ship-to Code" := LibraryUtility.GenerateRandomText(10);
        PurchInvHeader."Posting Description" := LibraryUtility.GenerateRandomText(100);

        // Act
        PurchInvHeaderEdit.EditPurchInvHeader(PurchInvHeader2, PurchInvHeader);

        // Assert
        Assert.AreEqual(PurchInvHeader."Payment Reference", PurchInvHeader2."Payment Reference", 'Payment Reference not updated');
        Assert.AreEqual(PurchInvHeader."Payment Method Code", PurchInvHeader2."Payment Method Code", 'Payment Method Code not updated');
        Assert.AreEqual(PurchInvHeader."Creditor No.", PurchInvHeader2."Creditor No.", 'Creditor No. not updated');
        Assert.AreEqual(PurchInvHeader."Ship-to Code", PurchInvHeader2."Ship-to Code", 'Ship-to Code not updated');
        Assert.AreEqual(PurchInvHeader."Posting Description", PurchInvHeader2."Posting Description", 'Posting Description not updated');
    end;

    [Test]
    procedure TestEditVendorLedgEntry()
    var
        PurchInvHeader: Record "Purch. Inv. Header";
        VendorLedgerEntry: Record "Vendor Ledger Entry";
        PurchInvHeaderEdit: Codeunit PurchInvHeaderEdit;
    begin
        // Assemble
        PurchInvHeader."Payment Reference" := LibraryUtility.GenerateRandomText(50);
        PurchInvHeader."Payment Method Code" := LibraryUtility.GenerateRandomText(10);
        PurchInvHeader."Creditor No." := LibraryUtility.GenerateRandomText(20);
        PurchInvHeader."Ship-to Code" := LibraryUtility.GenerateRandomText(10);
        PurchInvHeader."Posting Description" := LibraryUtility.GenerateRandomText(100);

        // Act
        PurchInvHeaderEdit.EditVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader);

        // Assert
        Assert.AreEqual(PurchInvHeader."Payment Reference", VendorLedgerEntry."Payment Reference", 'Payment Reference not updated');
        Assert.AreEqual(PurchInvHeader."Payment Method Code", VendorLedgerEntry."Payment Method Code", 'Payment Method Code not updated');
        Assert.AreEqual(PurchInvHeader."Creditor No.", VendorLedgerEntry."Creditor No.", 'Creditor No. not updated');
        Assert.AreEqual(PurchInvHeader."Posting Description", VendorLedgerEntry.Description, 'Posting Description not updated');
    end;

Very clean, very straightforward. And it takes next to no time to write these tests. Spoiler alert: copilot wrote them for me.

Before I go on, I want to take another look at the GetVendorLedgerEntry function, one function I didn’t refactor at all, and I kept as-is the way Microsoft did it:

internal procedure GetVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header"): Boolean
    begin
        if PurchInvHeader."Vendor Ledger Entry No." = 0 then
            exit(false);
        VendorLedgerEntry.ReadIsolation(IsolationLevel::UpdLock);
        exit(VendorLedgerEntry.Get(PurchInvHeader."Vendor Ledger Entry No."));
    end;

You could argue that this is not pure database access, and that there is business logic of interest to my tests here, and I would agree. Technically, this if statement requires me to test this path. The solution would be to split this function up the way I did with the others, so I have this decision decoupled from the database access that immediately follows. However, I don’t want to do it, because of the marginal value that would add. Why is that?

Imagine that I delete these two lines of code (the entire if statement with its exit part). imagine I truly only do just the database access here. Would anything change? No. The code would still work exactly is it does, the Get here is invoked conditionally, so it is not an error-inducing statement, it doesn’t add to cyclomatic complexity of anything, there are no different branches or paths control flow can follow here. Doing a Get on an entry no. 0 would return false anyway. The only reason why these two statements are here are to avoid an unnecessary database call if you know up front what its result would be. So even though it looks like a business logic of interest (from testing perspective) it truly is nothing but database access optimization. If you want to test it, be my guest, split it up and decouple it further, but I don’t want to go that far in this case. That’s why I kept it as is.

But, if you followed this with due attention you’ll ask me a valid question: what about the OnRun trigger and the refactored UpdateVendorLedgerEntry function? Don’t I want to test them? Because if the answer is yes, then all the changes I did seem like an exercise in futility, because the moment I start calling either of these functions, nothing I did up to this point matters, it’s all just useless, I am back to square one. A lot of givens, data preparation, and all…

But still – my answer is “yes, absolutely, I totally want to test those functions”. They are business logic of interest for me. The only issue is, I can’t do it just yet. They aren’t testable yet.

So here we get to the most important part. How do we make those testable? How do we make it possible for those two functions to be tested in isolation of the database and that other codeunit that my codeunit invokes?

The answer is inversion of control. I need to do something else to my code to make it testable in isolation of its dependencies. So let’s do it. BTW – I am not going to explain what inversion of control is here today. If you don’t know, then just follow the link I provided, or google it up to learn. It’s an important concept. I might touch on it directly in the future in one of the upcoming posts, but I don’t want to pollute this blog post with extra information, because it’s already getting seriously long.

Anyway, inversion of control. I need inversion of control so I can mock my dependencies. I’ll talk a lot about mocking in the future, as it is the central concept to successful and efficient testing.

First step in applying inversion of control is to extract the interface from my codeunit. This extracting interface is a concept pretty familiar to any C# or Java developer worth their salt, but for us in AL it may sound weird. Here’s a link to start with, and then you can google it more if you want: https://refactoring.guru/extract-interface

So here’s my extracted interface:

interface IPurchInvHeaderEdit
{
    Access = Internal;

    procedure FindPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header");
    procedure EditPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header");
    procedure ModifyPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header");

    procedure GetVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header"): Boolean;
    procedure EditVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header");
    procedure RunVendorEntryEdit(var VendorLedgerEntry: Record "Vendor Ledger Entry");
}

As you can see, this does nothing else but declare the functions already present in my codeunit. Also, the interface is marked as internal, so I am free to evolve it over time without worrying about breaking changes it would introduce to third parties. Nobody but me can access it.

Implementing that interface on that codeunit is now very easy, I only need to do this:

codeunit 50102 PurchInvHeaderEdit implements IPurchInvHeaderEdit

Done. Now that my codeunit implements this interface, I can mock it. In other words, in my tests, I can provide an alternative implementation that doesn’t involve actual dependencies, like database, or other codeunits. But before I can do it, I need to do some more changes to my code.

First change is not calling any of the functions I have declared in my interface directly, but through an interface variable. For example, instead of doing this:

FindPurchInvHeader(PurchInvHeader, Rec);
        EditPurchInvHeader(PurchInvHeader, Rec);
        ModifyPurchInvHeader(PurchInvHeader, Rec);

… want to do this instead:

// var Edit: Interface IPurchInvHeaderEdit
        Edit.FindPurchInvHeader(PurchInvHeader, Rec);
        Edit.EditPurchInvHeader(PurchInvHeader, Rec);
        Edit.ModifyPurchInvHeader(PurchInvHeader, Rec);

To be able to do this, I’ll take my OnRun trigger apart and create this new function from it:

internal procedure DoEditPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; var Rec: Record "Purch. Inv. Header"; Edit: Interface IPurchInvHeaderEdit)
    begin
        Edit.FindPurchInvHeader(PurchInvHeader, Rec);
        Edit.EditPurchInvHeader(PurchInvHeader, Rec);
        Edit.ModifyPurchInvHeader(PurchInvHeader, Rec);

        Rec.Copy(PurchInvHeader);

        UpdateVendorLedgerEntry(Rec, Edit);
    end;

Essentially, this function does everything OnRun did, except it does it in an abstract way, through an interface, rather than concretely, directly invoking the code in this codeunit. Then I can call this function from the OnRun trigger like this:

trigger OnRun()
    var
        PurchInvHeader: Record "Purch. Inv. Header";
        This: Codeunit PurchInvHeaderEdit;
    begin
        DoEditPurchInvHeader(PurchInvHeader, Rec, This);
    end;

Since the PurchInvHeaderEdit codeunit implements the interface I need, I can declare the “this” variable (it’s not truly “this” in the way “this” works in, say, C#, but it’s close enough). Essentially I am calling DoEditPurchInvHeader by passing an instance of this codeunit to it as its dependency. It’s called dependency injection. Again, this is no breaking change for any existing code that uses my codeunit. All such code can keep calling the Run on this codeunit just the same way as it did before, and all logic would then execute in exactly the same way as before. But, it allows me to call DoEditPurchInvHeader by passing another, different implementation into it, a mock, rather than an actual PurchInvHeaderEdit codeunit instance.

Another thing this achieved is that the OnRun trigger now falls cleanly into the “I don’t need to test this” bucket. Since this OnRun trigger does nothing else but calls another function, I don’t need to test OnRun but I can test that other function. Neat.

I will refactor the UpdateVendorLedgerEntry function exactly the same way:

local procedure UpdateVendorLedgerEntry(PurchInvHeader: Record "Purch. Inv. Header"; Edit: Interface IPurchInvHeaderEdit)
    var
        VendorLedgerEntry: Record "Vendor Ledger Entry";
    begin
        if not Edit.GetVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader) then
            exit;

        Edit.EditVendorLedgerEntry(VendorLedgerEntry, PurchInvHeader);
        Edit.RunVendorEntryEdit(VendorLedgerEntry);
    end;

It now receives the dependency through a parameter and calls the business logic through an interface variable.

And this concludes my codeunit refactoring. Now I can focus on testing the remaining business logic, and reap the benefits of extracting the interface and using it the way I did.

First of all, let’s see what it is that I truly want to test inside the DoEditPurchInvHeader function and UpdateVendorLedgerEntry functions. I tested the business logic inside the EditPurchInvHeader and EditVendorLedgerEntry functions already and I know they work. I also said I don’t want to test any of the database access code (FindPurchInvHeader, ModifyPurchInvHeader, GetVendorLedgerEntry) because I can safely assume they work correctly. They are third-party dependency for me, and assuming they work is fine, truly fine. I said it at my session yesterday, and I’ve mentioned it here, but I’ll repeat it: there is nothing of interest to me in those functions. If they don’t work, I have far deeper problem.

Before I can explain what I want to test in these remaining functions, let me explain first what I do not want to test about them. I do not want to test any output correctness, in short, I don’t want my tests to assert anything about any output they ultimately produce. All of their output is done in the delegated functions they call – EditPurchInvHeader and EditVendorLedgerEntry – and those functions I have tested already and proved that they work correctly. The only other output that DoEditPurchInvHeader produces directly, and UpdateVendorLedgerEntry produces indirectly through invoking the “Vend. Entry-Edit” codeunit is just database side effects, and I don’t want to test them as I have made the (safe) assumption that database (my third-party dependency) works correctly. Again – Microsoft has tested this for me, twice: the SQL team has proven that SQL accesses data correctly, and the BC team has proven that the NST uses SQL correctly, so nothing else for me to prove here that they didn’t prove already.

So what is it, then, that I do want to test here, if it’s not their output? Well, I want to test their flow. I want to test their execution paths and verify that everything that needs to happen based on decisions made by the code really happens when it needs to, and doesn’t happen when it doesn’t need to. In other word.

My code has two possible paths. The first path retrieves a purchase invoice header from the database, updates its values, writes it to the database, then attempts to get a vendor ledger entry from the database, doesn’t find it, and exits. The second path does all the same with the purchase invoice header, but it then successfully finds a vendor ledger entry, updates its values, and invokes the other codeunit to complete the work on it. And I now need to test these paths and prove that these things really happened: that a record would be read from the database when needed, that stuff would be updated when needed, and would not be updated when not needed, and that “Vend. Entry-Edit” codeunit would only be invoked when a ledger entry was found in the database.

To be able to verify this, I’ll need a mock that implements the IPurchInvHeaderEdit interface. That mock will do these things for me:

  • Nothing at all for finding or editing the values on the purchase invoice header. Finding is pure database work, and I since I don’t want to involve the database, there is nothing to do here. Editing values is something I have tested directly, so my mock also doesn’t have anything to do.
  • For ModifyPurchInvHeader I want to record whether it was called. In reality, I would have not even cared for this because it happens on both execution paths, and it happens every time, and while at runtime it may fail, those failures are unconditional third-party dependency failures there is nothing I want to truly test. But just as an example, I am logging the fact that modify was called.
  • For GetVendorLedgerEntry I don’t want to read anything from the database. The code that uses the vendor ledger entry doesn’t care if it was read from the database, or mocked. The only thing I want to be able to do here is control when to return true (simulating the “there is a ledger entry in the database” situation) and when to return false (simulating the “there is no entry” situation), so I could send the execution of tests both ways to cover both paths.
  • Finally, for EditVendorLedgerEntry and RunVendorEntryEdit I want to know whether they have been invoked. They should only be invoked when there is a ledger entry, and they must not be invoked if there is no ledger entry.

Here is my mock:

codeunit 50144 MockPurchInvHeaderEdit implements IPurchInvHeaderEdit
{
    procedure FindPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header");
    begin
        // Nothing to do
    end;

    procedure EditPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header");
    begin
        // Nothing to do
    end;

    var
        _purchInvHeaderModified: Boolean;

    procedure IsPurchInvHeaderModified(): Boolean
    begin
        exit(_purchInvHeaderModified);
    end;

    procedure ModifyPurchInvHeader(var PurchInvHeader: Record "Purch. Inv. Header"; FromPurchInvHeader: Record "Purch. Inv. Header");
    begin
        _purchInvHeaderModified := true;
    end;

    var
        _hasVendorLedgerEntry: Boolean;

    procedure SetHasVendorLedgerEntry(value: Boolean)
    begin
        _hasVendorLedgerEntry := value;
    end;

    procedure GetVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header"): Boolean;
    begin
        exit(_hasVendorLedgerEntry);
    end;

    var
        _editVendorLedgerEntryCalled: Boolean;

    procedure IsEditVendorLedgerEntryCalled(): Boolean
    begin
        exit(_editVendorLedgerEntryCalled);
    end;

    procedure EditVendorLedgerEntry(var VendorLedgerEntry: Record "Vendor Ledger Entry"; PurchInvHeader: Record "Purch. Inv. Header");
    begin
        _editVendorLedgerEntryCalled := true;
    end;

    var
        _vendorEntryEditCalled: Boolean;

    procedure IsVendorEntryEditCalled(): Boolean
    begin
        exit(_vendorEntryEditCalled);
    end;

    procedure RunVendorEntryEdit(var VendorLedgerEntry: Record "Vendor Ledger Entry");
    begin
        _vendorEntryEditCalled := true;
    end;
}

And finally, I can write my remaining tests:

[Test]
    procedure TestNoVendorLedgerEntry()
    var
        PurchInvHeader, PurchInvHeader2 : Record "Purch. Inv. Header";
        MockEdit: Codeunit MockPurchInvHeaderEdit;
        PurchInvHeaderEdit: Codeunit PurchInvHeaderEdit;
    begin
        // Act
        PurchInvHeaderEdit.DoEditPurchInvHeader(PurchInvHeader, PurchInvHeader2, MockEdit);

        // Assert
        Assert.IsTrue(MockEdit.IsPurchInvHeaderModified(), 'Purch. Inv. Header not modified');
        Assert.IsFalse(MockEdit.IsVendorEntryEditCalled(), 'Vendor Ledger Entry should not be modified');
        Assert.IsFalse(MockEdit.IsEditVendorLedgerEntryCalled(), 'Vendor edit should not be run');
    end;

    [Test]
    procedure TestWithVendorLedgerEntry()
    var
        PurchInvHeader, PurchInvHeader2 : Record "Purch. Inv. Header";
        MockEdit: Codeunit MockPurchInvHeaderEdit;
        PurchInvHeaderEdit: Codeunit PurchInvHeaderEdit;
    begin
        // Assemble
        MockEdit.SetHasVendorLedgerEntry(true);

        // Act
        PurchInvHeaderEdit.DoEditPurchInvHeader(PurchInvHeader, PurchInvHeader2, MockEdit);

        // Assert
        Assert.IsTrue(MockEdit.IsPurchInvHeaderModified(), 'Purch. Inv. Header not modified');
        Assert.IsTrue(MockEdit.IsVendorEntryEditCalled(), 'Vendor edit should be run');
        Assert.IsTrue(MockEdit.IsEditVendorLedgerEntryCalled(), 'Vendor Ledger Entry should be modified');
    end;

In the first test, I simulate the “no ledger entry” situation (by default, the GetVendorLedgerEntry function will return false), and then I verify is purchase header was modified, that vendor ledger entry wasn’t modified, and that “Vend. Entry-Edit” was not invoked.

In the second test, I simulate the “ledger entry exists” situation by setting the GetVendorLedgerEntry to true (my only “given” in these two tests, if you want, but let’s not call it “given” because it isn’t one). Then I verify that header was modified, ledger entry edited, and “Vend. Entry-Edit” invoked.

And that’s it.

Now, run these tests and compare the execution time. It takes no measurable time to run these tests now. All code paths were tested, all business logic was validated, and all of it was done in complete isolation from both the database and external dependencies. This is what unit testing is all about, and this is how you can do it.

In the context of my session yesterday, which was about code coverage, I have covered 65,63% of my code in this codeunit. Far below the (fake, meaningless) “target” of 80%. However, I have 100% coverage of any code of interest, and 0% coverage of any code that I don’t want to test.

I mentioned at the session yesterday, that this entire session was motivated by a comment I received from my audience about a session I did at Days of Knowledge in Odense in June this year: “I am scared of Vjeko’s 100% coverage policy”. First of all, I didn’t even mention any percentage, let alone 100% at my session in Odense – this “total coverage” policy was assumed by this person simply because I was explaining how I want my code tested, and I wasn’t even concerned about coverage. But if anyone ever assumes I am after 100% coverage ever in the future, I’ll now have this blog post to refer them to. About the code that deserves to be tested, I totally want 100% coverage, no less. And about the code that doesn’t need to be touched by tests, I want 0% coverage, no more. And I want a natural, clean, obvious, intentional separation of these two in all of the code I write.

And again – this is what unit testing is about. And this is why I find it valuable.

That’s it for today, this is the gist of what I presented yesterday, and this is my first actual contribution on this new testability journey I want to focus on for the foreseeable future.

Comments

Loading comments…