-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4/4] [test-built] Test against packaged version #1037
base: orga-pack
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
@## 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] \; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we reinstall with dev
dependencies if this is supposed to be the “packaged” version? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {} \;
4769d1b
to
abee5c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andweb_api
. - Changes some type hints.
.circleci/config.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #1040, this should be a make
target.
.circleci/config.yml
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #1040, this should be a make
target.
.circleci/config.yml
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use pytest
instead of calling the make
target?
Since #1040, this should be a make
target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, this is outdated.
conftest.py
Outdated
"tests.fixtures.appclient", | ||
"tests.fixtures.entities", | ||
"tests.fixtures.simulations", | ||
"tests.fixtures.taxbenefitsystems", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand why this was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.circleci/config.yml
Outdated
python setup.py egg_info # Create a requirements file. | ||
pip install `grep -v "^\[" *.egg-info/requires.txt` # Install openfisca's dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #1040, this should be a make
target.
Yes, this predates that conversation, I'll modify the things on that sense.
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.
Me neither: mypy is supposed to ignore the tests module, it can be safely removed. |
Thank you for these clarifications. Let's have a synchronous discussion on the only outstanding issue: packaging the tests 🙂 |
abee5c9
to
95d696b
Compare
a0d99ab
to
ce67c6d
Compare
95d696b
to
41cc469
Compare
ce67c6d
to
0c4da49
Compare
c97e1d3
to
eb80843
Compare
eb80843
to
53bd5e4
Compare
dc083fd
to
b69236f
Compare
53bd5e4
to
c095b09
Compare
e5d4203
to
2be5572
Compare
c095b09
to
1f4043d
Compare
Depends on #1059
Fixes #1064
See openfisca/country-template#59
Technical changes