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

get and enforce 100% coverage #3159

Merged
merged 42 commits into from
Dec 21, 2024
Merged

Conversation

graingert
Copy link
Member

@graingert graingert commented Dec 19, 2024

Using a combination of more tests and TODO comments that work as coverage pragmas

@graingert graingert changed the title test waitid on linux even with pidfd support get more coverage Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (8a3307f) to head (d962e05).
Report is 43 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                  @@
##                main        #3159         +/-   ##
====================================================
+ Coverage   99.62461%   100.00000%   +0.37538%     
====================================================
  Files            124          124                 
  Lines          18381        18427         +46     
  Branches        1226         1215         -11     
====================================================
+ Hits           18312        18427        +115     
+ Misses            47            0         -47     
+ Partials          22            0         -22     
Files with missing lines Coverage Δ
src/trio/_core/_io_kqueue.py 100.00000% <100.00000%> (+12.79999%) ⬆️
src/trio/_core/_io_windows.py 100.00000% <ø> (+1.19047%) ⬆️
src/trio/_core/_run.py 100.00000% <ø> (+0.97751%) ⬆️
src/trio/_core/_tests/test_io.py 100.00000% <100.00000%> (ø)
src/trio/_core/_tests/test_run.py 100.00000% <100.00000%> (ø)
src/trio/_dtls.py 100.00000% <ø> (+2.50000%) ⬆️
src/trio/_tests/test_dtls.py 100.00000% <100.00000%> (+0.20408%) ⬆️
src/trio/_tests/test_exports.py 100.00000% <100.00000%> (+0.37175%) ⬆️
src/trio/_tests/test_socket.py 100.00000% <100.00000%> (+0.15600%) ⬆️
src/trio/_tests/test_ssl.py 100.00000% <100.00000%> (+0.14184%) ⬆️
... and 3 more

... and 1 file with indirect coverage changes

Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Didn't see anything imminently except for a mypy issue

@@ -381,6 +384,38 @@ def check(*, expected_readers: int, expected_writers: int) -> None:
check(expected_readers=1, expected_writers=0)


@pytest.mark.skipif(sys.platform in {"win32", "linux"}, reason="no kqueue")
Copy link
Member

Choose a reason for hiding this comment

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

From mypy run annotations, looks like you need to inform mypy that you don't expect this function to run on windows or linux, because mypy doesn't understand pytest.mark.skipif.
I suggest something like this:

if sys.platform == "win32" or sys.platform == "linux":
    raise AssertionError("Should be unreachable")

Copy link
Member

Choose a reason for hiding this comment

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

@CoolCat467 perhaps, assert_never()?

Copy link
Member

Choose a reason for hiding this comment

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

Or better yet assert sys.platform not in {"win32", "linux"}

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get either option working (assert sys.platform != "win32" and sys.platform != "linux") does work in the playground, and if I create a new file.

@CoolCat467 CoolCat467 self-requested a review December 19, 2024 13:42
Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Prior comment

.codecov.yml Outdated Show resolved Hide resolved
.codecov.yml Outdated Show resolved Hide resolved
.codecov.yml Outdated
Comment on lines 29 to 31
project:
default:
target: 100%
Copy link
Member

Choose a reason for hiding this comment

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

FTR with #3162, there's going to be a need to split the checks into MyPy vs. pytest.

Comment on lines +310 to +317
"abc.abstractmethod",
"if TYPE_CHECKING.*:",
"if _t.TYPE_CHECKING:",
"if t.TYPE_CHECKING:",
"@overload",
'class .*\bProtocol\b.*\):',
"raise NotImplementedError",
'TODO: test this line'
Copy link
Member

Choose a reason for hiding this comment

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

Consider integrating the covdefaults plugin, as it injects most of these automatically. The only thing you'd need to override additionally is the fail_under setting, which it sets to 100%, but it's not compatible with runtime-dependent tests that cannot reach 100% under one specific runtime. So it's good to set it to something sufficiently low while requiring 100% in Codecov.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah nice, I'll do that in a followup

@graingert graingert closed this Dec 20, 2024
@graingert graingert reopened this Dec 20, 2024
@graingert graingert marked this pull request as ready for review December 20, 2024 20:11
@graingert graingert requested a review from CoolCat467 December 20, 2024 20:11
@graingert graingert changed the title get more coverage get and enforce 100% coverage Dec 20, 2024
@graingert graingert requested a review from webknjaz December 21, 2024 09:06
Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

oops, I forgot to submit my pending comments I wrote yesterday

src/trio/_tests/test_exports.py Outdated Show resolved Hide resolved
src/trio/_tools/gen_exports.py Outdated Show resolved Hide resolved
@graingert graingert requested a review from jakkdl December 21, 2024 11:37
Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

nice!

@graingert graingert added this pull request to the merge queue Dec 21, 2024
Merged via the queue into python-trio:main with commit a670d60 Dec 21, 2024
39 checks passed
@graingert graingert deleted the 100-coverage branch December 21, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants