Add test coverage for the oldest supported Ruff version #270
Conversation
zanieb
force-pushed
the
ruff-matrix-ci
branch
from
08f11f8 to
7c8a933
Compare
zanieb
force-pushed
the
ruff-matrix-ci
branch
from
d46fe63 to
f315678
Compare
zanieb
force-pushed
the
ruff-matrix-ci
branch
from
98f66e5 to
7f4a0ee
Compare
zanieb
changed the title
| | cut -c2- \ | ||
| ) | ||
| # Get the oldest supported version from the pyproject.toml | ||
| OLDEST=$(rg -No '"ruff>=(.*)"' -r '$1' pyproject.toml) |
There was a problem hiding this comment.
It's way more painful to extract the first matching group like this without ripgrep, afaict
There was a problem hiding this comment.
I would honestly be fine hard-coding this and the latest version, since we already search-and-replace the latest version every time we upgrade the LSP. Fewer dependencies, easier to maintain (IMO). But of course I defer to you.
There was a problem hiding this comment.
sed -rn 's/.*"ruff>=(.*).*",/\1/p' pyproject.toml works
There was a problem hiding this comment.
Maybe it's just me but a sed invocation like that is entirely incomprehensible haha :)
I'd like to try this out since I wrote it up and haven't tried generating matrix test versions like this before, but at the first sign of trouble I'm happy to just switch to hard-coded versions.
| exclude: | ||
| - os: windows-latest | ||
| ruff-version: ${{ needs.ruff-versions.outputs.oldest }} | ||
| - os: macos-latest | ||
| ruff-version: ${{ needs.ruff-versions.outputs.oldest }} |
There was a problem hiding this comment.
It seemed overkill to include these in the matrix — it was a lot of testing
There was a problem hiding this comment.
To me, lowest and highest python versions would be sufficient in general
| class TestServer(unittest.TestCase): | ||
| class TestServer: |
There was a problem hiding this comment.
I need the pytest fixture to be accessible here
There was a problem hiding this comment.
I'd like to follow-up with a pull request that removes the class, but want to keep the diff focused here.
| @@ -149,7 +164,7 @@ def _handler(params): | |||
| { | |||
| "code": "F401", | |||
| "codeDescription": { | |||
| "href": "https://docs.astral.sh/ruff/rules/unused-import" | |||
| "href": expected_docs_url + "rules/unused-import" | |||
There was a problem hiding this comment.
I wish we could just store "fixtures by version" or something, rather than having to construct these and encode facts like "Which URL is present at which version?" in the test logic. Like, in Rust, we'd just run the command with different versions and snapshot the output, which feels easier to maintain and understand. I don't know if I have a concrete suggestion here -- more reacting to the increased logic in the tests now.
There was a problem hiding this comment.
Hm interesting yeah.. this whole bit would make more sense as a snapshot too.
A strong case for us building Python test tooling <3
| | cut -c2- \ | ||
| ) | ||
| # Get the oldest supported version from the pyproject.toml | ||
| OLDEST=$(rg -No '"ruff>=(.*)"' -r '$1' pyproject.toml) |
There was a problem hiding this comment.
I would honestly be fine hard-coding this and the latest version, since we already search-and-replace the latest version every time we upgrade the LSP. Fewer dependencies, easier to maintain (IMO). But of course I defer to you.
| | cut -c2- \ | ||
| ) | ||
| # Get the oldest supported version from the pyproject.toml | ||
| OLDEST=$(rg -No '"ruff>=(.*)"' -r '$1' pyproject.toml) |
There was a problem hiding this comment.
sed -rn 's/.*"ruff>=(.*).*",/\1/p' pyproject.toml works
| exclude: | ||
| - os: windows-latest | ||
| ruff-version: ${{ needs.ruff-versions.outputs.oldest }} | ||
| - os: macos-latest | ||
| ruff-version: ${{ needs.ruff-versions.outputs.oldest }} |
There was a problem hiding this comment.
To me, lowest and highest python versions would be sufficient in general
Adds test coverage for the oldest supported Ruff version by adding the Ruff version to the CI test matrix.
The Ruff versions in the matrix are generated by parsing the lower pin from our
pyproject.tomland the latest release from GitHub. I opted for this approach because it's nice to see the version being tested in the CI job names and it does not require manual updates.The
just checkjob was split out from tests because it requiresruff formatwhich is not available in old releases.The tests did not pass on the old release of Ruff due missing
formatsupport and the new documentation domain. I've added toggles in the test to address those cases based on the Ruff version being tested.Motivated by a desire for test coverage in #266