-
Notifications
You must be signed in to change notification settings - Fork 387
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
Run tests without tox #794
Conversation
81c42a3
to
0248347
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #794 +/- ##
==========================================
+ Coverage 90.13% 90.83% +0.69%
==========================================
Files 27 27
Lines 1815 1822 +7
Branches 336 338 +2
==========================================
+ Hits 1636 1655 +19
+ Misses 134 122 -12
Partials 45 45 ☔ View full report in Codecov by Sentry. |
3406a87
to
b6b0e0d
Compare
@hartwork What do you think about this approach? I didn't have any strange errors when running the tests, the suite was executed without the need to run the pipeline to force the tests to turn green. |
c34c7e9
to
e8845a6
Compare
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.
@jairhenrique this reduces coverage with regard to covering with urllib3 1.x and 2.x for the same version of Python, e.g. for Python 3.10 we cover:
py310-requests-urllib3-1
py310-urllib3-1
py310-requests-urllib3-2
py310-urllib3-2
Too keep the same level of coverage and be protected against regressions this would probably need the include
variant of the matrix feature of GitHub Actions. And if we take this route we probably want to git rid of tox altogether to not have to maintain it and not have it uncovered. (Dropping tox will affect which tests contributors are running locally one way or another also.)
4b33f75
to
a342985
Compare
684f16e
to
1a0f17d
Compare
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.
@jairhenrique the general direction is good, some details need fixing:
-
git grep tox
shows more places that need adjustment:# git --no-pager grep tox .gitignore:.tox runtests.sh:# https://blog.ionelmc.ro/2015/04/14/tox-tricks-and-patterns/#when-it-inevitably-leads-to-shell-scripts runtests.sh:# You can and should use WSL for running tox on Windows when it calls bash scripts.
-
The pull request does 5+ different things all in one commit. It would be great to split this up into semantic units using
git rebase -i
. -
Plus everything I commented below:
0295490
to
bbcef92
Compare
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.
@jairhenrique exclude
is a nice idea — it's more future-proof than include
— but there is at least one key showstopper left below:
@jairhenrique did you see this^^? |
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.
@jairhenrique thanks for the adjustments! Is keeping .tox
in .gitignore
intentional?
@hartwork yes, to prevent someone from uploading this folder to the repository. |
@jairhenrique I'm not sure I understand — wouldn't we see an addition like that in the pull request when it's happening? Please help me understand. |
Yes, we would see. I just left it so I would have to avoid this work. I can remove it if you understand that this is a problem. |
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.
@jairhenrique I'm not sure I understand — wouldn't we see an addition like that in the pull request when it's happening? Please help me understand.
Yes, we would see. I just left it so I would have to avoid this work. I can remove it if you understand that this is a problem.
@jairhenrique I'll leave it to you, we'll probably be good either way.
Thanks for the other adjustments, the nice split up into multiple commits — this pull request has come a long way! 👍
@kevin1024 do you agree with these changes? |
fa8016a
to
69b2e3f
Compare
fde1307
to
af7ff0b
Compare
This broke the tornado tests: https://github.com/kevin1024/vcrpy/actions/runs/7620100960/job/20754258427?pr=794#step:5:38 note all the warnings |
actually it looks like it was broken before |
@graingert yes 😞 |
This change remove flaky errors on ci.
Tests are faster with dependency caching.