Refactor web_api test using pytest.fixtures

[RamParameswaran]

Context

• This PR begins the process of refactoring the openfisca web_api tests to decouple it from "openfisca_country_template"
• The current tests use data defined in "openfisca_country_template". This is a big anti-pattern 😵. All data used to test of-core should exist in of-core...!
• Currently, the TaxBenefitSystem is instantiated here and imported into all test modules. I recommend we aim to remove this entirely.
• Instead, we should use pytest fixtures to create a TaxBenefitSystem and add only the Variables, Parameters, etc we need for testing

Technical changes

• Create a TaxBenefitSystem fixture in conftest.py. This is a minimal TBS. The intent is that variables and parameters are added within test modules as needed. This keeps test data and test functions in the same location ✔️
• Create a test_client fixture in which we instantiate the variables we need for our tests ✔️
• Refactor test_variables.py to use the test_client fixture instead ✔️

Further steps

• This PR only refactors the test/web_api/test_variables.py module. To decouple all 'web_api' tests from the openfisca_country_template package, we need to also refactor test_calculate.py, test_entities.py, test_spec.py and test_trace.py.

Notes

• Failing tests are fixed by branch 'fix-doc-test-api'

Let me know your thoughts!
Thanks ✌️

[RamParameswaran]

Update

Upon @cesco-fran 's recommendation (thanks!) I have updated the scope for the 'test_tax_benefit_system' fixture to "module". This balances runtime (so we don't have to initialise a new tax-benefit-system for every test), with error potential due to tax-benefit-systems being mutable.

I think this PR is in any case an improvement on the current state, which mutates the TaxBenefitSystem object from the openfisca_country_template.

[bonjourmauko]

Thanks a lot @RamParameswaran ! Looks good. Regarding context:

1. There's a reason™ why tests were factored out to OpenFisca Template but I'm not able to find the relevant discussion here un Github, maybe @sandcha @fpagnoux could be of help - I guess it is this one here Use country template to run core tests #507

2. At the same time, we'll be facing problems due to circular dependencies starting from the next release of pip, as discovered here Upgrade numpy to add support for M1 processors #990

I think the idea behind the current situation is to have integration testing be done against a lively source of real-world examples and scenarios that, by the dependency between both, has to be always up to date - maintaining two different sets of templates proved burdensome, but keeping the current situation seems impossible also due to next changes to the dependencies ecosystem.

It would be great to have more comments and insight from the community to see what's the best route to go. I'll mark this contribution as RFC and follow it closely.

😃

[MattiSG]

Thanks @RamParameswaran for this contribution and thanks @maukoquiroga for the initial reply! 😃

There's a reason™ why tests were factored out to OpenFisca Template

Indeed, I decided back in 2017 to rely on the Country Template for testing, which resulted in #507, to solve two problems at once: (i) the lack of maintenance of the Country Template, (ii) the lack of integration tests in Core.

No matter how much testing we did on Core, there were always breaks in production which could only be caught once Core was loaded in a full model environment; on the other hand, the Country Template is only a burden for maintainers unless it is part of their standard testing procedure.

I appreciate how this PR improves software testing quality by ensuring fixtures are maintained in the same place as expected results are declared —this would have avoided #980, for example 🙂
However, I'm afraid that if we are to migrate all tests within Core, we will get back to the initial two problems outlined above, which in my opinion are bigger ones than outdated strings every few years, for which workarounds such as #987 can mitigate impact further.

I would thus be in favour of merging this if dedicated integration tests, probably still relying on the Country Template, are also added to the testing procedure 🙂

[RamParameswaran]

Thanks for your reply @MattiSG :)

I should note that the circular dependency on the Country Template also makes is very difficult to write functional tests for new features. An example is #983. In order to write functional tests for #983, we must: i) merge #983, ii) add the necessary test data to Country Template, iii) create a new PR with functional tests for #983.

However, I completely understand your point about the necessity to maintain the Country Template. For this purpose I support having dedicated integration tests in of-core.

Regarding the issues found in #990 - pip dependency resolution is a mystery to me so I can't really comment 😅

[bonjourmauko]

Hi @RamParameswaran ! You can actually install from Github like this in order to test:

pip install --editable git+https://github.com/openfisca/country-core.git@BRANCH_NAME#egg=OpenFisca-Core

But yes it is definitely not ideal.

I think a first move towards a solution would be to clearly separate unit, functional and integration tests, and keep country template's scenarios for testing only de latter. I'm taking a look at both this PR and #969 to come up with an RFC.

[RamParameswaran]

Hi all. I have updated this PR to align with #997.

• style fixes
• introduces a test_client fixture which replaces the test_client object that was previously defined in "tests/web_api/init.py"

@MattiSG to address your above concerns, this PR no longer aims to reduce the dependence on Country-Template. Instead it simply modularises the web_api test suite so it uses the pytest fixtures defined in "test/fixtures/". These fixtures themselves are created from the Country-Template 👌

As always, happy to hear your thoughts! 😄

[bonjourmauko]

✨ thanks @RamParameswaran ! Just the minor comment above on the underscore in the client argument otherwise LGTM 💯