Enable flake8-pytest-style #2822

CoolCat467 · 2023-10-19T01:04:13Z

This PR enables the flake8-pytest-style ruff rule.

codecov · 2023-10-19T01:07:00Z

Codecov Report

Merging #2822 (7a8f3ab) into master (60b9e14) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2822      +/-   ##
==========================================
+ Coverage   99.54%   99.55%   +0.01%     
==========================================
  Files         115      115              
  Lines       17663    17658       -5     
  Branches     3158     3160       +2     
==========================================
- Hits        17583    17580       -3     
  Misses         52       52              
+ Partials       28       26       -2

Files	Coverage Δ
src/trio/_core/_tests/test_guest_mode.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_instrumentation.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_io.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_local.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_mock_clock.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_multierror.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_parking_lot.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_run.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_thread_cache.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_windows.py	`100.00% <100.00%> (ø)`
... and 24 more

TeamSpen210 · 2023-10-19T01:55:16Z

Setting the fixture-parentheses option to false in pyproject.toml will make Ruff use @pytest.fixture, and instead warn if empty parentheses are used. It's probably better to be configuring linters to match project conventions, when the existing code is perfectly correct.

webknjaz · 2023-11-11T01:44:45Z

pyproject.toml

@@ -135,6 +136,9 @@ extend-exclude = [
 [tool.ruff.isort]
 combine-as-imports = true

+[tool.ruff.flake8-pytest-style]
+fixture-parentheses = false


🎉

I also like the setting that forces using a sequence of strings in parametrize rather than a comma-separated string...

trio/_core/_tests/test_instrumentation.py

trio/_core/_tests/test_io.py

trio/_tests/tools/test_mypy_annotate.py

trio/_tests/test_windows_pipes.py

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

…t-style

jakkdl

A couple nits, but overall great!

Going to be a ton of merge conflicts though 🙃

src/trio/_tests/test_socket.py

jakkdl · 2023-12-01T14:14:06Z

src/trio/_tests/test_socket.py

@@ -1116,7 +1132,8 @@ async def test_many_sockets() -> None:
        try:
            a, b = stdlib_socket.socketpair()
        except OSError as e:  # pragma: no cover
-            assert e.errno in (errno.EMFILE, errno.ENFILE)
+            # the noqa is "Found assertion on exception `e` in `except` block"
+            assert e.errno in (errno.EMFILE, errno.ENFILE)  # noqa: PT017


This one is quite interesting actually. This one is a loop that breaks prematurely only if the kernel responds with an Out of socket file ids or something error.

hmm, this is a weird test. it loops _x until it gets total//2-1 unless it throws an exception - ~~in which case it'd break and then the test would fail later in the last~~ nowait that's just a print! That print should totally be inside the except catcher and _x can be renamed to _. Probably also a pragma: no cover? codecov is acting weird so idk if it's getting covered or not.

Okay, so, it loops until it fails to create a socket - or it hits total//2. So supposedly it's not an error if it raises earlier, nor is it a problem if it doesn't raise, which means that pytest.raises is not relevant.

It'd maybe be nice to add some comments. Why pytest.raises isn't relevant, and maybe also nice to add that EMFILE is "Too many open files" and ENFILE is "File table overflow".

Fixed I think with c683a7d

src/trio/_core/_tests/test_run.py

jakkdl · 2023-12-01T14:16:52Z

src/trio/_core/_tests/test_run.py

-                assert len(multi_exc.exceptions) == 4
+                # the noqa is for "Found assertion on exception `multi_exc` in `except` block"
+                assert len(multi_exc.exceptions) == 4  # noqa: PT017


oh derp, I should've thought of this one myself. You can leave this one as a noqa though and let me handle it in the multierror PRs

jakkdl · 2023-12-01T14:20:22Z

src/trio/_core/_tests/test_multierror.py

-    with pytest.raises(MultiError):
+    with pytest.raises(MultiError):  # noqa: PT012


PT012 is a bit of a pain when trio so often tests context managers, so not entirely sure it's worth having enabled. It did catch a couple ugly cases this one time though.

jakkdl · 2023-12-01T14:25:19Z

src/trio/_tests/test_highlevel_socket.py

    for code in [errno.EMFILE, errno.EFAULT, errno.ENOBUFS]:
        with assert_checkpoints():
-            with pytest.raises(OSError) as excinfo:
+            with pytest.raises(OSError) as excinfo:  # noqa: PT011  # missing `match`
                await listener.accept()
            assert excinfo.value.errno == code


for code, match in zip((errno.EMFILE, errno.EFAULT, errno.ENOBUFS), ("Out of file descriptors", "attempt to write to read-only memory", "out of buffers"): with assert_checkpoints(): with pytest.raises(OSError, match=match) as excinfo: ... ...

should work?

By the way, it could be useful to have @pytest.mark.parametrize or pytest-subtests here so that separate checks would show up in the test report distinctly.

Changed in f89cd0a. I didn't do the parameterize because it would be a lot of setup code re-running, but if this is important I can still change it.

src/trio/_tests/test_socket.py

src/trio/_tests/test_subprocess.py

jakkdl · 2023-12-01T14:29:22Z

Interesting, the pypy-3.9 runner is segfaulting inside pytest from a from .test_socket import setsockopt_tests line. Any ideas?

Edit: https://github.com/python-trio/trio/actions/runs/7054572593/job/19203654087

it worked on a rerun... 🤷‍♀️

webknjaz · 2023-12-01T17:13:22Z

Interesting, the pypy-3.9 runner is segfaulting inside pytest from a from .test_socket import setsockopt_tests line. Any ideas?
Edit: python-trio/trio/actions/runs/7054572593/job/19203654087

it worked on a rerun... 🤷‍♀️

I've been observing flaky PyPy 3.9 segfaults on GHA in other projects FTR.

Co-authored-by: jakkdl <11260241+jakkdl@users.noreply.github.com>

jakkdl

Can probably ignore the codecov check failing, but it might be nice to make use of it to check out what's not properly getting run - and figure out if it should have pragma: no cover or if you messed with some check that now doesn't run anymore.

jakkdl · 2023-12-05T14:38:18Z

src/trio/_tests/test_socket.py

@@ -1116,7 +1132,8 @@ async def test_many_sockets() -> None:
        try:
            a, b = stdlib_socket.socketpair()
        except OSError as e:  # pragma: no cover
-            assert e.errno in (errno.EMFILE, errno.ENFILE)
+            # the noqa is "Found assertion on exception `e` in `except` block"
+            assert e.errno in (errno.EMFILE, errno.ENFILE)  # noqa: PT017


hmm, this is a weird test. it loops _x until it gets total//2-1 unless it throws an exception - ~~in which case it'd break and then the test would fail later in the last~~ nowait that's just a print! That print should totally be inside the except catcher and _x can be renamed to _. Probably also a pragma: no cover? codecov is acting weird so idk if it's getting covered or not.

Okay, so, it loops until it fails to create a socket - or it hits total//2. So supposedly it's not an error if it raises earlier, nor is it a problem if it doesn't raise, which means that pytest.raises is not relevant.

It'd maybe be nice to add some comments. Why pytest.raises isn't relevant, and maybe also nice to add that EMFILE is "Too many open files" and ENFILE is "File table overflow".

jakkdl · 2023-12-09T12:25:07Z

EDIT: split off to a separate PR #2900 and reverted most of the above commit

jakkdl · 2023-12-09T13:07:13Z

ah no, it was codecov apparently being smart enough to figure out that the assertion in the test was irrelevant given that the raises now has a match= ?! ... or it being too dumb to understand any and accidentally being a genius.

jakkdl · 2023-12-11T14:04:53Z

I would like to merge this unless somebody else wants to review

TeamSpen210

Looks all good. Perhaps we might want to enable the other config too to prevent parentheses in @pytest.mark.whatever() also, but not important.

CoolCat467 · 2023-12-12T01:58:02Z

Perhaps we might want to enable the other config too to prevent parentheses in @pytest.mark.whatever() also, but not important.

I think this PR is a bit big, if we would like to later I think another PR would be best.

CoolCat467 · 2023-12-13T20:20:07Z

Merging, we have two approvals.

WIP enable flake8-pytest-style

482446d

CoolCat467 force-pushed the enable-flake8-pytest-style branch from f7eabe2 to 482446d Compare October 19, 2023 02:17

CoolCat467 added 8 commits October 23, 2023 19:11

Merge branch 'master' into enable-flake8-pytest-style

1e429c3

Merge branch 'master' into enable-flake8-pytest-style

79f0fc4

Merge branch 'master' into enable-flake8-pytest-style

a99aaf2

Merge branch 'master' into enable-flake8-pytest-style

29f9ddc

Autofixes

09f5954

Add todo match arguments

2d56d57

WIP replace match todos

5ef2fe6

WIP match todos

331b85b

webknjaz reviewed Nov 11, 2023

View reviewed changes

trio/_core/_tests/test_instrumentation.py Outdated Show resolved Hide resolved

webknjaz reviewed Nov 11, 2023

View reviewed changes

trio/_core/_tests/test_io.py Outdated Show resolved Hide resolved

webknjaz reviewed Nov 11, 2023

View reviewed changes

trio/_tests/tools/test_mypy_annotate.py Outdated Show resolved Hide resolved

webknjaz reviewed Nov 11, 2023

View reviewed changes

trio/_tests/test_windows_pipes.py Outdated Show resolved Hide resolved

CoolCat467 and others added 12 commits November 10, 2023 21:15

Update trio/_core/_tests/test_instrumentation.py

f4cef61

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

Apply suggestions from code review

ff666ab

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

Merge remote-tracking branch 'origin/master' into enable-flake8-pytes…

2517bc4

…t-style

Merge branch 'master' into enable-flake8-pytest-style

2ed3285

Merge branch 'master' into enable-flake8-pytest-style

d7290cf

Replace more TODO matches

ed1976e

More match argument fixing

c4bf0c6

More match argument fixing

ec43741

Fix more match TODOs

858037a

Merge branch 'master' into enable-flake8-pytest-style

958f963

Fix remaining ruff issues

adddb54

Fix a few remaining windows match TODOs

0cb1b98

CoolCat467 changed the title ~~[WIP] Enable flake8-pytest-style~~ Enable flake8-pytest-style Dec 1, 2023

CoolCat467 added 2 commits November 30, 2023 20:41

Fix more regex errors

21807ef

Fix non-escaped ...

92fb1f1

CoolCat467 requested a review from webknjaz December 1, 2023 02:52

Fix unbalanced parenthesis

3cbcf4a

jakkdl reviewed Dec 1, 2023

View reviewed changes

CoolCat467 and others added 3 commits December 1, 2023 15:30

Merge branch 'master' into enable-flake8-pytest-style

1fc842a

Apply suggestions from code review

f89cd0a

Co-authored-by: jakkdl <11260241+jakkdl@users.noreply.github.com>

Fix is check

ba539d7

CoolCat467 requested a review from jakkdl December 1, 2023 22:21

CoolCat467 added 2 commits December 2, 2023 19:51

Merge branch 'master' into enable-flake8-pytest-style

77cd609

Fix new changes

e054523

jakkdl approved these changes Dec 5, 2023

View reviewed changes

CoolCat467 and others added 3 commits December 6, 2023 00:28

Add comments and simplify test_socket.py

c683a7d

Merge branch 'master' into enable-flake8-pytest-style

b6c1ea0

fix coverage fail - unrelated to this PR

33b6894

jakkdl added 2 commits December 9, 2023 13:49

try coaxing CI to run

dbad60c

pytest.raises(match=) makes assertion irrelevant

b729216

revert test_highlevel_open_tcp_listeners fix, for another PR

7a8f3ab

jakkdl mentioned this pull request Dec 9, 2023

open_tcp_listeners exception __cause__ is always an ExceptionGroup #2900

Merged

TeamSpen210 approved these changes Dec 11, 2023

View reviewed changes

CoolCat467 merged commit f1cb241 into python-trio:master Dec 13, 2023
29 checks passed

CoolCat467 deleted the enable-flake8-pytest-style branch December 20, 2023 19:26

CoolCat467 mentioned this pull request Jan 4, 2024

fix test regexes for musl libc #2917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable flake8-pytest-style #2822

Enable flake8-pytest-style #2822

CoolCat467 commented Oct 19, 2023

codecov bot commented Oct 19, 2023 •

edited

Loading

TeamSpen210 commented Oct 19, 2023

webknjaz Nov 11, 2023

jakkdl left a comment

jakkdl Dec 1, 2023

CoolCat467 Dec 1, 2023 •

edited

Loading

jakkdl Dec 5, 2023

CoolCat467 Dec 6, 2023 •

edited

Loading

jakkdl Dec 1, 2023

jakkdl Dec 1, 2023

jakkdl Dec 1, 2023

webknjaz Dec 1, 2023

CoolCat467 Dec 1, 2023

jakkdl commented Dec 1, 2023

webknjaz commented Dec 1, 2023

jakkdl left a comment

jakkdl Dec 5, 2023

jakkdl commented Dec 9, 2023 •

edited

Loading

jakkdl commented Dec 9, 2023 •

edited

Loading

jakkdl commented Dec 11, 2023

TeamSpen210 left a comment

CoolCat467 commented Dec 12, 2023

CoolCat467 commented Dec 13, 2023

		with pytest.raises(MultiError):
		with pytest.raises(MultiError): # noqa: PT012

Enable flake8-pytest-style #2822

Enable flake8-pytest-style #2822

Conversation

CoolCat467 commented Oct 19, 2023

codecov bot commented Oct 19, 2023 • edited Loading

Codecov Report

TeamSpen210 commented Oct 19, 2023

Choose a reason for hiding this comment

jakkdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoolCat467 Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoolCat467 Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Dec 1, 2023

webknjaz commented Dec 1, 2023

jakkdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Dec 9, 2023 • edited Loading

jakkdl commented Dec 9, 2023 • edited Loading

jakkdl commented Dec 11, 2023

TeamSpen210 left a comment

Choose a reason for hiding this comment

CoolCat467 commented Dec 12, 2023

CoolCat467 commented Dec 13, 2023

codecov bot commented Oct 19, 2023 •

edited

Loading

CoolCat467 Dec 1, 2023 •

edited

Loading

CoolCat467 Dec 6, 2023 •

edited

Loading

jakkdl commented Dec 9, 2023 •

edited

Loading

jakkdl commented Dec 9, 2023 •

edited

Loading