-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Disabling retry_persistent_connection in Tests #9396
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9396 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 107 107
Lines 35048 35063 +15
Branches 4150 4151 +1
=======================================
+ Hits 34547 34562 +15
Misses 334 334
Partials 167 167
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hi @Dreamsorcerer, I will make all the suggested changes. I saw many tests are using this ClientSession class, should I make these changes everywhere..? Also, how to pass the Label/Backport Label Check? Thanks for the inputs |
Hi @Dreamsorcerer, I have made all the suggested changes. |
There are a fair number of tests which create a ClientSession directly, but this should cover most tests. We just want a test now to validate the behaviour in tests. |
I'll probably take a look at it over the weekend, but if you'd like to give it a try, then I think we want a test that uses the aiohttp_client fixture with an app that has an endpoint which closes the connection (probably |
Ok, I will try to make a test like that. Can you point me to parts where I can take a look for inspiration while writing tests..? |
Probably any test that uses aiohttp_client currently. The test should be added to test_test_utils.py |
Hi @Dreamsorcerer, I have added the required test, I think it should work |
Thanks, I think that's close to what we want. I'll play with it tomorrow and make some tweaks. |
Cool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if there is anything else which I can try for hacktoberfest.
@ShubhAgarwal-dev while you did successfully change the code, I have feedback regarding using Git, specifically commit message and PR titles and descriptions. This is a very important skill both for yourself and when collaborating on projects with others. Please, check out this collection of articles on the topic: https://gist.github.com/webknjaz/cb7d7bf62c3dda4b1342d639d0e78d79. Following those recommendations will improve the quality of your interactions with Git and git hostings by a lot! Hope this helps :) Regarding more issues, check out https://github.com/aio-libs/aiohttp/issues?q=sort%3Aupdated-desc+is%3Aopen+label%3AHacktoberfest to see if there's anything you're able to understand the context of. |
Thanks for the comments. I will go through the articles and try to adopt the best practices. Once done, It will be my first-time contributing the changes to such a big open source project. |
Alright, updated test should be good now. I also rewrote the change fragment to focus on what a user will want to know. |
Backport to 3.11: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 6d8562d on top of patchback/backports/3.11/6d8562d8420b3e464877b941e04663fdac29a54c/pr-9396 Backporting merged PR #9396 into master
🤖 @patchback |
@ShubhAgarwal-dev Would you mind following the above instructions to create a backport PR? Thanks. |
Ok Cool I will do the same |
I mean the instructions that were just posted. :P |
Depending on conflicts, sometimes the bot does it directly without human intervention. |
(cherry picked from commit 6d8562d)
What do these changes do?
It disables retry logic by default in tests.
Are there changes in behavior for the user?
No
Is it a substantial burden for the maintainers to support this?
No
Related issue number
#9141
Checklist
CHANGES/
folderCONTRIBUTORS.txt