Enable flake8-pytest-style#2822
Conversation
Codecov Report
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
|
|
Setting the fixture-parentheses option to |
f7eabe2 to
482446d
Compare
| combine-as-imports = true | ||
|
|
||
| [tool.ruff.flake8-pytest-style] | ||
| fixture-parentheses = false |
There was a problem hiding this comment.
🎉
I also like the setting that forces using a sequence of strings in parametrize rather than a comma-separated string...
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
jakkdl
left a comment
There was a problem hiding this comment.
A couple nits, but overall great!
Going to be a ton of merge conflicts though 🙃
src/trio/_tests/test_socket.py
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
| 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 |
There was a problem hiding this comment.
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
| assert exc1 != exc3 | ||
|
|
||
| with pytest.raises(MultiError): | ||
| with pytest.raises(MultiError): # noqa: PT012 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
left a comment
There was a problem hiding this comment.
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.
src/trio/_tests/test_socket.py
Outdated
| 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 |
There was a problem hiding this comment.
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".
|
EDIT: split off to a separate PR #2900 and reverted most of the above commit |
|
ah no, it was codecov apparently being smart enough to figure out that the assertion in the test was irrelevant given that the |
|
I would like to merge this unless somebody else wants to review |
TeamSpen210
left a comment
There was a problem hiding this comment.
Looks all good. 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. |
|
Merging, we have two approvals. |
This PR enables the flake8-pytest-style ruff rule.