Allow variable neutralization in YAML test files

[benjello]

Connected to openfisca/openfisca-france#1481

New features

• Allow variable neutralization in YAML test files`

[sandcha]

@benjello Do you have an example where you needed to neutralize a variable in a YAML test?

Is it useful to temporarily check a behavior? I imagine that we can use the YAML tests with this new option to check the effect of a reform. Or is it useful for another use case?

[benjello]

It is useful when you will use a specific "reform" of the tax benefit system consisting only of neutralizing some variables for example to avoid never ending spirals for example to focus on some specific domains. Our use case right now is France's assurance-chômage.

[benjello]

@sandcha @maukoquiroga : can I merge this PR ?

[sandcha]

Thank you @benjello for this new feature!
Here is a commit with a test example that fails. Could you fix it or update the way of testing the feature? 🤔

[sandcha]

It is useful when you will use a specific "reform" of the tax benefit system consisting only of neutralizing some variables for example to avoid never ending spirals for example to focus on some specific domains. Our use case right now is France's assurance-chômage.

@benjello Is it also true to say that to check the French unemployment insurance (that, in its last reform, neutralizes some old incomes when a person becomes unemployed), it's useful to define YAML tests with the neutralized incomes to check the unemployment insurance formula?

[benjello]

@sandcha : I was more concerned by neutralizing many resources that may induce spirals that needs a lot of initialisation to be circumvent.

[benjello]

And I do not understand why the test is not passing since it passes locally on my computer ...

[sandcha]

@benjello in case we have a difference on the command, here is what I used:

pytest tests/core/test_yaml.py

And this is what the CI does as well.

[benjello]

@sandcha : I used

openfisca test tests/core/yaml_tests/test_with_neutralized_variables.yaml 

and it pass.

But if i try exactly what you did I do have a failure.

Which is strange BTW.

[sandcha]

The error at pytest tests/core/test_yaml.py execution might be linked to the tax_benefit_system as it is shared between yaml tests when it's identified as the same tbs. So here are two commits to set a specific tbs to the cache when there are neutralized variables.

The test_yaml.py now only works when test_with_extension and test_name_filter are commented. When they are not, the thrown errors seem to be linked to the next test in the test_yaml.py file. 🙃 🤔

[sandcha]

When we run pytest tests/core/test_yaml.py, the Holder of a neutralized variable (like basic_income in the YAML test) doesn't remember that the variable is neutralized (while he does when the same test in executed with openfisca test tests/core/yaml_tests/test_with_neutralized_variables.yaml).
We can see that by printing what happens in the get_array function of openfisca_core/holders/holder.py file.

Then, when we look further:

• ✅ openfisca test tests/core/yaml_tests/test_with_neutralized_variables.yaml runs openfisca_core/taxbenefitsystems/tax_benefit_system.py file get_variable and load_variable methods before running the tests and get_variable within test execution.
• ❌ pytest tests/core/test_yaml.py doesn't call get_variable or load_variable before the tests and even if it runs get_variable during test execution, it doesn't come with the variable.is_neutralized attribute.

[benjello]

@sandcha : can I help you on this or are you still exploring ?

[sandcha]

@benjello Yes, of course it would be great if you looked into the holders difference between the pytest and the openfisca test command! That's why I shared the last debugging results above :)
We can also decide to cut the issue into two subjects: open an issue on the holders for python tests and merge this PR if we are confident about the openfisca test behavior.

[benjello]

I am not sure I can help you a lot with the pytest machinery I never used ... Can't we, actually shoudn't we, align both ?

@MattiSG @maukoquiroga we need you here !

[benjello]

@cbenz : could you help us here ?

[MattiSG]

Can't we, actually shoudn't we, align both?

Yes, this is a major issue if both are expected to be used interchangeably. However, I only see references to openfisca test in the doc, and personally only use that one, which makes sense to me as some OpenFisca-specific test machinery / setup is used.

@sandcha under which circumstances do you use pytest? Do you have a specific reason for preferring it over openfisca test?

[MattiSG]

I don't have a strong opinion on the code, it seems of consistent quality and readability with the existing codebase.

I fail to understand two points:

1. In which way is this different from forcing the value of a variable to some value in the tested situation?
2. Can you please confirm this only makes available in YAML declarative tests a feature that already exists in the Python API (neutralize_variable)?

In any case, this behaviour should be documented (both Python and YAML).

[benjello]

Thanks @MattiSG for your questions:

• 1. Neutralizing sets a variable to its default value for every period so using it in YAML matches the Python API
• 2. Yes.

I will open a companion doc if it is ok with you.

[benjello]

It is actually documented on the pyhton API but the doc is broken as noted here.

[MattiSG]

Thank you for these replies @benjello!

Unblocking this PR

I understand that:

• Once Python API documentation seems broken openfisca-doc#244 is fixed (possibly by test: improve commons module doctests #1033), we should have Python doc for neutralize_variable.
• There is no PR yet on https://github.com/openfisca/openfisca-doc for documenting the YAML behaviour introduced by this PR.
• The currently blocking point here is that make test fails on this changeset while openfisca test works. Maybe it would be worth investigating if something like Retire le doublon pytest openfisca-france#1649 could be applied to Core. Another venue for exploration could be @maukoquiroga’s major Makefile refactor / overhaul in Run country & extension template tests systematically #1020.

I honestly start wondering if we should not get rid of the Makefile altogether and move the instructions in openfisca commands and in act if #1030 is confirmed.

Syntax discussion

It is probably a good thing to enable further YAML / Python equivalence.
However, I am not super excited about the current proposed syntax:

- name: "Result outside neutralized variables"
   period: 2021-01
   neutralized_variables:
     - housing_allowance
   input:
     age: 30
   output:
     basic_income: 600

Is “neutralising” the variable an economic domain term? If not, I would be more in favour of being more explicit and using something like force_default:

- name: "Result outside neutralized variables"
   period: 2021-01
   input:
     age: 30
   output:
     basic_income: 600
   force_default:
     - housing_allowance

Alternatively, what about using a special default keyword in YAML? This would make the syntax as follows:

- name: "Result outside neutralized variables"
   period: 2021-01
   input:
     age: 30
   output:
     basic_income: 600
     housing_allowance: default

[benjello]

@MattiSG : I am not opposed to a change in terminology.
But I would use the force_default keyword since the variable is set to default for every period.
The other notation is more prone to interpretation and we definitively want to avoid that.

[MattiSG]

the variable is set to default for every period

Ah, that's an important point to note indeed! Then, there is a bit of an issue with the current syntax, as force_default (or neutralized_variables) is at the same level as period. It doesn't seem clear to me that the default value will be enforced for all periods 🤔

What about never_calculate?

[benjello]

What about never_calculate?

Then I would prefer neutralize ;-) but I won't die on that hill.

period could be changed to default period but it would be longer and not add much since you know that you will test for at least one specific period when you write your tests.

[bonjourmauko]

Can't we, actually shoudn't we, align both?

Yes, this is a major issue if both are expected to be used interchangeably. However, I only see references to openfisca test in the doc, and personally only use that one, which makes sense to me as some OpenFisca-specific test machinery / setup is used.

@sandcha under which circumstances do you use pytest? Do you have a specific reason for preferring it over openfisca test?

I think the use of pytest being misleading, that's why in #1020 I propose to use just openfisca test instead.

[benjello]

So if I understand correctly #1020 will fix the test right @maukoquiroga ?

So we will wait for #1020 to get this one in then openfisca/openfisca-doc#252.

cc @MattiSG @HAEKADI @sandcha

[bonjourmauko]

So if I understand correctly #1020 will fix the test right @maukoquiroga ?

So we will wait for #1020 to get this one in then openfisca/openfisca-doc#252.

cc @MattiSG @HAEKADI @sandcha

Most probably, and if there's some more work to do it is lost in the middle of #1031.

By the way, yaml tests in core are not meant to be run "as-is" they're there to test the Yaml Test API (some of them are expected to fail).

[benjello]

@maukoquiroga : I rebased after the recent fix of the doc but there are still problems with this PR I do not understand.
Could you give me a hint ?

[bonjourmauko]

@benjello I think it is because you have to rebase also the corresponding branch in the doc.

[benjello]

@maukoquiroga : I do not understand what I am supposed to do to have the tests passing. Do you mean that I have to merge the doc branch first ?

[benjello]

@maukoquiroga @MattiSG @sandcha : could you give me a hint of what I should fix to have this PR and its companion openfisca/openfisca-doc#252.

Thanks !

[benjello]

@maukoquiroga : I rebased this branch and there is a broken test and I cannot understand why !
Could you give it a look ? Thanks !