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

Add more information to contributing page #7916

Merged
merged 42 commits into from Jan 28, 2024
Merged

Conversation

Dreamsorcerer
Copy link
Member

@Dreamsorcerer Dreamsorcerer commented Nov 27, 2023

Mostly revamp coverage section with an example demonstrating how to analyse covecov reports.

I think #7829 works as a perfect example of how codecov is really useful to us.

Preview: https://aiohttp--7916.org.readthedocs.build/en/7916/contributing.html#tests-coverage

@Dreamsorcerer Dreamsorcerer added backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot labels Nov 27, 2023
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 27, 2023
docs/contributing.rst Outdated Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69bbe87) 97.48% compared to head (c949f75) 97.48%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7916   +/-   ##
=======================================
  Coverage   97.48%   97.48%           
=======================================
  Files         107      107           
  Lines       32600    32600           
  Branches     3800     3800           
=======================================
  Hits        31781    31781           
  Misses        616      616           
  Partials      203      203           
Flag Coverage Δ
CI-GHA 97.40% <ø> (ø)
OS-Linux 97.07% <ø> (ø)
OS-Windows 95.57% <ø> (ø)
OS-macOS 96.88% <ø> (ø)
Py-3.10.11 95.50% <ø> (ø)
Py-3.10.13 96.87% <ø> (ø)
Py-3.11.7 96.56% <ø> (ø)
Py-3.12.1 96.69% <ø> (ø)
Py-3.8.10 95.47% <ø> (ø)
Py-3.8.18 96.81% <ø> (ø)
Py-3.9.13 95.47% <ø> (ø)
Py-3.9.18 96.84% <ø> (ø)
Py-pypy7.3.15 96.40% <ø> (ø)
VM-macos 96.88% <ø> (ø)
VM-ubuntu 97.07% <ø> (ø)
VM-windows 95.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/contributing.rst Outdated Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
@Dreamsorcerer Dreamsorcerer enabled auto-merge (squash) January 28, 2024 14:49
@Dreamsorcerer Dreamsorcerer merged commit 822fbc7 into master Jan 28, 2024
28 of 30 checks passed
@Dreamsorcerer Dreamsorcerer deleted the docs-contributing branch January 28, 2024 14:56
Copy link
Contributor

patchback bot commented Jan 28, 2024

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/822fbc7431f3c5522d3e587ad0b658bef8b6a0ab/pr-7916

Backported as #8072

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 28, 2024
Mostly revamp coverage section with an example demonstrating how to
analyse covecov reports.

---------

Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
(cherry picked from commit 822fbc7)
Copy link
Contributor

patchback bot commented Jan 28, 2024

Backport to 3.10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.10/822fbc7431f3c5522d3e587ad0b658bef8b6a0ab/pr-7916

Backported as #8073

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 28, 2024
Mostly revamp coverage section with an example demonstrating how to
analyse covecov reports.

---------

Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
(cherry picked from commit 822fbc7)
Dreamsorcerer added a commit that referenced this pull request Jan 28, 2024
…g page (#8072)

**This is a backport of PR #7916 as merged into master
(822fbc7).**

Co-authored-by: Sam Bull <git@sambull.org>
Dreamsorcerer added a commit that referenced this pull request Jan 28, 2024
…ng page (#8073)

**This is a backport of PR #7916 as merged into master
(822fbc7).**

Co-authored-by: Sam Bull <git@sambull.org>

.. tip::
Sometimes the diff on *codecov.io* doesn't make sense. This is usually
caused by the branch being out of sync with master. Try merging
Copy link
Member

Choose a reason for hiding this comment

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

@Dreamsorcerer this isn't exactly the case I meant. What I'm talking about is GHA attempting to upload coverage and getting HTTP 500 from Codecov API in some or all of the jobs. Sometimes, restarting those jobs doesn't help and it just keeps erroring out for hours.
This means that part of the coverage uploads happened from some envs, but not all. And depending on which ones failed, the coverage may show up lower than it actually is.

To make things worse, by default, the GHA step doing the upload succeeds silently. And the only way to discover if some jobs didn't actually upload coverage is to click through each CI job, expand that step and read the log (it'll either have a URL of the upload for the current commit/PR or some error message at the end of the console output).

Copy link
Member Author

Choose a reason for hiding this comment

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

What I'm talking about is GHA attempting to upload coverage and getting HTTP 500 from Codecov API in some or all of the jobs.

Oh, I think I enabled the option to fail CI if an error occurs in upload (unless it was a different repo), so we shouldn't get that happening anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Different repo, probably. But still, I'm not sure if that option is what we want — it effectively marks entire jobs as failing. And whenever Codecov has platform issues, that means fully blocking merges for hours if not days.

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is that we've had several repos which have been failing coverage uploads for a long time, due to configuration issues. I've personally not seen codecov fail on upload as an issue on their end yet.

Copy link
Member

Choose a reason for hiding this comment

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

I've definitely seen some. Both in aio-libs and other places. It's sometimes flaky on their platform side. And restarts result in HTTP 500, then in several hours it just works when restarted. And I observed these outages affecting repos in different orgs in the same time frame, while their status pages happily reported "no platform issues".

And that is the primary reason https://hynek.me/articles/ditch-codecov-python/ is a thing.
I actually think, parts of that article should be implemented here too. Maybe, not in place of Codecov but additionally to it.

P.S. In the past I also blamed misconfiguration a lot of times. But that doesn't account for when its not the case.

@@ -1,15 +1,23 @@
.. _Adding change notes with your PRs:
Copy link
Member

Choose a reason for hiding this comment

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

Changing this label is going to be problematic as other projects rely on it for referring the changelog guidelines from other places. Not all the PR recommendations set. It'll have to be reverted.

.. _Adding change notes with your PRs:
.. _Making a pull request:

Making a pull request
Copy link
Member

Choose a reason for hiding this comment

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

@Dreamsorcerer this title was a mistake:

  1. Since it's rendered on GitHub in a folder with change notes, on any branch, it shouldn't really mention the PR process that much — just the process for having the changelog.
  2. The Sphinx site now has 2 subsequent titles with the same name but different capitalization — https://docs.aiohttp.org/en/stable/contributing.html#making-a-pull-request and https://docs.aiohttp.org/en/stable/contributing.html#id2.


When making a pull request, please include a short summary of the changes
and a reference to any issue tickets that the PR is intended to solve.
All PRs with code changes should include tests. All changes should
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I missed that this now refers to tests that are not related to the https://github.com/aio-libs/aiohttp/tree/master/CHANGES#readme directory at all.

webknjaz added a commit to webknjaz/aiohttp that referenced this pull request Jan 30, 2024
…#7916)"

This partially reverts commit 822fbc7.

In particular, this drops the top level title from README in the
`CHANGES/` folder and restores the original label.

For the proper Sphinx ToC structuring, a title in the
`docs/contributing.rst` document.
webknjaz added a commit that referenced this pull request Jan 30, 2024
…tion to contributing page (#7916)" (#8109)

**This is a backport of PR #8107 as merged into master
(854e6d8).**

This partially reverts commit 822fbc7.

In particular, this drops the top level title from README in the
`CHANGES/` folder and restores the original label.

For the proper Sphinx ToC structuring, a title in the
`docs/contributing.rst` document.

Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
webknjaz added a commit that referenced this pull request Jan 30, 2024
…ion to contributing page (#7916)" (#8108)

**This is a backport of PR #8107 as merged into master
(854e6d8).**

This partially reverts commit 822fbc7.

In particular, this drops the top level title from README in the
`CHANGES/` folder and restores the original label.

For the proper Sphinx ToC structuring, a title in the
`docs/contributing.rst` document.

Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
webknjaz added a commit to webknjaz/aiohttp that referenced this pull request Jan 30, 2024
@@ -220,47 +260,19 @@ To run spell checker on Linux box you should install it first:
$ sudo apt-get install enchant
$ pip install sphinxcontrib-spelling

Changelog update
Copy link
Member

Choose a reason for hiding this comment

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

@Dreamsorcerer I think you had Sphinx problems because you didn't keep this title as in examples. I applied a partial revert and there's no errors.

webknjaz added a commit that referenced this pull request Jan 30, 2024
patchback bot pushed a commit that referenced this pull request Jan 30, 2024
patchback bot pushed a commit that referenced this pull request Jan 30, 2024
webknjaz added a commit that referenced this pull request Jan 30, 2024
… PR #7916 (#8112)

**This is a backport of PR #8110 as merged into master
(2d8ffdf).**

None

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <sviat@redhat.com>
webknjaz added a commit that referenced this pull request Jan 30, 2024
#7916 (#8111)

**This is a backport of PR #8110 as merged into master
(2d8ffdf).**

None

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <sviat@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot 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

3 participants