From 822fbc7431f3c5522d3e587ad0b658bef8b6a0ab Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 28 Jan 2024 14:56:43 +0000 Subject: [PATCH] Add more information to contributing page (#7916) Mostly revamp coverage section with an example demonstrating how to analyse covecov reports. --------- Co-authored-by: Sviatoslav Sydorenko --- CHANGES/7916.doc | 1 + CHANGES/README.rst | 29 +- docs/_static/img/contributing-cov-comment.svg | 55 ++ docs/_static/img/contributing-cov-header.svg | 15 + docs/_static/img/contributing-cov-miss.svg | 709 ++++++++++++++++++ docs/_static/img/contributing-cov-partial.svg | 268 +++++++ docs/contributing.rst | 124 +-- docs/spelling_wordlist.txt | 1 + 8 files changed, 1137 insertions(+), 65 deletions(-) create mode 100644 CHANGES/7916.doc create mode 100644 docs/_static/img/contributing-cov-comment.svg create mode 100644 docs/_static/img/contributing-cov-header.svg create mode 100644 docs/_static/img/contributing-cov-miss.svg create mode 100644 docs/_static/img/contributing-cov-partial.svg diff --git a/CHANGES/7916.doc b/CHANGES/7916.doc new file mode 100644 index 00000000000..b616ae85bbe --- /dev/null +++ b/CHANGES/7916.doc @@ -0,0 +1 @@ +Updated :ref:`contributing/Tests coverage ` section to show how we use ``codecov`` -- by :user:`Dreamsorcerer`. diff --git a/CHANGES/README.rst b/CHANGES/README.rst index bf467d2bc07..5beb8999226 100644 --- a/CHANGES/README.rst +++ b/CHANGES/README.rst @@ -1,7 +1,15 @@ -.. _Adding change notes with your PRs: +.. _Making a pull request: + +Making a pull request +===================== + +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 +include a changelog entry. Adding change notes with your PRs -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +--------------------------------- It is very important to maintain a log for news of how updating to the new version of the software will affect @@ -9,7 +17,7 @@ end-users. This is why we enforce collection of the change fragment files in pull requests as per `Towncrier philosophy`_. The idea is that when somebody makes a change, they must record -the bits that would affect end-users only including information +the bits that would affect end-users, only including information that would be useful to them. Then, when the maintainers publish a new release, they'll automatically use these records to compose a change log for the respective version. It is important to @@ -19,7 +27,7 @@ to the end-users most of the time. And so such details should be recorded in the Git history rather than a changelog. Alright! So how to add a news fragment? -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +--------------------------------------- ``aiohttp`` uses `towncrier `_ for changelog management. @@ -34,11 +42,14 @@ for the users to understand what it means. combined with others, it will be a part of the "news digest" telling the readers **what changed** in a specific version of the library *since the previous version*. You should also use -reStructuredText syntax for highlighting code (inline or block), +*reStructuredText* syntax for highlighting code (inline or block), linking parts of the docs or external sites. -If you wish to sign your change, feel free to add ``-- by -:user:`github-username``` at the end (replace ``github-username`` -with your own!). +However, you do not need to reference the issue or PR numbers here +as *towncrier* will automatically add a reference to all of the +affected issues when rendering the news file. +If you wish to sign your change, feel free to add +``-- by :user:`github-username``` at the end (replace +``github-username`` with your own!). Finally, name your file following the convention that Towncrier understands: it should start with the number of an issue or a @@ -77,7 +88,7 @@ necessary to make a separate documentation fragment for documentation changes accompanying the relevant code changes. Examples for adding changelog entries to your Pull Requests -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +----------------------------------------------------------- File :file:`CHANGES/6045.doc.1.rst`: diff --git a/docs/_static/img/contributing-cov-comment.svg b/docs/_static/img/contributing-cov-comment.svg new file mode 100644 index 00000000000..c5ba1005641 --- /dev/null +++ b/docs/_static/img/contributing-cov-comment.svg @@ -0,0 +1,55 @@ + + + + + + + + + + + + + + + + + + + + + Hits 31428 31504 +76 + + + + + + + + + + + + - + + + + Misses 632 633 +1 + + + + + + + + + + + + - + + + + Partials 203 205 +2 + + + diff --git a/docs/_static/img/contributing-cov-header.svg b/docs/_static/img/contributing-cov-header.svg new file mode 100644 index 00000000000..f51c8957cd1 --- /dev/null +++ b/docs/_static/img/contributing-cov-header.svg @@ -0,0 +1,15 @@ + + + + + + + + + Codecov + + + + Report + + diff --git a/docs/_static/img/contributing-cov-miss.svg b/docs/_static/img/contributing-cov-miss.svg new file mode 100644 index 00000000000..d431cd0f1fc --- /dev/null +++ b/docs/_static/img/contributing-cov-miss.svg @@ -0,0 +1,709 @@ + + + + + + + + + + + + + + 733 + + + + + + + + + + + + + + 740 + + + + + + + + + + + + + + + + + + async + + + + + + + + + + def + + + + + + + + + + resolve + + + + + ( + + + + + self + + + + + , + + + + + request + + + + + : + + + + + Request + + + + + ) + + + + + + + + + + - + + + + + > + + + + + _Resolve + + + + + : + + + + + + + + + + + 15 + + + + + + + + + + + + + + + + + 734 + + + + + + + + + + + + + + 741 + + + + + + + + + + + + + + + + + + + + + + + if + + + + + + + + + + ( + + + + + + + + + ! + + + + + + + + + + + + + + + + + + + + + + 735 + + + + + + + + + + + + + 742 + + + + + + + + + + + + + + + not + + + + + request + + + + + . + + + + + url + + + + + . + + + + + raw_path + + + + + . + + + + + startswith + + + + + ( + + + + + self + + + + + . + + + + + _prefix2 + + + + + ) + + + + + + + + + + + + + + + + + + 736 + + + + + + + + + + + + + 743 + + + + + + + + + + + + + + + and + + + + + request + + + + + . + + + + + url + + + + + . + + + + + raw_path + + + + + != + + + + + self + + + + + . + + + + + _prefix + + + + + + + + + + + + + + + + + 737 + + + + + + + + + + + + + 744 + + + + + + + + + + + + + + + ) + + + + + : + + + + + + + + + + + + + + + + + + + 738 + + + + + + + + + + + + + + 745 + + + + + + + + + + + + + + + + + + + + + + + return + + + + + + + + + + None + + + + + , + + + + + + + + + + set + + + + + ( + + + + + ) + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 739 + + + + + + + + + + + + + + 746 + + + + + + + + + + + + + + + + + + match_info + + + + + = + + + + + + + + + + await + + + + + self + + + + + . + + + + + _app + + + + + . + + + + + router + + + + + . + + + + + resolve + + + + + ( + + + + + request + + + + + ) + + + + + + + + + + + 15 + + + + + + + diff --git a/docs/_static/img/contributing-cov-partial.svg b/docs/_static/img/contributing-cov-partial.svg new file mode 100644 index 00000000000..5eceb26b9eb --- /dev/null +++ b/docs/_static/img/contributing-cov-partial.svg @@ -0,0 +1,268 @@ + + + + + + + + + + + + + + 1001 + + + + + + + + + + + + + + + + + + + + url_part + + + + + = + + + + + request + + + + + . + + + + + rel_url + + + + + . + + + + + raw_path + + + + + + + + + + 15 + + + + + + + + + + + + + + + + + 1002 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + while + + + + + url_part + + + + + : + + + + + + + + + ! + + + + + + + + + + + + + + + + + + + + + + + 1003 + + + + + + + + + + + + + + + + + + + + + + + + + for + + + + + candidate + + + + + in + + + + + resource_index + + + + + . + + + + + get + + + + + ( + + + + + url_part + + + + + , + + + + + + + + + + ( + + + + + ) + + + + + ) + + + + + : + + + + + + + + + + + 15 + + + + + + + diff --git a/docs/contributing.rst b/docs/contributing.rst index cd6a613c576..73aba4527dd 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -1,12 +1,12 @@ .. _aiohttp-contributing: Contributing -============ +************ (:doc:`contributing-admins`) Instructions for contributors ------------------------------ +============================= In order to make a clone of the GitHub_ repo: open the link and press the "Fork" button on the upper-right menu of the web page. @@ -25,7 +25,7 @@ Workflow is pretty straightforward: 4. Make sure all tests passed - 5. Add a file into the ``CHANGES`` folder (see `Changelog update`_ for how). + 5. Add a file into the ``CHANGES`` folder (see `Making a pull request`_ for how). 6. Commit changes to your own aiohttp clone @@ -53,7 +53,7 @@ Workflow is pretty straightforward: Preconditions for running aiohttp test suite --------------------------------------------- +============================================ We expect you to use a python virtual environment to run our tests. @@ -116,7 +116,7 @@ Congratulations, you are ready to run the test suite! Run autoformatter ------------------ +================= The project uses black_ + isort_ formatters to keep the source code style. Please run `make fmt` after every change before starting tests. @@ -127,7 +127,7 @@ Please run `make fmt` after every change before starting tests. Run aiohttp test suite ----------------------- +====================== After all the preconditions are met you can run tests typing the next command: @@ -158,35 +158,75 @@ Any extra texts (print statements and so on) should be removed. make test-3.10-no-extensions -Tests coverage --------------- +Code coverage +============= -We are trying hard to have good test coverage; please don't make it worse. +We use *codecov.io* as an indispensable tool for analyzing our coverage +results. Visit https://codecov.io/gh/aio-libs/aiohttp to see coverage +reports for the master branch, history, pull requests etc. -Use: +We'll use an example from a real PR to demonstrate how we use this. +Once the tests run in a PR, you'll see a comment posted by *codecov*. +The most important thing to check here is whether there are any new +missed or partial lines in the report: -.. code-block:: shell +.. image:: _static/img/contributing-cov-comment.svg + +Here, the PR has introduced 1 miss and 2 partials. Now we +click the link in the comment header to open the full report: + +.. image:: _static/img/contributing-cov-header.svg + :alt: Codecov report + +Now, if we look through the diff under 'Files changed' we find one of +our partials: - $ make cov-dev +.. image:: _static/img/contributing-cov-partial.svg + :alt: A while loop with partial coverage. -to run test suite and collect coverage information. Once the command -has finished check your coverage at the file that appears in the last -line of the output: -``open file:///.../aiohttp/htmlcov/index.html`` +In this case, the while loop is never skipped in our tests. This is +probably not worth writing a test for (and may be a situation that is +impossible to trigger anyway), so we leave this alone. -Please go to the link and make sure that your code change is covered. +We're still missing a partial and a miss, so we switch to the +'Indirect changes' tab and take a look through the diff there. This +time we find the remaining 2 lines: +.. image:: _static/img/contributing-cov-miss.svg + :alt: An if statement that isn't covered anymore. -The project uses *codecov.io* for storing coverage results. Visit -https://codecov.io/gh/aio-libs/aiohttp for looking on coverage of -master branch, history, pull requests etc. +After reviewing the PR, we find that this code is no longer needed as +the changes mean that this method will never be called under those +conditions. Thanks to this report, we were able to remove some +redundant code from a performance-critical part of our codebase (this +check would have been run, probably multiple times, for every single +incoming request). + +.. 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 + master into the branch and it will likely fix the issue. Failing + that, try checking coverage locally as described in the next section. + +Other tools +----------- The browser extension https://docs.codecov.io/docs/browser-extension -is highly recommended for analyzing the coverage just in *Files -Changed* tab on *GitHub Pull Request* review page. +is also a useful tool for analyzing the coverage directly from *Files +Changed* tab on the *GitHub Pull Request* review page. + + +You can also produce coverage reports locally with ``make cov-dev`` +or just adding ``--cov-report=html`` to ``pytest``. + +This will run the test suite and collect coverage information. Once +finished, coverage results can be view by opening: +```console +$ python -m webbrowser -n file://"$(pwd)"/htmlcov/index.html +``` Documentation -------------- +============= We encourage documentation improvements. @@ -202,7 +242,7 @@ Once it finishes it will output the index html page Go to the link and make sure your doc changes looks good. Spell checking --------------- +============== We use ``pyenchant`` and ``sphinxcontrib-spelling`` for running spell checker for documentation: @@ -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 ----------------- - -The ``CHANGES.rst`` file is managed using `towncrier -`_ tool and all non trivial -changes must be accompanied by a news entry. -To add an entry to the news file, first you need to have created an -issue describing the change you want to make. A Pull Request itself -*may* function as such, but it is preferred to have a dedicated issue -(for example, in case the PR ends up rejected due to code quality -reasons). - -Once you have an issue or pull request, you take the number and you -create a file inside of the ``CHANGES/`` directory named after that -issue number with an extension of ``.removal``, ``.feature``, -``.bugfix``, or ``.doc``. Thus if your issue or PR number is ``1234`` and -this change is fixing a bug, then you would create a file -``CHANGES/1234.bugfix``. PRs can span multiple categories by creating -multiple files (for instance, if you added a feature and -deprecated/removed the old feature at the same time, you would create -``CHANGES/NNNN.feature`` and ``CHANGES/NNNN.removal``). Likewise if a PR touches -multiple issues/PRs you may create a file for each of them with the -exact same contents and *Towncrier* will deduplicate them. - -The contents of this file are *reStructuredText* formatted text that -will be used as the content of the news file entry. You do not need to -reference the issue or PR numbers here as *towncrier* will automatically -add a reference to all of the affected issues when rendering the news -file. +.. include:: ../CHANGES/README.rst Making a Pull Request ---------------------- +===================== After finishing all steps make a GitHub_ Pull Request with *master* base branch. Backporting ------------ +=========== All Pull Requests are created against *master* git branch. @@ -301,7 +313,7 @@ like *needs backport to 3.1*. merging the backport. How to become an aiohttp committer ----------------------------------- +================================== Contribute! diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index d776db14506..49beecf5432 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -68,6 +68,7 @@ CIMultiDict ClientSession cls cmd +codebase codec Codings committer