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

Enable Python warnings during tests and fix all in the code #1233

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Enable Python warnings during tests and fix all in the code #1233

merged 1 commit into from
Nov 26, 2020

Conversation

jdufresne
Copy link
Member

When running the test suite with Python warnings enabled, several
warnings are emitted. They look like:

tests/test_utils.py::test_format_requirement
  /usr/lib64/python3.9/tempfile.py:817: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpezxehep1source'>
    _warnings.warn(warn_message, ResourceWarning)

To solve, make Repository objects a context manager. Upon entering the
with statement, the build an source caches are created and upon exit
they are cleaned up. This ensures they only live for the duration they
are required. This could theoretically be used creating and destroying
any necessary resources. Tests were adjusted.

tests/test_repository_pypi.py::test_pypirepo_build_dir_is_str
  .../pip/_vendor/requests/adapters.py:59: ResourceWarning: unclosed file <_io.FileIO name='/tmp/pytest-of-jon/pytest-70/test_open_local_or_remote_file5/foo.txt' mode='rb' closefd=True>
    super(BaseAdapter, self).__init__()

To solve, test_open_local_or_remote_file__remote_file() was adjusted to
close its file after use.

tests/test_cli_compile.py::test_redacted_urls_in_verbose_output[--find-links]
  .../pip-tools/piptools/scripts/compile.py:306: FutureWarning: --index and --no-index are deprecated and will be removed in future versions. Use --emit-index-url/--no-emit-index-url instead.
    warnings.warn(

To solve, use --no-emit-index-url in
test_redacted_urls_in_verbose_output().

There remains one warning left, but this is out of the control of
pip-tools. An upstream PR has been filed:
pypa/pip#9156

Enabling the warnings in test runs will help catch them earlier and make
debugging/fixing easier.

Changelog-friendly one-liner:

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@jdufresne jdufresne added this to the 5.5.0 milestone Nov 21, 2020
piptools/resolver.py Outdated Show resolved Hide resolved
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Since freshen_build_caches does nothing interesting but yield it can be declared as an abstract method (same as allow_all_wheels). What do you think?

piptools/repositories/base.py Outdated Show resolved Hide resolved
piptools/repositories/local.py Outdated Show resolved Hide resolved
piptools/repositories/pypi.py Outdated Show resolved Hide resolved
@jdufresne
Copy link
Member Author

it can be declared as an abstract method

Can you explain a bit how that helps us, what the benefit is, or what it works towards solving?

When running the test suite with Python warnings enabled, several
warnings are emitted. They look like:

    tests/test_utils.py::test_format_requirement
      /usr/lib64/python3.9/tempfile.py:817: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpezxehep1source'>
        _warnings.warn(warn_message, ResourceWarning)

To solve, make Repository.freshen_build_caches() a context manager. Upon
entering the with statement, the build an source caches are created and
upon exit they are cleaned up. This ensures they only live for the
duration they are required. Tests were adjusted.

    tests/test_repository_pypi.py::test_pypirepo_build_dir_is_str
      .../pip/_vendor/requests/adapters.py:59: ResourceWarning: unclosed file <_io.FileIO name='/tmp/pytest-of-jon/pytest-70/test_open_local_or_remote_file5/foo.txt' mode='rb' closefd=True>
        super(BaseAdapter, self).__init__()

To solve, test_open_local_or_remote_file_remote_file() was adjusted to
close its file after use.

    tests/test_cli_compile.py::test_redacted_urls_in_verbose_output[--find-links]
      .../pip-tools/piptools/scripts/compile.py:306: FutureWarning: --index and --no-index are deprecated and will be removed in future versions. Use --emit-index-url/--no-emit-index-url instead.
        warnings.warn(

To solve, use --no-emit-index-url in
test_redacted_urls_in_verbose_output().

There remains one warning left, but this is out of the control of
pip-tools. An upstream PR has been filed:
pypa/pip#9156

Enabling the warnings in test runs will help catch them earlier and make
debugging/fixing easier.
@jdufresne
Copy link
Member Author

I went ahead with the abstract method approach. The reduction in repetition in indentation is nice.

One downside, this required adding an empty method to FakeRepository to fulfill the ABC requirements. This will be true of any new repository that doesn't require build caches. In practice, this probably won't be an issue going forward, just something to mention w/r/t the trade offs.

Thanks for the reviews!

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thanks!

@jdufresne jdufresne merged commit b09432c into jazzband:master Nov 26, 2020
@jdufresne jdufresne deleted the resource-warnings branch November 26, 2020 16:04
@atugushev atugushev added the tests Testing and related things label Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Testing and related things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants