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

Unintentional dependency on pytest #1473

Closed
tmhofler opened this issue Jun 1, 2020 · 5 comments · Fixed by #1477
Closed

Unintentional dependency on pytest #1473

tmhofler opened this issue Jun 1, 2020 · 5 comments · Fixed by #1477
Assignees

Comments

@tmhofler
Copy link

tmhofler commented Jun 1, 2020

Description of issue

nidcpower has a dependency on pytest (I assume unintentional since it is not listed as a dependency).

System report

python -c "import niscope; niscope.print_diagnostic_information()" output (replace niscope with appropriate package name)

python -c "import nidcpower; nidcpower.print_diagnostic_information()
Traceback (most recent call last):
File "", line 1, in
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower_init_.py", line 10, in
from nidcpower.session import Session # noqa: F401
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower\session.py", line 8, in
import nidcpower._attributes as _attributes
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower_attributes.py", line 3, in
import nidcpower._converters as _converters
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower_converters.py", line 10, in
import pytest
ModuleNotFoundError: No module named 'pytest'

Steps to reproduce issue

pip install nidcpower==1.3.0
python
import nidcpower

Traceback (most recent call last):
File "", line 1, in
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower_init_.py", line 10, in
from nidcpower.session import Session # noqa: F401
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower\session.py", line 8, in
import nidcpower._attributes as _attributes
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower_attributes.py", line 3, in
import nidcpower._converters as _converters
File "C:\Users\nitest.virtualenvs\nidcpower_test-yBwmuPo5\lib\site-packages\nidcpower_converters.py", line 10, in
import pytest
ModuleNotFoundError: No module named 'pytest'

@sbethur
Copy link
Contributor

sbethur commented Jun 1, 2020

Yup, it's an unintentional dependency :( We have some unit tests for converters. In a recent change some unit tests started using pytest.approx. I see couple of things we need to address:

  1. Remove the pytest dep and do a patch release - Should be straight-forward to do.
  2. Ensure we don't add unintentional deps used by unit tests in the future - The unit tests are run only on nifake module. I propose we put the unit tests in converters mako under % if config['module_name'] == 'nifake':. Then flake8 can catch unused deps in the generated converters file.
  3. Create installer test to catch unintentional deps. Not clear how we can do it. But there is an existing issue to address this - No test coverage for verifying installers pull in dependencies correctly #1446. We should prioritize this.

@marcoskirsch, thoughts?

@marcoskirsch
Copy link
Member

I propose we put the unit tests in converters mako under % if config['module_name'] == 'nifake':. Then flake8 can catch unused deps in the generated converters file.

Or move them to their own separate file so that we don't add further conditional logic to the mako template.

@sbethur sbethur self-assigned this Jun 1, 2020
@thejcannon
Copy link

Having the tests be in the same file as the source code is not something I'd expect when installing a Python module.

  • It pollutes the API of the module (attribute discovery will show a bunch of unrelated functions)
  • It can pollute the module's dependencies
  • It bloats the installed modules (more LOC to download and sift through)
  • It makes features such as pytest's conftest.py harder to use.'

If you want people to be able to run tests after installation you should allow installation using an extra (like nidmm[test])

@marcoskirsch
Copy link
Member

If you want people to be able to run tests after installation you should allow installation using an extra (like nidmm[test])

We don't.

@marcoskirsch
Copy link
Member

is not something I'd expect when installing a Python module.

Meeting Python user expectations is important to nimi-python so this is good feedback.

@sbethur sbethur changed the title Unintentional dependency on pytest? Unintentional dependency on pytest Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants