Fix regression on YAML tests runner cache for tax benefit systems with reforms or extensions

[sandcha]

Bug fix

• Fixes regression introduced by Core v25 on YAML tests runner (tools/test_runner)
  • Updates reforms value type when one reform only is given in yaml test
• Updates reforms and extensions recognition in yaml tests runner cache

Every known tax benefit system is recognised in the cache by a key built with reforms value.
When we defined one reform only in the tax benefit system, the key was built with a string value instead of list value. This had an unexpected effect: reforms name with same characters (in different order) looked identical.

[Morendil]

Mostly optional feedback (we can add more tests to better document the current behaviour and preserve against regression) that could be left to a future PR, except that I'd prefer to see lines 40-47 in the test removed unless there's a good reason for them that I missed.

[Morendil]

Why did you find it necessary to have these lines 40-47 in the test? Deleting them, it seems that they are not missed, and that's expected since we mocked out the part that uses actual Python import and class loading… but maybe I'm missing something.

[sandcha]

Oops, sorry! I just forgot to remove them. Done.

[Morendil]

It's interesting to see it tested that way, it's not quite what I was expecting - and I can see how it adds value.

As I read this, we're testing only the caching mechanism; the test doesn't show anything about the "string -> list" conversion mechanism that provides some flexibility in how tests are written.

It would be a good idea to have one test showing the list usage and one test showing the string usage.

Similarly, for the cache it would be a good idea to show when two reform/extension specs lead to the same TaxBenefitSsytem object and when they don't.

For instance, the frozenset() implementation implies that

_get_tax_benefit_system(baseline, ['one', 'two'], extensions)

is the same object as

_get_tax_benefit_system(baseline, ['two', 'one'], extensions)

…but do we really want that? Can we say for sure that it doesn't matter in what order we apply two different reforms? If order matters, we should replace frozenset() by tuple() instead.

[Morendil]

I suggest inlining the extensions value - it draws attention away from what's interesting which is the reforms parameter.

[Morendil]

Rebased.

[Morendil]

I think these tests say all we want to say, for reforms.

It seems very likely we have a latent bug of the same kind but for extensions. If you have time, it would be good to have the same three tests but for extensions; or if you prefer we can open an issue and leave that for a later PR.