On the difficulty of reproducing a defect

[ploeh]

Given the defect identified in the discussion of #441, I wanted to reproduce the bug as an automated test (one for each instance of the problem, in fact).

This has turned out to be surprisingly difficult.

First I tried with fully-defined FsCheck generators, but even though I had help from Copilot, this quickly blew up due to the deeply nested structure of the data types. To show the results of this attempt, I've pushed a branch called gen-for-order-plan. The test fails, as it should, but it also fails when you apply a fix.

The problem is that getting to a non-empty OrderPlan.Totals value requires, it seems, a very particular combination of input values, and it's not clear to me what such a combination might look like. I've found no tests to guide me in that direction, either.

After some time, I put the gen-for-order-plan branch on hold in order to start with a more focused approach. This, then, has met with the same problem. I've pushed that attempt to a branch named update-order-plan-test-attempt. Here, I've instead tried to use FsCheck to run through possible values and filter out those that produce empty Totals values, so far with the outcome that the test hangs, since the generators never arrive at a 'winning combination'.

Since I have access to all the source code, I can, of course, given enough time, reverse-engineer the implementation of OrderPlanService.calculateTotals, but so far, I've already spent more hours on this problem than I would have expected.

And since I'm on the clock, I only find it mature and responsible to stop and ask: Am I missing something obvious, or is it really that hard, given that I have no domain knowledge to draw on?

Please advice on how I should continue.

[7sharp9]

@ploeh I find Anthropic's Claude blows Copilot into space in terms of capability and reasoning, this is good when you need to brain stamp this type of complexity and lay it down in simple terms, I have a spare trial if you need an invite etc.

[ploeh]

@7sharp9 It may be time for me to bite the bullet, so if you can spare that trial, I would appreciate it. Thank you.

[halcwb]

@ploeh and @7sharp9

There is quite a lot of things to say about this. First of all, my sincere apologies too @ploeh. I may have set you on the wrong track. But, to look at the bright side of things, I think (at least I) can learn a couple of lessons. This will be quite a long comment so bear with me.

I may have screwed up using AI

With that I mean that I let AI review the original pr and this let to flagging an issue with the getTotals. And I think this was, partly, stated in a confusing way. I have re-challenged the issue and now it is framed as:

GetTotals() returns TotalsData[] — a value snapshot, not a live handle. Caching happens one level up, inside the provider. So:

• let totals = provider.GetTotals() outside the lambda → array reference frozen at that moment, forever.
• let totals = provider.GetTotals() inside the lambda → re-reads whatever the provider's cache currently holds.

An that makes sense. So, only when you reload and the actual totals data have been changed this could lead to the use of invalid out of date totals when captured "outside the lambda".

Using Mermaids is helpful

I agree with @7sharp9 that for software knowledge the Anthropic models and cli is far better than for example copilot. For example if you look at the below mermaid sequence diagram. The whole issue becomes much clearer what getTotals actually does.

sequenceDiagram
    autonumber
    participant C as Caller<br/>(ServerApi.Adapters /<br/>AgentAdapters / Services)
    participant P as IResourceProvider<br/>(CachedResourceProvider)
    participant L as LoadEngine
    participant R as Registry entry<br/>"totalsData"
    participant M as Mapping.getTotals
    participant GS as Google Sheets<br/>(CSV)
    participant S as Consumers<br/>(OrderService / OrderPlanService /<br/>NutritionPlanService)

    C->>P: GetTotals()
    activate P
    P->>P: lock + check cache (TTL)

    alt cache miss or expired
        P->>P: loadFresh() → loadAllResources
        P->>L: ForceAll()
        activate L
        L->>R: loader (memoised, once per key)
        activate R
        R->>M: getTotals dataUrlId
        M->>GS: fetch sheet as CSV
        GS-->>M: rows
        alt Ok data
            M-->>R: Ok TotalsData[]
            R-->>L: Ok (box data, [])
        else Error msgs
            M-->>R: Error msgs
            Note over R: optional resource —<br/>swallow to [||],<br/>emit Warning
            R-->>L: Ok (box [||], warnings)
        end
        deactivate R
        L-->>P: Resolved map + Warnings
        deactivate L
        P->>P: state.Data.TotalsData ← resolved<br/>cache ← (loaded, UtcNow)
    else cache hit
        P->>P: selector _.State.Data.TotalsData
    end

    P-->>C: TotalsData[]
    deactivate P

    Note over C: totals hoisted once per port /<br/>per command handler

    alt OrderContext path (Services.fs:364)
        C->>S: updateIntake totals ctx
        S->>S: OrderService.getTotals totals age weight orders
        S-->>C: ctx with Intake set
    else OrderPlan path (Adapters.fs:75 / AgentAdapters.fs:167)
        C->>S: OrderPlanService.calculateTotals totals plan
        S-->>C: OrderPlan with totals
    else NutritionPlan path (Adapters.fs:97 / AgentAdapters.fs:250)
        C->>S: init / add / remove / update ... totals
        S-->>C: NutritionPlan with totals
    end

