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

[backport][3.8][PR#5494] Release all forgotten resources in tests #5495

Closed
wants to merge 2 commits into from

Conversation

webknjaz
Copy link
Member

What do these changes do?

Ever since pytest got updated to v6.2, it started emitting resource
warnings under new Pythons 3.8+ which results in a failed status.

This change addresses this issue by properly most of the resources
so they don't even appear and ignoring the rest in cases of race
conditions and other cases that are hard to track down.

Are there changes in behavior for the user?

No.

Related issue number

N/A

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Feb 25, 2021
@webknjaz webknjaz force-pushed the backport/3.8/bugfixes/release-resources-pytest branch 4 times, most recently from 5c87c6a to 06a4149 Compare February 25, 2021 00:57
This change makes sure to release most of the hanging resources
in the lib and tests. They were detected by pytest v6.2+.

PR #5494

(cherry picked from commit 8c82ba1)
@webknjaz webknjaz force-pushed the backport/3.8/bugfixes/release-resources-pytest branch from 06a4149 to fe5e684 Compare February 25, 2021 01:06
Comment on lines 516 to 518
session = aiohttp.ClientSession(loop=loop)
assert session.version == aiohttp.HttpVersion11
await session.close()

Choose a reason for hiding this comment

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

Good afternoon. Maybe it's better to use a context manager for the session?

async with aiohttp.ClientSession(loop=loop) as session:
    assert session.version == aiohttp.HttpVersion11

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's better in general but I wanted to keep the patch as small as possible. Besides, if this fails in tests, we'll notice it and if the test succeeds, it does not really matter if an async CM was used or a regular close was called.

Besides, it's a backport PR, it should be kept as close to the original one as possible.

I'm more concerned about all the warnings that I failed to fix and had to ignore for now because they are either flaky or it is unclear what causes them.

That said, feel free to contribute improvements. I have a lot on my plate so I've only made a bare minimum of the changes necessary to get the new pytest up and running. Strictly speaking, I could just fill the config with ignores but I've made an extra effort to improve things which required substantially more involvement than one would expect at glance.

@webknjaz
Copy link
Member Author

Superseded by #6104.

@webknjaz webknjaz closed this Oct 20, 2021
@webknjaz webknjaz deleted the backport/3.8/bugfixes/release-resources-pytest branch October 20, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants