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

Deptry should detect missing dependencies in tests/ #302

Open
MaxG87 opened this issue Feb 22, 2023 · 8 comments
Open

Deptry should detect missing dependencies in tests/ #302

MaxG87 opened this issue Feb 22, 2023 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@MaxG87
Copy link

MaxG87 commented Feb 22, 2023

As of now, deptry simply ignores eveything under tests/. This means that imports that are used but not present will be noticed only at execution time, not before.

For instance, suppose I have a tests/test_sending_request.py with the following content:

import httpretty

def test_sending_request():
    with httpretty.enabled():
        # do stuff
        pass

If httpretty is not included in the dev-dependencies, my test would crash with ModuleNotFoundError, even if I used deptry. Given that deptry can detect exactly this kind of issue for normal dependencies, I would love to see that extended to tests/ and, possibly, other folders as well.

@MaxG87
Copy link
Author

MaxG87 commented Feb 22, 2023

The part regarding possibly, other folders as well is explained in a bit more detail in #300.

@fpgmaas
Copy link
Owner

fpgmaas commented Mar 18, 2023

This would be a nice feature, but it does require us to rethink and redesign the way deptry handles development dependencies. Right now, all development dependencies are placed together in one list, see here. However, if we want to support the functionality mentioned here, we would need to ;

  1. Extract development dependencies as groups (This project has the groups dev and docs)
  2. Map which group of development dependencies should be mapped to which directory. e.g. the group dev should be considered for the tests directory, but the group docs should not.

For point (1) this is already a bit complicated, since deptry supports Poetry, PDM, PEP-621, and requirement.txt.

  • For Poetry, we can simply extract the different groups, although before Poetry 1.2.x, there was no way to group development dependencies (see the docs.
  • For requirements.txt, I think we can allow the user to pass a dict as arguments {'dev' : 'requirements-dev.txt', 'docs' : 'requirements-docs.txt'}.
  • For PEP-621, there does not seem to be a standardized way to list or group development dependencies, see here.
  • The documentation for PDM, also does not seem to include a method to group development dependencies.

So with the lack of standardization, I am a bit hesitant to incorporate this. Maybe I am over-complicating this or overlooking something. Please let me know if you have any feedback or thoughts on the above!

@MaxG87
Copy link
Author

MaxG87 commented Mar 21, 2023

I understand your hesitation here. However, I think much can be gained by using a less complex approach. If deptry continues to place all development dependencies in a single list, it could nonetheless check if a dependency used in tests/ is part of that single list.

This approach would cover a huge use case of simple projects with only prod- and dev-dependencies. It would allow to cover the use case in #300. It would not cover the advanced use cases you presented, but these are not supported as of now neither. So, in a nutshell: Some additional use cases would be supported without, presumably, too much effort.

However, I would totally understand if you would prefer not to implement something that does not cover all use cases.

@fpgmaas
Copy link
Owner

fpgmaas commented Apr 1, 2023

I think your suggestion makes sense. If we were go with the approach you mention, I think we should add a fifth check (next to the four existing checks that deptry scans for; missing development dependency.

In addition to adding the check, we would also need to enable the user to configure directories that use development dependencies, such as scripts and tests. Then, deptry should ignore these directories for the four existing checks, but use it for the fifth check.

Then the remaining question is; is the check 'missing development dependency' worth the (small) additional complexity in the code and configuration? Wouldn't missing development dependency in general already caught be CI/CD pipelines trying to run unit tests? For regular dependencies, finding obsolete ones was originally the main purpose, since those do not raise errors, and also finding missing dependencies was just a nice additional result. But for development dependencies the obsoleteness-check is not possible.

@fpgmaas
Copy link
Owner

fpgmaas commented Apr 1, 2023

P.S. Sorry for my slow responses!

@MaxG87
Copy link
Author

MaxG87 commented Apr 3, 2023

You are totally right, missing dependencies used in unit tests should be noticed when running the tests. However, if deptry would cover that, missing dependencies could be caught by a pre-commit hook or even by a LSP compatible IDE. This would move the check into the developers direct workflow, which might be a nice thing. At least I would welcome that.

However, the most value of a tests check would come from supporting #300. After having very fine-granular dependencies for some time, I am back to lumping everything into group.dev.dependencies if it is not relevant for production. As you mentioned, having Python files in scripts/ being dependencies-checked would be very helpful, because these are not part of the test suite usually.

@fpgmaas
Copy link
Owner

fpgmaas commented Apr 6, 2023

That makes sense! I personally have no time to work on this at the moment, might pick it up in the future if it remains open. In the meantime, if anyone feels like this is something they want to implement I'd be happy to assist or review PR's.

@nathanjmcdougall
Copy link
Contributor

nathanjmcdougall commented Oct 29, 2024

One idea, now that PEP 735 is a thing, is to have a special case to handle the tests and docs dirs, etc., via a special dep group in [dependency-groups] called test, doc, etc.

One approach would just be to disable checking the tests, docs dir, etc. (which is the current default) unless [dependency-groups] is provided, in which case the special dev-dep rules can kick in for these dirs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants