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

PR: Fix and improve Coveralls reporting #268

Merged
merged 4 commits into from
Nov 1, 2021

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Oct 29, 2021

Coveralls is now showing 0 coverage after PR #262 . This PR will fix it, as well as implement some other low-hanging fruit fixes and improvements on top of that.

As it turns out, it was indeed due to the test working dir not matching the one coveralls was running in, due to the hacky workaround to fix the issues from QtPy having a non-src dir layout.

Not only does this fix the issue with coveralls, but it also makes the paths much cleaner, reports coverage from all jobs and with descriptive job names, and combines coverage across all bindings (since naturally a lot of code is specific to one binding), along with fixing/cleaning up a few other things. This makes our total coverage much higher and more representative, bringing it from 25% to 75%, a 3x improvement, as well as makes the coveralls output easier to read and more informative.

  • Fix coveralls reporting showing zero coverage
  • Improve coverage reporting
    • Use .coveragerc config file
    • Provide a more descriptive name for the report
    • Drop repo token and use Github's instead
  • Collect coverage from all bindings
    • Use common env name for all runs and save it
    • Remove common basedir from all runs
    • Combine coverage from all bindings
  • Collect coverage from all runners

Fix #267

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Oct 29, 2021

Sidenote: With setuptools 58.3.0, which just went live between when #262 was merged and now, we are now getting a formal DeprecationWarning when installing the package (which correctly fails the CIs, due to -W error) due to it using the old legacy setup.py backend to setuptools instead of a the modern pyproject.toml-based one that conforms to the PEP 517 standard. I was already planning to migrate in a followup PR to #262 since its pretty straightforward to do and I'm well familiar with it, but this gives it more urgency. For now I'll just drop -W error so we can get this PR merged and handle it in the next one.

@CAM-Gerlach CAM-Gerlach force-pushed the fix-coveralls branch 3 times, most recently from be5ddac to 18e6f43 Compare October 29, 2021 23:33
@CAM-Gerlach CAM-Gerlach force-pushed the fix-coveralls branch 2 times, most recently from 090134d to 5e6e223 Compare October 30, 2021 00:21
@CAM-Gerlach CAM-Gerlach changed the title PR: Fix (and improve) Coveralls reporting PR: Fix and improve Coveralls reporting Oct 30, 2021
@CAM-Gerlach CAM-Gerlach added this to the v2.0.0 milestone Oct 30, 2021
@CAM-Gerlach CAM-Gerlach force-pushed the fix-coveralls branch 2 times, most recently from b795856 to 48ee5d5 Compare October 30, 2021 01:12
@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review October 30, 2021 01:31
@CAM-Gerlach
Copy link
Member Author

@dalthviz As usual, I spent way more time on this than I should have, but this brings our reported coverage from the pre- #262 25% to now 75%, a full 3x improvement, with more detailed, representative and easy to read data. We might want to consider adding coveralls to our formal CI checks, like Spyder has, to check for significant regressions.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @CAM-Gerlach ! LGTM 👍

@dalthviz dalthviz merged commit 664561b into spyder-ide:master Nov 1, 2021
@CAM-Gerlach
Copy link
Member Author

Thanks @dalthviz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coveralls shows 0 coverage despite coverage report and runner output looking fine
2 participants