Loading

Unclear domain modelling.

This is also a lesson. Apparently the code is now insufficient to describe the domain. What is intended is that the getTotals returns TotalsData that describe what and how you want order totals to be calculated. So, this is just misleading naming. It should be called something like getTotalsCalculationConfig. This is then used to determine which things to total in a set of orders depending on the patient context (age and weight).

The problem is that I just "know" this. But, for someone not familiar with the rather specific domain knowledge, this is quite difficult to understand, especially given the misleading naming.

Testing the wrong hypothesis

So, I think that the wrong hypothesis was tested. I think the hypothesis, and thus the tests, should be: when using getTotals to calculate the totals for a fixed set of orders for a patient, changing the totals data should lead to changed totals calculated after reload of the resources, given that totals is part of the resources.

P.S. That's by the way a different hypothesis from can calculate totals from getTotals.

Should we use branches in the main repository?

@ploeh you created some exemple branches in the main repository. I would like to make the point of keeping the main repository as "clean" as possible. I have started a discussion on this topic.

So, long story short, I owe this issue and should fix this. But from this emerges a more broader issue, need to improve the code base so the code is more readable. And also, I very very much agree with the concept that code should fit into your head. My feeling is that you also should make sure that you can level up that concept to higher levels of architecture of the code. So, a bit like the old divide and conquer technique to handle big complex problems where each sub problem you can "fit in your head".

[ploeh]

There is quite a lot of things to say about this. First of all, my sincere apologies too @ploeh.

There's no need to apologise. Issues like these are par for the course in a 'real' code base. We write code to the best of our knowledge at the time. Later, we have new insights, and our old efforts look either naive, hopelessly over-complicated, or even both at the same time.

If I came across as annoyed, that was not my intention. I was only concerned about burning billable hours without coordinating with you.

[...] And I think this was, partly, stated in a confusing way. [...]

Perhaps, but I don't think I was confused about the problem. We may verify that later, since I may have a workaround for the test issue. Once I send a pull request for that, you can assess whether or not I understood the problem correctly 😉

Unclear domain modelling.

This is also a lesson. Apparently the code is now insufficient to describe the domain. What is intended is that the getTotals returns TotalsData that describe what and how you want order totals to be calculated. So, this is just misleading naming. It should be called something like getTotalsCalculationConfig. This is then used to determine which things to total in a set of orders depending on the patient context (age and weight).

The problem is that I just "know" this. But, for someone not familiar with the rather specific domain knowledge, this is quite difficult to understand, especially given the misleading naming.

Yes, that's true. That's also the reason I went looking for some existing test coverage for some of this, because tests, as examples, can really help illustrate the intended use of an API.

If we wish to improve this aspect of the code base, adding more tests as instructional examples could be a major part of that effort.

Testing the wrong hypothesis

So, I think that the wrong hypothesis was tested. I think the hypothesis, and thus the tests, should be: when using getTotals to calculate the totals for a fixed set of orders for a patient, changing the totals data should lead to changed totals calculated after reload of the resources, given that totals is part of the resources.

Indeed, that was what I have been trying to test, but in order to test that Totals change between two otherwise identical calls, I need two different values. It's easy enough to make the SUT produce an empty Totals value, but that still means that I need to poke the SUT in a way that it produces another, non-empty Totals value, and that is what yet eludes me.

To be clear, since I have access to all of the source code, it's only a matter of time before I'm able to reverse-engineer the implementation to identify such a combination of input values.

As stated above, I was getting concerned about burning billable hours on this problem without consulting you.

@ploeh you created some exemple branches in the main repository. I would like to make the point of keeping the main repository as "clean" as possible. I have started a discussion on this topic.

I wasn't aware of that, but we can delete the branches again. I'll do that.

[halcwb]

Indeed, that was what I have been trying to test, but in order to test that Totals change between two otherwise identical calls, I need two different values. It's easy enough to make the SUT produce an empty Totals value, but that still means that I need to poke the SUT in a way that it produces another, non-empty Totals value, and that is what yet eludes me.

The totals are calculated if a set of orders contain items match a "total". For example:

order: morfin
  component: morfin ampul
    item: morfin
  component: glucose solution
    item: glucose

totals contains glucose

-> glucose is totalled because it exists in the order as an item.

Note that volume is an exception as this is determined at the orderable level looking at whether quantities are a volume.