-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix travis config and coverage report. #6884
Conversation
57ae4d7
to
7610861
Compare
@@ -119,6 +119,7 @@ changedir = testing/freeze | |||
# Disable PEP 517 with pip, which does not work with PyInstaller currently. | |||
deps = | |||
pyinstaller | |||
setuptools < 45.0.0 |
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.
Needed to make the installation of pyinstaller
successful. Check out pypa/setuptools#1963
This PR solves #6821. I'll appreciate input on the coverage checks since I'm having a hard time understanding if those numbers look correct or not. |
7610861
to
410db1e
Compare
Thanks a lot @eb-fmezzabotta!
The coverage overall was tweaked a lot by @blueyed, with specific tweaks like running some envs with Did you try to instead copy over the version from @blueyed fork for Travis as suggested in #6870 (comment)? That's probably easier and will have complete coverage. |
@eb-fmezzabotta I've ported @blueyed's changes over at #6889, and it seems to be working. I think we should add that configuration over your changes here because I believe we will fix coverage. 👍 (I would do that myself unfortunately I can't push to your branch) |
I will do that tomorrow, @nicoddemus. Sorry, forgot to allow you guys to push. |
@nicoddemus alright, updated! I had seen that recommendation, but seemed weird to me to remove so many test-envs so I went with the other one, mentioned in #6821 (comment). |
9d5f1a9
to
e5a5643
Compare
Coverage report seems still somewhat odd after fixing the travis config and actually reporting coverage, but seems to me that it's because of having had CI broken for some time 🤔. See that in commit f606fef the CI was already broken so coverage failed to be reported (once in
Check out https://docs.codecov.io/docs/unexpected-coverage-changes See also that 94.16% (the reported coverage percentage for the project in this PR) + 1.78% (the difference reported by codecov against f606fef is actually 95.94%, which is exactly the percentage reported by codecov for the project, for the commit b39b867 (the one before the build broke) |
Hmm problem is now that we don't have Windows coverage anymore. Before moving officially to GitHub actions, we used AppVeyor, but since then have dropped its configuration and disabled it entirely (otherwise it sill tries to build IIRC). How did it fair when you just updated the |
@nicoddemus oh, you meant for me to use both travis and GitHub actions! Did not get that. So do you believe we should have all environments in GH Actions apart from the travis ones? Or we should have only-windows/macos environments there? |
549e32c
to
98826df
Compare
Oh sorry I wasn't clearer!
The Travis configuration contains only a few environments, so I'm OK to leave it together with the others, even if they overlap a bit. 👍 |
ea5cdfe
to
27ca6cf
Compare
27ca6cf
to
c5831ac
Compare
Alright, @nicoddemus! What do you think? |
@nicoddemus bump! (No rush, just making sure you saw the latest update) |
Thanks! Been a bit swamped/busy later, appreciate the ping. Great work! |
No issue whatsoever, thanks for taking the time! I have now updated #6870 :). |
This PR solves #6821 by fixing the Travis CI which was borked, and adding GitHub actions to test some of the other environments and operating systems not being tested in Travis anymore. Also makes sure coverage is 💚.