Skip to content

Conversation

@InSyncWithFoo
Copy link
Contributor

Summary

Follow-up to #16659.

This change adds tests for these three cases, which are (also) not covered by existing tests:

  • # noqa: A (lone incomplete code)
  • # noqa: A123, B (complete codes, last one incomplete)
  • # noqa: A123B (squashed codes, last one incomplete)

The third case is mentioned in the specification, but neither of the other two are:

If an item does not consist entirely of codes (e.g. F401abc), parsing aborts with an error and the user is warned of an InvalidCodeSuffix [...]

Test Plan

Unit tests.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 12, 2025

CodSpeed Performance Report

Merging #16677 will degrade performances by 10.49%

Comparing InSyncWithFoo:noqa (f644fbb) with micha/ruff-0.10 (e9bfdfd)

Summary

❌ 1 regressions
✅ 31 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
red_knot_check_file[incremental] 4.9 ms 5.5 ms -10.49%

@github-actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser requested a review from dylwil3 March 12, 2025 14:16
@MichaReiser MichaReiser added the testing Related to testing Ruff itself label Mar 12, 2025
@dylwil3
Copy link
Collaborator

dylwil3 commented Mar 12, 2025

Thanks!

The third case is mentioned in #16483 (comment), but neither of the other two are

These cases are covered by this part of the spec:

We expect a list of rule codes, separated by commas or whitespace. After splitting first on commas, and then on whitespace, we attempt to parse each item in turn. If parsing a nonempty item returns no rule codes, we stop. If no codes were found at all we warn the user with MissingCodes.

e.g. A is not a valid code, so we stop. There were no preceding codes, so we return MissingCodes, etc.

@dylwil3 dylwil3 merged commit 464ea4a into astral-sh:micha/ruff-0.10 Mar 12, 2025
20 of 21 checks passed
@InSyncWithFoo InSyncWithFoo deleted the noqa branch March 12, 2025 17:12
dylwil3 pushed a commit that referenced this pull request Mar 12, 2025
## Summary

Follow-up to #16677.

This change converts all unit tests (69 of them) in `noqa.rs` to use
inline snapshots instead. It extends the file by more than 1000 lines,
but the tests are now much easier to read and reason about.

## Test Plan

`cargo insta test`.
MichaReiser pushed a commit that referenced this pull request Mar 13, 2025
## Summary

Follow-up to #16659.

This change adds tests for these three cases, which are (also) not
covered by existing tests:

* `# noqa: A` (lone incomplete code)
* `# noqa: A123, B` (complete codes, last one incomplete)
* `# noqa: A123B` (squashed codes, last one incomplete)
MichaReiser pushed a commit that referenced this pull request Mar 13, 2025
## Summary

Follow-up to #16677.

This change converts all unit tests (69 of them) in `noqa.rs` to use
inline snapshots instead. It extends the file by more than 1000 lines,
but the tests are now much easier to read and reason about.

## Test Plan

`cargo insta test`.
MichaReiser pushed a commit that referenced this pull request Mar 13, 2025
## Summary

Follow-up to #16659.

This change adds tests for these three cases, which are (also) not
covered by existing tests:

* `# noqa: A` (lone incomplete code)
* `# noqa: A123, B` (complete codes, last one incomplete)
* `# noqa: A123B` (squashed codes, last one incomplete)
MichaReiser pushed a commit that referenced this pull request Mar 13, 2025
## Summary

Follow-up to #16677.

This change converts all unit tests (69 of them) in `noqa.rs` to use
inline snapshots instead. It extends the file by more than 1000 lines,
but the tests are now much easier to read and reason about.

## Test Plan

`cargo insta test`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to testing Ruff itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants