python / cpython

The Python programming language
https://www.python.org
Other
62.29k stars 29.93k forks source link

Metabug: Improving C-level coverage #94808

Open mdboom opened 2 years ago

mdboom commented 2 years ago

[edit by @encukou, May 2024] The coverage report and checklist are outdated. Run coverage locally before contributing.


This bug is going to be used to track work in a other bugs to improve the C-level coverage of the CPython test suite.

There is a set of baseline coverage results on main [edit: outdated, see above] that can be used to find coverage gaps.

The plan, discussed on discuss.python.org is as follows:

Related work:

There is related work to publish coverage results from CPython on a regular basis, but this issue is concerned with using those results to actually reduce our gaps in coverage.

List of source files:

ezio-melotti commented 2 years ago

@mdboom: I think a better approach would be to create sub-checklists or a single issue per file with another checklist for the paths that need coverage, instead of opening dozen of issues for each individual path.

Even having an issue per file will result in 548 new issues though, so I'm not sure if we want to do that preemptively for each file. I'd say it's better to keep the checklist with the files and paths here, and then directly create PRs for each (or multiple) paths.

Another option is to create a project, and handle it there. You can add a new custom field to specify the file, and create draft issues for each path without creating actual issues here (let me know if you need more help with that).

mdboom commented 2 years ago

I think a better approach would be to create sub-checklists or a single issue per file with another checklist for the paths that need coverage, instead of opening dozen of issues for each individual path.

I thought about that, but most of the uncovered areas will have independent fixes, and bug-per-area sets up that work to happen.

I'm starting with files that have seen a lot of changes lately, so we're seeing quite a few issues in them. I suspect most files will not be that way -- most will probably have no issues, and we can just check the box here and not create a flood of issues where they aren't needed.

Even having an issue per file will result in 548 new issues though, so I'm not sure if we want to do that automatically for each file. I'd say it's better to keep this checklist here, and create PRs.

Agreed.

Another option is to create a project, and handle it there (let me know if you need help with that).

I'm happy to use a project instead if you'd prefer. IIUC, it's pretty easy to move the existing issues already created into it. I know I can't create a project, but once it's created, I don't know what my limited permissions will allow me to do.

ezio-melotti commented 2 years ago

Maybe it would be better to start by trimming down the list to remove files that have no issues, and see how many files are left first.

The remaining files and their paths could be listed here, and if someone starts working on them and wants to discuss the approach they could create the issues lazily or directly create PRs that refers to this meta issues if the fix is straightforward enough.

I don't think having lot of almost empty issues (like https://github.com/python/cpython/issues/94817) will help this effort.

mdboom commented 2 years ago

Maybe it would be better to start by trimming down the list to remove files that have no issues, and see how many files are left first.

I programmatically removed files that have 100% coverage, and then also (manually) removed anything platform specific and all of Modules (which can be handled easily in separate waves later -- they aren't a priority now). This gets us down to 136 tasks from 500+, which is a lot more manageable.

The remaining files and their paths could be listed here, and if someone starts working on them and wants to discuss the approach they could create the issues lazily or directly create PRs that refers to this meta issues if the fix is straightforward enough.

My thinking was this checklist was to say "someone has read through the file and identified all of the potential issues". Some of the issues will have simple tests that can be written, some are dead code, some might reveal bugs, but we need a place to have those discussions and deal with them individually (not here, ideally). For really simple ones, if it's ok to just reference this bug, that's fine by me, but we still need a way to keep track of which files have been vetted to track progress.

I don't think having lot of almost empty issues (like #94817) will help this effort.

Maybe not in general. The ones I've filed so far are directly tied to the faster CPython work, and the ability to move with more confidence there. So there are motivated people who want to close these bugs. But I empathize with the concern of just creating bugs for the sake of creating them.

ezio-melotti commented 2 years ago

This gets us down to 136 tasks from 500+, which is a lot more manageable.

This is much better, thanks for doing this!

My thinking was this checklist was to say "someone has read through the file and identified all of the potential issues".

My idea was to do something like:

Once people start working on these items, there are different options. They can:

  1. create a single PR to fix all 3, and add it to the checklist (maybe next to the filename)
  2. create multiple PRs to fix them individually, and add them to the checklist (as sub-items)
  3. create a single issue for the file with its own checklist where to discuss all 3, add it to the checklist, and then create PRs linked to the issue
  4. create multiple issues for each of the problems in case they need to be discussed individually (what you were doing)

The first two options work for straightforward cases, the third works if the problems are similar, and the last for more complex cases that require discussions. You can also do this incrementally, i.e. start with a list like the one above, then convert some to PRs and some to issues as needed.

In addition, if you hover with the mouse over a checklist item, a ⨀ icon will appear to the right which will allow you to quickly convert the item to an actual issue if/when you decide to open a discussion about a specific file or problem.

So there are motivated people who want to close these bugs.

That's great to hear, I just wanted to avoid creating a bunch of issues that might end up sitting there indefinitely :)

mdboom commented 2 years ago

Thanks. The revised plan you suggested should work just fine. I didn't know about the feature to automatically open an issue from a checklist item.

brandtbucher commented 2 years ago

@pablogsal, should we be backporting these coverage-improving tests to 3.11? I think we should be consistent in our handling of all of them.

I don't think we usually backport test-only PRs, but perhaps these are sort of a special case.

pablogsal commented 2 years ago

Test only PRs are generally ok to backport πŸ‘

ofey404 commented 2 years ago

When someone has "read through" a particular source file and added created subitems for any interesting gaps, they should check it off on the list below and add links to any issues created.

Hi, do you need some more eyes in checking coverage report? I just read Python Developer's Guide and I'm finding some issue with "easy" tag to fix ;)

