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

Remove test_run_black #2993

Merged
merged 2 commits into from Apr 28, 2024
Merged

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Apr 25, 2024

#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.

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
src/trio/_tests/tools/test_gen_exports.py 100.00% <ø> (ø)

@A5rocks
Copy link
Contributor Author

A5rocks commented Apr 25, 2024

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
Copy link
Contributor

CoolCat467 commented Apr 25, 2024

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.

Copy link
Contributor

@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.

I would be for removing this test as well, but I think I remember I tried removing it or something in a previous pull request (I specifically remember the import waffle haha) and I think I recall it lead to a loss of test coverage that was an issue I think. Don't completely remember the details, but thought I'd share.

@A5rocks
Copy link
Contributor Author

A5rocks commented Apr 25, 2024

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).

@A5rocks A5rocks merged commit e1e3eff into python-trio:master Apr 28, 2024
28 checks passed
@A5rocks A5rocks deleted the remove-unnecessary-test branch April 28, 2024 05:28
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.

None yet

3 participants