python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
5.98k stars 325 forks source link

Remove `test_run_black` #2993

Closed A5rocks closed 2 months ago

A5rocks commented 2 months ago

https://github.com/python-trio/trio/pull/2988 was closed, but I think we agree that test_run_black can be removed. I don't actually remember why it was introduced, but using blame all the way back leads to it not seeming necessary. It was made because test_lint_failure wouldn't check that ruff was actually running, so we duplicated that to have a black-specific test and a ruff-specific test. The ruff one is still useful (maybe), but the black one is duplicated.

I'm not so sure any of these tests should exist anymore. I think I kinda pushed for the ruff-specific one but now I think it's ultimately unnecessary. If ruff doesn't run in gen_exports, that's ... fine. We have CI jobs dedicated to checking our code is alright and if it isn't, then that will be caught. Removing that is another PR's job though.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.63%. Comparing base (b4c19bc) to head (229c76b).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2993 +/- ## ========================================== - Coverage 99.63% 99.63% -0.01% ========================================== Files 117 117 Lines 17602 17593 -9 Branches 3174 3173 -1 ========================================== - Hits 17537 17528 -9 Misses 46 46 Partials 19 19 ``` | [Files](https://app.codecov.io/gh/python-trio/trio/pull/2993?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | | |---|---|---| | [src/trio/\_tests/tools/test\_gen\_exports.py](https://app.codecov.io/gh/python-trio/trio/pull/2993?src=pr&el=tree&filepath=src%2Ftrio%2F_tests%2Ftools%2Ftest_gen_exports.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3Rlc3RzL3Rvb2xzL3Rlc3RfZ2VuX2V4cG9ydHMucHk=) | `100.00% <ø> (ø)` | |
A5rocks commented 2 months ago

Yikes looks like GHA sometimes makes macos-latest use the new m1 chips even though setup python doesn't like that: https://github.com/A5rocks/trio/actions/runs/8827503948/job/24234861793

CoolCat467 commented 2 months ago

If ruff doesn't run in gen_exports, that's ... fine. We have CI jobs dedicated to checking our code is alright and if it isn't, then that will be caught. Removing that is another PR's job though.

I think running ruff in gen_exports is important, now that ruff handles import sorting, and if it isn't run then ruff will complain about generated files. I remember working on exactly this at one point.

A5rocks commented 2 months ago

Doesn't look like this loses test coverage. Removing test_run_ruff probably does though, and removing test_lint_failures definitely does. (but I'd argue that coverage loss is beneficial.)

For clarity, when I say "If ruff doesn't run in gen_exports, that's ... fine." I mean "if we break it accidentally" (which is the case the tests guard against).