fatelei commented 2 years ago

i am not sure why i run ./python Lib/test/test_funcattrs.py: test__builtins__ failed

image
mdboom commented 2 years ago

@ofey404 wrote:

Hi, do you need some more eyes in checking coverage report? I just read Python Developer's Guide and I'm finding some issue with "easy" tag to fix

Yes. We could use help both finding coverage holes (by taking a file without a checkmark above and adding entries for anything not well-covered beneath it), or taking any of the holes and trying to write a test to cover it.

mdboom commented 2 years ago

@fatelei wrote:

i am not sure why i run ./python Lib/test/test_funcattrs.py: testbuiltins failed

If you want to run a single test file, you should use:

./python -m test test_funcattrs
MonadChains commented 2 years ago

Hi @mdboom, I tried to improve the test coverage of PyObject_HasAttrString. I'm new to the C API of Python and I hope that my PR can help you πŸ˜„

sobolevn commented 1 year ago

Working on gen_new_with_qualname and API PyGen_NewWithQualName and PyGen_New have no coverage PR πŸ’ͺ

sobolevn commented 1 year ago

Question from https://github.com/python/cpython/pull/98545

Do we care to cover deprecated API?

gvanrossum commented 1 year ago

No, just like we don’t care to fix bugs in deprecated code (security excepted).

sobolevn commented 1 year ago

Thank you, https://github.com/python/cpython/pull/98545 is now closed.

sobolevn commented 1 year ago

@mdboom I have several pending PRs to get merged, and after that I propose to update the meta information / coverage data. There will be a lot of changes in coverage.

Plus, there are new changes to the source code.

What do you think? :)

iritkatriel commented 1 year ago

@sobolevn could you request review from me on these PRs? I'll see if I can work through some of them.

sobolevn commented 1 year ago

WIP:

tqxia commented 1 year ago

Hi, I tried to improve coverage for Objects/dictobject.c with this PR:

100619

BTW, it seems like _PyDict_GetItemHint (which is mentioned in the list) no longer exists.

sobolevn commented 1 year ago

The list is quite outdated :)

mjdominus commented 3 months ago

The checklist above says

PyNumber_Check doesn't test complex

but Objects/abstract.c:905 currently has

return nb && (nb->nb_index || nb->nb_int || nb->nb_float || PyComplex_Check(o));

PyComplex_Check itself seems well-developed in Objects/complexobject.c.

Is this checklist item obsolete?

mjdominus commented 3 months ago

The checklist above says

PyMapping_HasKey and PyMapping_HasKeyString are untested

This appears not to be the case. The baseline coverage report linked in the bug description says they are uncovered, but that report is from 2022. I ran a full lcov today and the functions do appear to be fully covered. as of commit 16c9415fba4 from August 2023. For example:

Screenshot_20240520_131902

Lib/test/test_capi/test_abstract.py lines 440–528 appear to test these functions extensively.

Is this checklist item obsolete?

mdboom commented 3 months ago

Yes, it's quite possible the list is obsolete at this point.

mjdominus commented 3 months ago

Perhaps edit the comment:

If you like, I can check into the other Object/abstract.c items and let you know if they can all be checked off.

encukou commented 3 months ago

I've added a warning about outdated. There is a PR for publishing automatic coverage reports here: https://github.com/python/cpython/pull/94760 @mdboom, do you need help pushing that forward?

mdboom commented 3 months ago

@encukou: Yeah, I haven't really thought about #94760 in a while. It still seems like a worthwhile thing to do, but I'm not sure I will have time in the near future. Feel free to take it over if you are interested.