Skip to content
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

script/cibuild-setup-py: install all dev requirements for testing #42

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

z-bsod
Copy link
Contributor

@z-bsod z-bsod commented Jan 3, 2025

install all requirements stored in requirements-dev instead of only a short hardcoded list for the test of the test after build

install all requirements stored in requirements-dev instead of only a
short hardcoded list
@ross
Copy link
Contributor

ross commented Jan 3, 2025

install all requirements stored in requirements-dev instead of only a short hardcoded list for the test of the test after build

It intentionally doesn't install requirements-dev.txt before running tests to prevent things in that file from being accidental production/runtime dependencies. Those requirements are test-time and rather than development related.

You can see the distinction in setup.py,

tests_require = (
'pytest',
'pytest-cov',
'pytest-network',
# TODO: other test-time requirements
)

and

extras_require={
'dev': tests_require
+ (
# we need to manually/explicitely bump major versions as they're
# likely to result in formatting changes that should happen in their
# own PR. This will basically happen yearly
# https://black.readthedocs.io/en/stable/the_black_code_style/index.html#stability-policy
'black>=24.3.0,<25.0.0',
'build>=0.7.0',
'isort>=5.11.5',
'pyflakes>=2.2.0',
'readme_renderer[md]>=26.0',
'twine>=3.4.2',
),
'test': tests_require,
},

Iirc i looked into installing the test specific requirements with pip in the past with something like

pip install dist/*$VERSION*.whl[test]

but ran into problems/bugs that prevented it from functioning correctly. Even then it would allow things in the test section to be accidental "production" requirements so there's a small(er) risk to that.

@ross
Copy link
Contributor

ross commented Jan 3, 2025

Oh, forgot to mention, setup.py also explicitly doesn't use requirements.txt and sources everything from setup.py since that's what it's specifically testing. requirements*.txt is used for all the other testing.

install test dependencies from setup.py / built wheel file before running pytest
z-bsod pushed a commit to octodns/octodns-autodns that referenced this pull request Jan 7, 2025
don't install complete list of dev requirements for the test execution
as this could mask runtime dependencies to dev requirements as discussed
in octodns/octodns-template#42
@z-bsod
Copy link
Contributor Author

z-bsod commented Jan 7, 2025

i was aware of the difference between the requirements for test and dev in setup.py.

If you list the same test requirements in the ci script as in the requirements_tests in the setup.py file there should be no difference regarding accidential requirements to test dependencies but i don't see any way of resolving this as you can't run the tests without the test requirements.

i just tried running the tests with pip install "${wheelfile}[test]" and it seems to work correctly the only issue is that pip doesn't support file globbing when executed this way.

I updated the script in my MR and tried the modified version in the repo of the autodns-provider and it does seem to work correctly there (example run)

Copy link
Contributor

@ross ross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just tried running the tests with pip install "${wheelfile}[test]" and it seems to work correctly the only issue is that pip doesn't support file globbing when executed this way.

🆒 It was long enough ago that I don't remember what problems(s) I hit with it. Maybe this was it or it was another since addressed shortcoming with pip.

I updated the script in my MR and tried the modified version in the repo of the autodns-provider and it does seem to work correctly there (example run)

👍

@ross ross merged commit 661c1e8 into octodns:main Jan 7, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants