fpgmaas/deptry

Deptry should detect missing dependencies in tests/

MaxG87 opened this issue · 7 comments

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.

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

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 commented

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.

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.

P.S. Sorry for my slow responses!

MaxG87 commented

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.

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.