Test the resolver with the install_deps option
#297
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nour-Mws
force-pushed
the
nour/refactor-resover-tests
branch
5 times, most recently
from
e7443ab to
3725cef
Compare
Nour-Mws
force-pushed
the
nour/test_resolver_with_install_deps
branch
from
c0fc39d to
9160cdd
Compare
Nour-Mws
force-pushed
the
nour/test_resolver_with_install_deps
branch
from
9160cdd to
9a9eb71
Compare
jherland
reviewed
Apr 24, 2023
Nour-Mws
force-pushed
the
nour/test_resolver_with_install_deps
branch
from
d918986 to
dc402f6
Compare
jherland
requested changes
Apr 25, 2023
There was a problem hiding this comment.
I love the idea of using the pytest cache to cache this venv across test runs, but I really don't like having to import pytest in our shipped FawltyDeps code in order to make it work. I think it should be possible to move all the pytest-specific stuff into the test_resolver, and only leave a path to the prepared venv here.
See below for individual suggestions, or look at jherland@841ab91 for a commit on top of this that contains all my suggestions.
Nour-Mws
force-pushed
the
nour/test_resolver_with_install_deps
branch
from
dc402f6 to
8e71b4b
Compare
|
@jherland this is ready for a final look! Thanks a lot for the helpful suggestions and detailed comments. |
This option is not available via the `resolve_dependencies` function which calls `TemporaryPipInstallResolver`. In tests, patch `TemporaryPipInstallResolver` so that it's called with the cache option.
The last changes defined the cache in TemporaryPipInstallResolver with Pytest's cache. We introduce a new class member `cached_venv`, to TemporaryPipInstallResolver. This is only used by `test_resolver()` where it's set to a Pytest's cache directory, thus removing the direct dependency of TemporaryPipInstallResolver on Pytest. Furthermore, the new TemporaryPipInstallResolver context manager introduced in the recent changes reuses code for intsalling requirements and managing the venv from `temp_installed_requirements()`. This commit splits out this duplicate code into a context manager, `installed_requirements()`, that manages the venv creation (if needed) and installs packages. The existing `temp_installed_requirements()` context manager will then only concern itself with creating the temporary directory (and delegate to installed_requirements() for the rest). As this replaces the initialization logic in TemporaryPipInstallResolver with a simple class member that defines the cached_venv, it allows us to avoid depending on pytest-mock for patching the class. Co-authored-by: Johan Herland <[email protected]>
Nour-Mws
force-pushed
the
nour/test_resolver_with_install_deps
branch
from
9f2fd34 to
6ba3523
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I opted to make this into a separate PR from the refactor PR because the battery of hypothesis tests slows down significantly with this change (it now takes ~35 seconds to run tests in
test_resolver.py).The second commit is my attempt to speed things up by installing packages from wheels instead of tarballs. Although the package is installed from the wheel (see log below), the gain in speed is not significant.
Looking in links: /home/nour/.cache/pytest/d/fawltydeps Processing /home/nour/.cache/pytest/d/fawltydeps/leftpadx-0.0.1-py3-none-any.whl Installing collected packages: leftpadx Successfully installed leftpadx-0.0.1I've also tried to skip the
get_tarballs()step (which logically should not involve any time-consuming logic if the tarballs are already downloaded). Unfortunately, I did not see any speed improvement.