Directions EMEA 2023 demo – decoupling base app

  • Post category:Testing
  • Post comments:28 Comments
  • Reading time:40 mins read

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.

Vjeko

Vjeko has been writing code for living since 1995, and he has shared his knowledge and experience in presentations, articles, blogs, and elsewhere since 2002. Hopelessly curious, passionate about technology, avid language learner no matter human or computer.

This Post Has 28 Comments

  1. David Roys

    What an amazing post! Thanks for taking the time to write this up. I’ve been having exactly the problem you describe with building huge “Given” sections in order to satisfy the dependencies on the base application. Also the mocking stuff was great and I’m definitely going to read through this again and see if I can apply it. One question I have is around the assertion that I don’t need to test the FindPurchInvHeader procedure because that would only be testing the built in database access. I agree completely, but what if I make a mistake when I define the procedure and use FindPurchInvHeader(PurchInvHeader: Record “Purch. Inv. Header”; FromPurchInvHeader: Record “Purch. Inv. Header”) instead of FindPurchInvHeader(var PurchInvHeader: Record “Purch. Inv. Header”; FromPurchInvHeader: Record “Purch. Inv. Header”)? – note my mistake is I forgot to include the var in the method signature. In that case my method would not work as intended but that is not because the database access layer is not working but because I made a (fairly common) mistake. Now maybe there are other ways to trap that error (haven’t even checked to see if the compiler complains) and the use of interfaces would mean I couldn’t make the mistake as my implementation wouldn’t match the interface definition, so maybe it’s not a real problem. Just wondered if you had thought of that. I realise you are illustrating writing good code by re-implementing base app functionality (which we would probably not do in the real world, but the same principles apply to our own code which we may wish to refactor). Shouldn’t we also have the test of the code before we refactor it (using the terrible Given stack to create and post the purchase invoices, etc.) so that when we refactor we are confident that, not only have we written better code, we haven’t broken the functionality that existed before we started refactoring? Or is the assumption that if all of the lower level units work as expected, the higher level unit that puts them all together doesn’t need to be tested?

    1. Vjeko

      First of all, thanks for the kind words, Dave. And nice to talk to you again ๐Ÿ˜Š Now, the problem you describe is common, and I do intend to cover that, I just couldn’t cover everything in this single post – as I said, this is a journey.

      But let me give you general hints. Yes, you are totally right that while we can make assumption that the underlying dependency (database in this case) works fine, we can use that dependency in a wrong way, and break the code. 100% true, and 100% realistic. One other thing that you noticed that is spot on is that while I did use the refactoring example, this really isn’t about refactoring. Very often people ask me if I really write code like this, and my answer is a profound yes, but I don’t refactor. The point of this blog was to prove that it’s possible to achieve also by refactoring existing code, and that it’s easy to do it if you want to.

      However, if you are doing TDD, your code ends up looking like this from the onset. You don’t write tightly coupled code first so you can refactor it by decoupling it so you can test it. No. You do red-green-refactor all along, and your code grows from zero to hero by being decoupled from the onset.

      And this is where this problem you describe bubbles up. When you do TDD in this red-green-refactor way, you’ll spot it right away that you forgot to add “var” (I spotted it right away, your clarification was redundant ๐Ÿ˜Š) and you’ll fix it. One of your “red” cycles will be precisely about you forgetting to put “var” where it was due, and you’ll fix it and move on. You’ll do the same about a lot of other problems that you’ll notice as you evolve your code through looping through red-green-refactor until you are satisfied. Should you write tests specifically for that to make sure nobody removes “var” in the future, accidentally or less so – I think not. With TDD, if you want to be lean and efficient, at some point you should give yourself a benefit of the doubt and say that you need to put some trust into certain simple things. I mean, honestly, once you have a working piece of code, who and why would want to remove the “var” declaration in the future even if there are no tests that would pinpoint it right away?

      But then – there will be tests that will show you that: your integration tests. The fact that we do unit testing (and unit testing will never expose this type of a problem!) does not mean that we should stop writing integration tests. We need to keep writing integration tests. So once I have written my unit tests, I will write an integration test for an entire feature. But my point is, I will have very few integration tests, high up the dependency chain, as high up as possible (for this particular example, it’s not really any way higher than this codeunit itself) and those tests will fail because of a missing “var”.

      The problem I’ve identified in this community is that unfortunately we don’t know how to write unit tests, so we end up writing only integration tests, and that’s bad. We need both kind, both have their place, and they work together towards the same goal.

      1. David Roys

        Yes, of course you’re probably right about all of that. And it’s good to hear that the integration test is still needed and would catch it. I wonder if we could actually test the FindPurchInvHeader without hitting the DB? I can’t think of a way (that actually adds any value and doesn’t become totally convoluted) but I wonder whether the FindPurchInvHeader should just blow up as it does now if the record doesn’t exist or actually return true or false. It feels like there is a mocking option that would allow us to have both a record is found and record is not found scenario that doesn’t require the record existing which would allow us to trap the missing var (because the mock is simulating finding the record by just copying the value but our record doesn’t change). The more I think about it the more I’m talking myself out of it :-D. Anyway – really enjoyed the post and looking forward to more.

        1. NavNab

          When refactoring or writing new code, I tend to declare procedures like

          internal procedure FindPurchInvHeader(FromPurchInvHeader: Record “Purch. Inv. Header”) PurchInvHeader: Record “Purch. Inv. Header”

          instead of the “old way”

          internal procedure FindPurchInvHeader(var PurchInvHeader: Record “Purch. Inv. Header”; FromPurchInvHeader: Record “Purch. Inv. Header”)

          to clearly express my intention (for future me or other developers) that I do want to read the record only. I do not intend to modify it inside the procedure.

          By doing this I can’t by mistake forget to use the ‘var’.

          @vjeko thank you for the great work. I enjoyed your session in Lyon and I enjoy reading your blog :). Do you think this approach makes the code ‘better’ or ‘more testable’?

          1. Vjeko

            Thanks Nabil ๐Ÿ˜Š As to your question, this definitely makes the code more testable. Whether it makes it better depends a lot on your definition of good. I know that for some people this makes code less readable, and I can understand that. If you are not used to reading such code, I can understand where your “I can’t read this” comes from. Another complaint is that this makes your code more tedious to debug manually (using the debugger), and the deeper you go with abstraction and interface, the more expressed this “problem” becomes. But then again, I need to debug through my code less when it’s been properly unit-tested. It’s a long story ๐Ÿ˜Š

        2. Vjeko

          I totally missed addressing this point you make about whether we could/should test FindPurchInvHeader without hitting the database. We definitively could, that’s for sure, but whether or not it adds any value is highly debatable. That’s why I prefer leaving it in the “I won’t ever test this directly” bucket. And I’ll always try to structure my code in such a way that anything I don’t want to test directly is clearly separated from all those things I do want to test directly.

          One thing that may not be obvious from these two posts I did on the this topic of testing is that I do do integration tests, and those tests will hit the database in all those spots that I didn’t want to cover with unit tests, and that’s all still fine. But I will talk directly about that, it’s on the very top of my to-do list.

          But sometimes I will also want to mock the database, and that’s also on my to-do list ๐Ÿ˜Š

  2. Capone

    I’m glad I could motivate you to write this excellent and highly appreciated post! ๐Ÿ˜โœŒ๏ธ
    Are you planning to make a post/write something about “replacing” generic method codeunits with this type of coding?

    1. Vjeko

      Oh, it was you who made that comment? Well, thank you ๐Ÿ˜Š I’ll write a lot about testing and testability, and I am definitely going to “attack” (once again) the handled “pattern”.

  3. Todd

    First of all thank you for this. I have been trying to express this to my colleagues for some time now.

    I have a related question but not about the testing. I am confused by the original OnRun procedure.
    What is the point of the field assignments? The PurchInvHeader.Copy(Rec); will set the fields. Why “re-assign” the values? Also, do you really need the PurchInvHeader.Find()at all, the PurchInvHeader.Modify() will find the record. Without the field assignments you have one less function to test.

    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;

    1. Vjeko

      This code wants to assign values of only those five fields (lines 13-17) to the latest record stored in the database. Those fields “arrive” into this codeunit inside the Rec variable (which every codeunit receives by reference when you call Codeunit.Run and pass a record. Line 10 (PurchInvHeader.Copy(Rec)) copies all the fields. The idea here is not to assign any values, but to have an exact copy of the Rec variable so that you don’t operate directly on Rec. Line 12 (PurchInvHeader.Find()) finds the record in the database by primary key values. I agree that it makes it kind of less readable than if lines 10 and 12 would have been replaced with PurchInvHeader.Get(Rec.”No.”). But after this, everthing should be pretty clear.

      1. Todd

        But the result is that any field can be changed. The only thing that prevents that is that the page this is called from only has the 5 fields as editable on the page. If only those 5 fields are to updateable then there should be a way the restrict changes to only those fields.

        I know this code is not yours and the purpose of this post is not about what the code does but how to improve its testability; I am just trying to understand it.

        1. Vjeko

          It’s not true, only those five fields are changeable. You find the original record in the database (this operation will replace the values of all fields in the record variable with those in the database), the write values of those five fields, and then modify the record. No bug here.

          1. Todd

            Thank you, I guess the PurchInvHeader.Copy(Rec) through me. I see what it is doing now. I probably have gone with PurchInvHeader.Get(Rec.”No.”) and no copy but the Copy()/Find() allows for a change in PK I guess.

    1. Vjeko

      Very much possible. I took events out first, for clarity purpose to avoid pollution while providing examples and explanations. I’ll check tomorrow and fix it if I made a mistake. Thanks for pointing this out!

    2. Vjeko

      You are absolutely right. This was because I first took events out for clarity purpose (to avoid pollution while explaining things) and then later put them back into code, but didn’t pay enough attention. Thanks! ๐Ÿ˜Š

  4. Filip

    In the last part, when writing the full test, why twice asserting IsVendorEntryEditCalled? Shouldn’t one of them be IsEditVendorLedgerEntryCalled? The comments at the end of the lines are also swapped in TestNoVendorLedgerEntry versus TestWithVendorLedgerEntry. Can these both be copy-paste issues? Or am I missing something :-)?

    1. Vjeko

      Good catch! This is actually a copilot issue! As I said, copilot wrote the tests, and I didn’t pay attention. In my hurry to create the demo, I didn’t pay much attention and then some names of functions in my interface ended up confusingly similar to each other (so much about “clean code” ๐Ÿคช) and apparently I didn’t notice while “copiloting” my tests. Fixed it now!

  5. Bajke

    I havenโ€™t read anything better about BC/AL development recently, maybe never. You should write a book with such examples and clarifications. It could be your version of โ€œClean Code/Architectureโ€ from BC/AL perspective. ๐Ÿ™

    1. Vjeko

      Thanks for the kind words. I am now seriously considering this…

  6. Todd

    In order to write the unit tests all your procedures must be “internal” and no “local” procedures. So they are not available outside the app but are available inside to any object. Is this the intention? So to unit testing we have to eliminate all(most) local procedures? Or am I missing something?

    1. Vjeko

      Well, I haven’t quite covered that part yet (local/internal and whether it’s good or bad). There are pros and cons, and I plan to address that as well. There is still place for local procedures, but I intentionally didn’t go into explaining what and why, otherwise that blog post would have been much longer. Stay tuned, I have much much more to say about testability ๐Ÿ˜‰

  7. Morten Sorlie

    Hi! Greate post!

    I have tried to follow your pattern in my own code.
    What I cannot find in your example is where you initialize the interface.

    E.g. I have to add this command line in my code to initialize the interface to run.

    var
    MyInterface: Interface “My Interface”;

    MyInterface := Enum::”Interface Enum.”::Value;

    Is this something you do not have to do in your example?

    1. Vjeko

      Of course you have to initialize the interface, and of course I do it. My final shape of the OnRun trigger looks like this:

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

      The “This” variable is passed onto the DoEditPurchInvHeader function, and there you have the interface initialization.

    2. Vjeko

      One common misconception is that interfaces must be used in conjunction with enums and that you can only initialize them from enum variables/constants. That’s just not all there is to interfaces. You can initialize them from codeunit variables as well.

  8. David

    Impressive work. Keep going please, looking forward to your next posts and i try to reuse some of your patterns in near future.
    Hopefully Microsoft will read your posts too and refactor the base app accordingly because most work we do for customers is tightly coupled to base functionality (posting functions for example) and depends on MS logic. And writing tests for it feels like a pain:)

    1. Vjeko

      Thanks! Well, I know people at Microsoft read my posts because I do discuss these things with them. What they will do in their codebase, however, is entirely their choice. The switch on a codebase large as theirs is not easy, and it’s more than just a technical challenge. And this applies to any team with a large codebase. Switching will take time.

Leave a Reply