[4/4] [test-built] Test against packaged version #1037

bonjourmauko · 2021-08-25T13:03:18Z

Depends on #1059
Fixes #1064

Technical changes

Test openfisca-core against its packaged version
- Doing so allows to prevent publishing corrupted versions
- What we test is what the user gets when installing openfisca-core

MattiSG

Thanks @maukoquiroga! I definitely see the value of testing against the packaged version.

However, I am not 100% of the implementation. I understand it mirrors the one in OpenFisca-France, which already seemed very convoluted to me.

I have many questions, which I added as comments here. Until they are addressed, I am not even certain I can request specific changes.

In terms of implementation, I am a bit concerned that this effort seems to overlap with #1030, #1031 and openfisca/openfisca-france#1663, but I don't see good reason to hold from improving the situation even just for a few days. As discussed yesterday over video, I will try to consolidate a vision for where different steps should be described (CI config vs makefile vs OpenFisca CLI).

Makefile

.circleci/config.yml

MattiSG · 2021-09-03T07:27:00Z

Makefile

+	@## of openfisca-core, the same we put in the hands of users and reusers.
+	pip install --upgrade pip wheel
+	python $< bdist_wheel
+	find dist -name "*.whl" -exec pip install --force-reinstall {}[dev] \;


Why do we reinstall with dev dependencies if this is supposed to be the “packaged” version? 🤔

Wouldn't the find dist -name "*.whl" step more appropriately expressed as find dist -name "*.whl" -exec rm in the clean target, and pip install run after the clean when we want to publish?

Because we're doing it wrong at the outset: we're using pip install .[x] to install dependencies, which is actually to install openfisca core and the dependencies. Those need to be separate steps.

Fixed it, replaced with a more proper -yet not perfect- way of doing it, in the absence of requirement files:

python setup.py egg_info # Create a requirements file. pip install `grep -v "^\[" *.egg-info/requires.txt` # Install openfisca's dependencies.

Note that this is going to install openfisca-core via country-template and extension-template, so we're forced to reinstall openfisca-core.

But, we will reinstall only openfisca-core:

python setup.py bdist_wheel find dist -name "*.whl" -exec pip install --force-reinstall --no-dependencies {} \;

MattiSG

Thank you very much for having answered my questions, and thank you for having refined this PR! It seems to grow, which is good in terms of features but bad in terms of review duration 😅

The current implementation goes against RFC #1040, as it adds many specific commands to the CI. It seems necessary to now converge to having everything in the task manager (which is currently make, for better or for worse). This will enable testing the packaged version and publishing from any machine that can execute the Makefile. The CI should only call it.

I am not sure I understand why this changeset:

Moves all test files and splits them between core and web_api.
Changes some type hints.

MattiSG · 2021-09-23T13:21:38Z

.circleci/config.yml

@@ -17,21 +17,34 @@ jobs:
            python -m venv /tmp/venv/openfisca_core
            echo "source /tmp/venv/openfisca_core/bin/activate" >> $BASH_ENV

+      - run:
+          name: Install base utils
+          command: pip install --upgrade pip setuptools wheel


Since #1040, this should be a make target.

MattiSG · 2021-09-23T13:22:43Z

.circleci/config.yml

+          command: |
+            python setup.py bdist_wheel
+            find dist -name "*.whl" -exec pip install --force-reinstall --no-dependencies {} \;  # https://github.com/openfisca/openfisca-core/pull/1015#discussion_r687051988


Since #1040, this should be a make target.

MattiSG · 2021-09-23T13:24:18Z

.circleci/config.yml

+          name: Run openfisca-core tests
+          command: |
+            make clean check-syntax-errors check-style check-types
+            PYTEST_ADDOPTS="$PYTEST_ADDOPTS --cov=openfisca_core --exitfirst" pytest --pyargs openfisca_core.tests openfisca_web_api.tests  # Run the tests on the built & installed version of openfisca.


Why do we use pytest instead of calling the make target?
Since #1040, this should be a make target.

Sure, this is outdated.

MattiSG · 2021-09-23T13:25:11Z

conftest.py

-    "tests.fixtures.appclient",
-    "tests.fixtures.entities",
-    "tests.fixtures.simulations",
-    "tests.fixtures.taxbenefitsystems",


I am not sure I understand why this was deleted.

Because otherwise we can't predictably test the built packages in an isolated way.

PyTest decided to forbid users to have more than one conftest per project. I don't know why they did that, but it forces us to remove it if we want isolation as we actually ship two packages independently, core and web-api.

One alternative is to ship all together, which makes a bit more sense.

Today we have:

openfisca_core openfisca_web_api tests (not shipped)

With this PR we will have:

openfisca_core openfisca_core.tests etc.

In order for this file to be kept, we would have to do something like

src/openfisca_core str/openfisca_web_api

It is my preferred approach, but it seemed too much of a change.

MattiSG · 2021-09-23T13:26:08Z

.circleci/config.yml

+            python setup.py egg_info  # Create a requirements file.
+            pip install `grep -v "^\[" *.egg-info/requires.txt`  # Install openfisca's dependencies.


Since #1040, this should be a make target.

bonjourmauko · 2021-09-23T18:55:58Z

Thank you very much for having answered my questions, and thank you for having refined this PR! It seems to grow, which is good in terms of features but bad in terms of review duration 😅

The current implementation goes against RFC #1040, as it adds many specific commands to the CI. It seems necessary to now converge to having everything in the task manager (which is currently make, for better or for worse). This will enable testing the packaged version and publishing from any machine that can execute the Makefile. The CI should only call it.

