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.

Sign up for GitHub

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

Jump to bottom

Add test coverage for the oldest supported Ruff version #270

Merged
merged 17 commits into from
Oct 11, 2023
Merged

Add test coverage for the oldest supported Ruff version #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7c8a933
Add the latest and oldest Ruff versions to the CI test matrix
zanieb Oct 10, 2023
1c12607
Use sudo for apt-get install
zanieb Oct 10, 2023
e4a270e
Clone the repository
zanieb Oct 10, 2023
f315678
Separate check and test
zanieb Oct 10, 2023
1603030
Remove curl and jq from dependency install
zanieb Oct 10, 2023
8a8f0ca
Cache pip dependencies
zanieb Oct 10, 2023
9a225b4
Only test oldest Ruff version on Ubunutu
zanieb Oct 10, 2023
7f4a0ee
Use exclude instead of include to reduce matrix;
zanieb Oct 10, 2023
ee7fc02
Add format test support for oldest supported version
zanieb Oct 10, 2023
1bed5c6
Add server test support for oldest supported version
zanieb Oct 10, 2023
c7c63e9
Remove extra newlines
zanieb Oct 10, 2023
3fd0e60
Add the conftest file
zanieb Oct 10, 2023
fbd9d55
Lint
zanieb Oct 10, 2023
e356f10
Fix type errors
zanieb Oct 10, 2023
99894e7
Prefix versions in CI job name
zanieb Oct 10, 2023
0d5c284
use space instead of :
zanieb Oct 10, 2023
31be9b8
Actually, use a - to match the os one :0
zanieb Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 60 additions & 3 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,68 @@ on:
branches: [main]

jobs:
ci:
check:
name: Check
runs-on: ubuntu-latest
steps:
- uses: extractions/setup-just@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: "3.11"
cache: 'pip'
- name: Install dependencies
run: just install
- name: Run checks
run: just check
ruff-versions:
name: Generate test versions
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Install dependencies
run: |
sudo apt-get install -y ripgrep
- name: Determine test versions
id: set-versions
run: |
# Get the latest release version from GitHub
LATEST=$( \
curl -L \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/astral-sh/ruff/releases/latest \
| jq '.tag_name' --raw-output \
| cut -c2- \
)
# Get the oldest supported version from the pyproject.toml
OLDEST=$(rg -No '"ruff>=(.*)"' -r '$1' pyproject.toml)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's way more painful to extract the first matching group like this without ripgrep, afaict

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sed -rn 's/.*"ruff>=(.*).*",/\1/p' pyproject.toml works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


echo "::set-output name=versions::[\"$OLDEST\", \"$LATEST\"]"
echo "::set-output name=oldest::$OLDEST
echo "::set-output name=latest::$LATEST
outputs:
versions: ${{ steps.set-versions.outputs.versions }}
oldest: ${{ steps.set-versions.outputs.oldest }}
latest: ${{ steps.set-versions.outputs.latest }}
test:
name: Test (python-${{ matrix.python-version }}, ruff-${{ matrix.ruff-version }}, ${{ matrix.os }})
needs: ruff-versions
strategy:
fail-fast: false
matrix:
python-version: ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12-dev"]
ruff-version: ${{ fromJson(needs.ruff-versions.outputs.versions) }}
os: [ubuntu-latest, macos-latest, windows-latest]

exclude:
- os: windows-latest
ruff-version: ${{ needs.ruff-versions.outputs.oldest }}
- os: macos-latest
ruff-version: ${{ needs.ruff-versions.outputs.oldest }}
Comment on lines +66 to +70
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed overkill to include these in the matrix — it was a lot of testing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, lowest and highest python versions would be sufficient in general


runs-on: ${{ matrix.os }}
steps:
- uses: extractions/setup-just@v1
Expand All @@ -22,9 +78,10 @@ jobs:
- uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: 'pip'
- name: Install dependencies
run: just install
- name: Run checks
run: just check
- name: Install test Ruff version
run: pip install ruff==${{ matrix.ruff-version }}
- name: Run tests
run: just test
6 changes: 4 additions & 2 deletions ruff_lsp/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,7 @@ class Executable(NamedTuple):


def _find_ruff_binary(
settings: WorkspaceSettings, version_requirement: SpecifierSet
settings: WorkspaceSettings, version_requirement: SpecifierSet | None
) -> Executable:
"""Returns the executable along with its version.

Expand All @@ -1063,7 +1063,9 @@ def _find_ruff_binary(
path = _find_ruff_binary_path(settings)

version = _executable_version(path)
if not version_requirement.contains(version, prereleases=True):
if version_requirement and not version_requirement.contains(
version, prereleases=True
):
message = f"Ruff {version_requirement} required, but found {version} at {path}"
show_error(message)
raise RuntimeError(message)
Expand Down
22 changes: 22 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from pathlib import Path

import pytest
from packaging.version import Version

from ruff_lsp.server import _find_ruff_binary, _get_global_defaults, uris
from ruff_lsp.settings import WorkspaceSettings


@pytest.fixture(scope="session")
def ruff_version() -> Version:
# Use the ruff-lsp directory as the workspace
workspace_path = str(Path(__file__).parent.parent)

settings = WorkspaceSettings(
**_get_global_defaults(), # type: ignore[misc]
cwd=None,
workspacePath=workspace_path,
workspace=uris.from_fs_path(workspace_path),
)

return _find_ruff_binary(settings, version_requirement=None).version
23 changes: 18 additions & 5 deletions tests/test_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
from __future__ import annotations

from contextlib import nullcontext

import pytest
from packaging.version import Version
from pygls.workspace import Workspace

from ruff_lsp.server import _format_document_impl
from ruff_lsp.server import (
VERSION_REQUIREMENT_FORMATTER,
_format_document_impl,
)
from tests.client import utils

original = """
Expand All @@ -15,14 +21,21 @@


@pytest.mark.asyncio
async def test_format(tmp_path):
async def test_format(tmp_path, ruff_version: Version):
test_file = tmp_path.joinpath("main.py")
test_file.write_text(original)
uri = utils.as_uri(str(test_file))

workspace = Workspace(str(tmp_path))
document = workspace.get_document(uri)

result = await _format_document_impl(document)
[edit] = result
assert edit.new_text == expected
handle_unsupported = (
pytest.raises(RuntimeError, match=f"Ruff .* required, but found {ruff_version}")
if not VERSION_REQUIREMENT_FORMATTER.contains(ruff_version)
else nullcontext()
)

with handle_unsupported:
result = await _format_document_impl(document)
[edit] = result
assert edit.new_text == expected
35 changes: 25 additions & 10 deletions tests/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@

import os
import tempfile
import unittest
from threading import Event

from packaging.version import Version

from tests.client import defaults, session, utils

# Increase this if you want to attach a debugger
Expand All @@ -16,11 +17,19 @@
print(x)
"""

VERSION_REQUIREMENT_ASTRAL_DOCS = Version("0.0.291")


class TestServer(unittest.TestCase):
class TestServer:
Comment on lines -20 to +23
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need the pytest fixture to be accessible here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to follow-up with a pull request that removes the class, but want to keep the diff focused here.

maxDiff = None

def test_linting_example(self) -> None:
def test_linting_example(self, ruff_version: Version) -> None:
expected_docs_url = (
"https://docs.astral.sh/ruff/"
if ruff_version >= VERSION_REQUIREMENT_ASTRAL_DOCS
else "https://beta.ruff.rs/docs/"
)

with tempfile.NamedTemporaryFile(suffix=".py") as fp:
fp.write(CONTENTS.encode())
fp.flush()
Expand Down Expand Up @@ -60,7 +69,7 @@ def _handler(params):
{
"code": "F401",
"codeDescription": {
"href": "https://docs.astral.sh/ruff/rules/unused-import"
"href": expected_docs_url + "rules/unused-import"
},
"data": {
"fix": {
Expand Down Expand Up @@ -88,7 +97,7 @@ def _handler(params):
{
"code": "F821",
"codeDescription": {
"href": "https://docs.astral.sh/ruff/rules/undefined-name"
"href": expected_docs_url + "rules/undefined-name"
},
"data": {"fix": None, "noqa_row": 3},
"message": "Undefined name `x`",
Expand All @@ -102,9 +111,15 @@ def _handler(params):
],
"uri": uri,
}
self.assertEqual(expected, actual)
assert expected == actual

def test_no_initialization_options(self, ruff_version: Version) -> None:
expected_docs_url = (
"https://docs.astral.sh/ruff/"
if ruff_version >= VERSION_REQUIREMENT_ASTRAL_DOCS
else "https://beta.ruff.rs/docs/"
)

def test_no_initialization_options(self) -> None:
with tempfile.NamedTemporaryFile(suffix=".py") as fp:
fp.write(CONTENTS.encode())
fp.flush()
Expand Down Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm interesting yeah.. this whole bit would make more sense as a snapshot too.

A strong case for us building Python test tooling <3

},
"data": {
"fix": {
Expand Down Expand Up @@ -177,7 +192,7 @@ def _handler(params):
{
"code": "F821",
"codeDescription": {
"href": "https://docs.astral.sh/ruff/rules/undefined-name"
"href": expected_docs_url + "rules/undefined-name"
},
"data": {"fix": None, "noqa_row": 3},
"message": "Undefined name `x`",
Expand All @@ -191,4 +206,4 @@ def _handler(params):
],
"uri": uri,
}
self.assertEqual(expected, actual)
assert expected == actual
Loading