I am not sure I understand why this changeset:

Changes some type hints.

Yes, this predates that conversation, I'll modify the things on that sense.

Moves all test files and splits them between core and web_api.

Because, currently, the tests module is not being packaged, hence there is no possible way to run the tests against the built package —in a way the user would do it, which is the whole idea of the thing.

What ensues is indeed a more standard and opinionated way of structuring a package, as country-template already does.

Changes some type hints.

Me neither: mypy is supposed to ignore the tests module, it can be safely removed.

MattiSG · 2021-09-24T08:53:07Z

Thank you for these clarifications. Let's have a synchronous discussion on the only outstanding issue: packaging the tests 🙂

bonjourmauko added the kind:devops Continuous ops, integration & deployment label Aug 25, 2021

MattiSG reviewed Sep 3, 2021

View reviewed changes

MattiSG mentioned this pull request Sep 3, 2021

Consolidate build workflow #1040

Open

bonjourmauko requested a review from MattiSG September 3, 2021 15:23

bonjourmauko force-pushed the test-build branch 3 times, most recently from 4769d1b to abee5c9 Compare September 9, 2021 10:54

MattiSG requested changes Sep 23, 2021

View reviewed changes

bonjourmauko mentioned this pull request Sep 29, 2021

[1/4] [test-built] Fix yaml tests bug #1048

Merged

bonjourmauko modified the milestone: Improve testing & releases Sep 29, 2021

bonjourmauko added the kind:roadmap A group of issues, constituting a delivery roadmap label Sep 29, 2021

bonjourmauko added this to the Improved releases milestone Sep 29, 2021

bonjourmauko changed the title ~~Test against packaged version~~ [2/2] Test against packaged version Sep 29, 2021

bonjourmauko force-pushed the test-build branch from abee5c9 to 95d696b Compare October 1, 2021 15:29

bonjourmauko changed the title ~~[2/2] Test against packaged version~~ [3/3] Test against packaged version Oct 1, 2021

bonjourmauko changed the base branch from master to fix-build October 1, 2021 15:32

bonjourmauko requested a review from MattiSG October 1, 2021 15:35

bonjourmauko force-pushed the fix-build branch from a0d99ab to ce67c6d Compare October 1, 2021 16:27

bonjourmauko force-pushed the test-build branch from 95d696b to 41cc469 Compare October 1, 2021 16:31

bonjourmauko changed the title ~~[3/3] Test against packaged version~~ [3/3] [test-built] Test against packaged version Oct 1, 2021

bonjourmauko force-pushed the fix-build branch from ce67c6d to 0c4da49 Compare October 1, 2021 16:45

bonjourmauko force-pushed the test-build branch from c97e1d3 to eb80843 Compare October 1, 2021 16:46

bonjourmauko mentioned this pull request Oct 1, 2021

[3/4] [test-built] Adapt package organisation #1059

Draft

bonjourmauko changed the title ~~[3/3] [test-built] Test against packaged version~~ [4/4] [test-built] Test against packaged version Oct 1, 2021

bonjourmauko changed the base branch from fix-build to orga-pack October 1, 2021 17:15

bonjourmauko force-pushed the test-build branch from eb80843 to 53bd5e4 Compare October 1, 2021 17:16

bonjourmauko force-pushed the orga-pack branch from dc083fd to b69236f Compare October 1, 2021 17:42

bonjourmauko force-pushed the test-build branch from 53bd5e4 to c095b09 Compare October 1, 2021 17:43

bonjourmauko force-pushed the orga-pack branch 3 times, most recently from e5d4203 to 2be5572 Compare October 1, 2021 18:02

Mauko Quiroga added 3 commits October 1, 2021 20:05

Add test-built task

d9b8cbb

Adapt circleci

7beeb3b

Bump patch to 35.5.5

1f4043d

bonjourmauko force-pushed the test-build branch from c095b09 to 1f4043d Compare October 1, 2021 18:06

bonjourmauko removed the kind:roadmap A group of issues, constituting a delivery roadmap label Oct 7, 2021

bonjourmauko marked this pull request as draft December 9, 2022 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4/4] [test-built] Test against packaged version #1037

[4/4] [test-built] Test against packaged version #1037

bonjourmauko commented Aug 25, 2021 •

edited

Loading

MattiSG left a comment

MattiSG Sep 3, 2021

MattiSG Sep 3, 2021

bonjourmauko Sep 3, 2021

bonjourmauko Sep 3, 2021

MattiSG left a comment

MattiSG Sep 23, 2021

MattiSG Sep 23, 2021

MattiSG Sep 23, 2021

bonjourmauko Sep 23, 2021

MattiSG Sep 23, 2021

bonjourmauko Sep 23, 2021

MattiSG Sep 23, 2021

bonjourmauko commented Sep 23, 2021

MattiSG commented Sep 24, 2021

		python setup.py egg_info # Create a requirements file.
		pip install `grep -v "^\[" *.egg-info/requires.txt` # Install openfisca's dependencies.

[4/4] [test-built] Test against packaged version #1037

Are you sure you want to change the base?

[4/4] [test-built] Test against packaged version #1037

Conversation

bonjourmauko commented Aug 25, 2021 • edited Loading

Technical changes

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonjourmauko commented Sep 23, 2021

MattiSG commented Sep 24, 2021

bonjourmauko commented Aug 25, 2021 •

edited

Loading