From d60690ce46ae522b7342b422c7d9e2bc99f7bdc1 Mon Sep 17 00:00:00 2001 From: Simon de Vlieger Date: Mon, 20 Mar 2023 11:54:35 +0100 Subject: [PATCH] tox: add tox `tox` is a standard testing tool for Python projects, this allows you to test locally with all your installed Python version with the following command: `tox -m test -p all` To run the tests in parallel for all supported Python versions. To run linters or type analysis: ``` tox -m lint -p all tox -m type -p all ``` This commit *also* disables the `import-error` warning from `pylint`, not all Python versions have the system-installed Python libraries available and they can't be fetched from PyPI. Some linters have been added and the general order linters run in has been changed. This allows for quicker test failure when running `tox -m lint`. As a consequence the `test_pylint` test has been removed as it's role can now be fulfilled by `tox`. Other assorted linter fixes due to newer versions: - use a str.join method (`consider-using-join`) - fix various (newer) mypy and pylint issues - comments starting with `#` and no space due to `autopep8` This also changes our CI to use the new `tox` setup and on top of that pins the versions of linters used. This might move into separate requirements.txt files later on to allow for easier updating of those dependencies. --- .github/workflows/check.yml | 72 ++++++++++++ .github/workflows/differential-shellcheck.yml | 24 ---- .../workflows/{checks.yml => generate.yml} | 22 +--- .github/workflows/{tests.yml => test.yml} | 22 ++-- .gitignore | 4 +- .ruff.toml | 6 + assemblers/org.osbuild.oci-archive | 2 +- assemblers/org.osbuild.qemu | 4 +- osbuild/buildroot.py | 4 +- osbuild/formats/v2.py | 8 +- osbuild/host.py | 4 +- osbuild/loop.py | 2 +- osbuild/objectstore.py | 2 +- osbuild/pipeline.py | 2 +- osbuild/util/ostree.py | 5 +- setup.cfg | 3 +- stages/org.osbuild.dracut | 2 +- stages/org.osbuild.first-boot | 2 +- stages/org.osbuild.grub2 | 2 +- stages/org.osbuild.oci-archive | 2 +- stages/org.osbuild.ovf | 6 +- test/src/test_pylint.py | 110 ------------------ tox.ini | 67 +++++++++++ 23 files changed, 193 insertions(+), 184 deletions(-) create mode 100644 .github/workflows/check.yml delete mode 100644 .github/workflows/differential-shellcheck.yml rename .github/workflows/{checks.yml => generate.yml} (68%) rename .github/workflows/{tests.yml => test.yml} (65%) create mode 100644 .ruff.toml delete mode 100644 test/src/test_pylint.py create mode 100644 tox.ini diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml new file mode 100644 index 00000000..feb36d06 --- /dev/null +++ b/.github/workflows/check.yml @@ -0,0 +1,72 @@ +name: Checks + +on: [pull_request, push] + +permissions: + contents: read + +jobs: + spelling_checker: + name: "Spelling" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: codespell-project/actions-codespell@master + with: + ignore_words_list: msdos, pullrequest + skip: ./.git,coverity,rpmbuild,samples + + python_code_linters: + name: "Python Linters" + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + linter: + - "ruff" + - "pylint" + - "autopep8" + - "isort" + steps: + - name: "Clone Repository" + uses: actions/checkout@v3 + - name: "Run Linters" + uses: osbuild/containers/src/actions/privdocker@552e30cf1b4ed19c6ddaa57f96c342b3dff4227b + with: + image: ghcr.io/osbuild/osbuild-ci:latest-202304251412 + run: | + tox -e "${{ matrix.linter }}" + + python_code_types: + name: "Python Typing" + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + typer: + - "mypy" + steps: + - name: "Clone Repository" + uses: actions/checkout@v3 + - name: "Run Linters" + uses: osbuild/containers/src/actions/privdocker@552e30cf1b4ed19c6ddaa57f96c342b3dff4227b + with: + image: ghcr.io/osbuild/osbuild-ci:latest-202304251412 + run: | + tox -e "${{ matrix.typer }}" + + shell_linters: + name: "Shell Linters" + runs-on: ubuntu-latest + + steps: + - name: "Clone Repository" + uses: actions/checkout@v3 + with: + fetch-depth: 0 + + - name: "Differential ShellCheck" + uses: redhat-plumbers-in-action/differential-shellcheck@v3 + with: + severity: warning + token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/differential-shellcheck.yml b/.github/workflows/differential-shellcheck.yml deleted file mode 100644 index 223984f4..00000000 --- a/.github/workflows/differential-shellcheck.yml +++ /dev/null @@ -1,24 +0,0 @@ -name: "Differential ShellCheck" -on: - pull_request: - branches: [main] - -permissions: - contents: read - -jobs: - lint: - name: "Differential Shell Check" - runs-on: ubuntu-latest - - steps: - - name: "Clone Repository" - uses: actions/checkout@v3 - with: - fetch-depth: 0 - - - name: "Differential ShellCheck" - uses: redhat-plumbers-in-action/differential-shellcheck@v3 - with: - severity: warning - token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/checks.yml b/.github/workflows/generate.yml similarity index 68% rename from .github/workflows/checks.yml rename to .github/workflows/generate.yml index af1bbb75..4e148b38 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/generate.yml @@ -1,10 +1,10 @@ -name: Checks +name: Generate on: [pull_request, push] jobs: - documentation: - name: "📚 Documentation" + generate_documentation: + name: "Documentation" runs-on: ubuntu-latest container: image: docker.io/library/python:3.7 @@ -33,8 +33,8 @@ jobs: test -d docs test -f docs/osbuild.1 - test_data: - name: "Regenerate Test Data" + generate_test_data: + name: "Test Data" runs-on: ubuntu-latest steps: - name: "Clone Repository" @@ -42,17 +42,7 @@ jobs: - name: "Regenerate Test Data" uses: osbuild/containers/src/actions/privdocker@552e30cf1b4ed19c6ddaa57f96c342b3dff4227b with: - image: ghcr.io/osbuild/osbuild-ci:latest-202304110753 + image: ghcr.io/osbuild/osbuild-ci:latest-202304251412 run: | make test-data git diff --exit-code -- ./test/data - - codespell: - name: "Spell check" - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - uses: codespell-project/actions-codespell@master - with: - ignore_words_list: msdos, pullrequest - skip: ./.git,coverity,rpmbuild,samples diff --git a/.github/workflows/tests.yml b/.github/workflows/test.yml similarity index 65% rename from .github/workflows/tests.yml rename to .github/workflows/test.yml index 7d50c190..4d7abdff 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/test.yml @@ -4,7 +4,7 @@ on: [pull_request, push] jobs: test_suite: - name: "Test Suite" + name: "Unittest" runs-on: ubuntu-latest strategy: fail-fast: false @@ -19,17 +19,21 @@ jobs: - "test.run.test_noop" - "test.run.test_sources" - "test.run.test_stages" - - "test.src" + environment: + - "py36" + - "py37" + - "py38" + - "py39" + - "py310" + - "py311" + - "py312" steps: - name: "Clone Repository" uses: actions/checkout@v3 - - name: "Run Tests" + - name: "Run" uses: osbuild/containers/src/actions/privdocker@552e30cf1b4ed19c6ddaa57f96c342b3dff4227b with: - image: ghcr.io/osbuild/osbuild-ci:latest-202304110753 + image: ghcr.io/osbuild/osbuild-ci:latest-202304251412 run: | - python3 -m pytest \ - --pyargs "${{ matrix.test }}" \ - --rootdir=. \ - --cov-report=xml --cov=osbuild \ - -v + TEST_CATEGORY="${{ matrix.test }}" \ + tox -e "${{ matrix.environment }}" diff --git a/.gitignore b/.gitignore index 3e4f64b9..50a3602f 100644 --- a/.gitignore +++ b/.gitignore @@ -20,4 +20,6 @@ cov-int/ /docs/osbuild-manifest.5 venv -.venv \ No newline at end of file +.venv + +/.tox diff --git a/.ruff.toml b/.ruff.toml new file mode 100644 index 00000000..98c97c6e --- /dev/null +++ b/.ruff.toml @@ -0,0 +1,6 @@ +line-length = 120 + +ignore = [ + "E741", # ambiguous variable names + "E501", # line too long +] diff --git a/assemblers/org.osbuild.oci-archive b/assemblers/org.osbuild.oci-archive index b2db43bf..7f2c5eab 100755 --- a/assemblers/org.osbuild.oci-archive +++ b/assemblers/org.osbuild.oci-archive @@ -228,7 +228,7 @@ def create_oci_dir(tree, output_dir, options): blobs = os.path.join(output_dir, "blobs", "sha256") os.makedirs(blobs) - ## layers / rootfs + # layers / rootfs digest, info = blobs_add_layer(blobs, tree) diff --git a/assemblers/org.osbuild.qemu b/assemblers/org.osbuild.qemu index e7caca73..1d383892 100755 --- a/assemblers/org.osbuild.qemu +++ b/assemblers/org.osbuild.qemu @@ -502,7 +502,7 @@ def grub2_partition_id(pt: PartitionTable): return label2grub[pt.label] -#pylint: disable=too-many-branches +# pylint: disable=too-many-branches def install_grub2(image: str, pt: PartitionTable, options): """Install grub2 to image""" platform = options.get("platform", "i386-pc") @@ -637,7 +637,7 @@ def install_zipl(root: str, device: str, pt: PartitionTable): check=True) -#pylint: disable=too-many-branches +# pylint: disable=too-many-branches def main(tree, output_dir, options, loop_client): fmt = options["format"] filename = options["filename"] diff --git a/osbuild/buildroot.py b/osbuild/buildroot.py index 0c33c5ee..5b47d70e 100644 --- a/osbuild/buildroot.py +++ b/osbuild/buildroot.py @@ -15,7 +15,7 @@ import stat import subprocess import tempfile import time -from typing import Optional, Set +from typing import Set from osbuild.api import BaseAPI from osbuild.util import linux @@ -101,7 +101,7 @@ class BuildRoot(contextlib.AbstractContextManager): self.proc = None self.tmp = None self.mount_boot = True - self.caps: Optional[set] = None + self.caps = None @staticmethod def _mknod(path, name, mode, major, minor): diff --git a/osbuild/formats/v2.py b/osbuild/formats/v2.py index 5640b6ac..c0366d5e 100644 --- a/osbuild/formats/v2.py +++ b/osbuild/formats/v2.py @@ -2,7 +2,7 @@ Second, and current, version of the manifest description """ -from typing import Any, Dict +from typing import Any, Dict, Optional from osbuild.meta import Index, ModuleInfo, ValidationResult @@ -197,7 +197,7 @@ def sort_devices(devices: Dict) -> Dict: desc = devices[name] parent = desc.get("parent") - if parent and not parent in result: + if parent and parent not in result: # if the parent is not in the `result` list, it must # be in `todo`; otherwise it is missing if parent not in todo: @@ -391,8 +391,8 @@ def load(description: Dict, index: Index) -> Manifest: return manifest -#pylint: disable=too-many-branches -def output(manifest: Manifest, res: Dict, store: ObjectStore = None) -> Dict: +# pylint: disable=too-many-branches +def output(manifest: Manifest, res: Dict, store: Optional[ObjectStore] = None) -> Dict: """Convert a result into the v2 format""" def collect_metadata(p: Pipeline) -> Dict[str, Any]: diff --git a/osbuild/host.py b/osbuild/host.py index 4857c2ac..c2dd87cf 100644 --- a/osbuild/host.py +++ b/osbuild/host.py @@ -272,7 +272,7 @@ class Service(abc.ABC): # an exception in `sock.send` later. self._check_fds(reply_fds) - except: # pylint: disable=bare-except + except Exception: # pylint: disable=broad-exception-caught reply_fds = self._close_all(reply_fds) _, val, tb = sys.exc_info() reply = self.protocol.encode_exception(val, tb) @@ -351,7 +351,7 @@ class ServiceClient: def call_with_fds(self, method: str, args: Optional[Union[List[str], Dict[str, Any]]] = None, fds: Optional[List[int]] = None, - on_signal: Callable[[Any, Optional[Iterable[int]]], None] = None + on_signal: Optional[Callable[[Any, Optional[Iterable[int]]], None]] = None ) -> Tuple[Any, Optional[Iterable[int]]]: """ Remotely call a method and return the result, including file diff --git a/osbuild/loop.py b/osbuild/loop.py index 5e1725f0..a0f5e811 100644 --- a/osbuild/loop.py +++ b/osbuild/loop.py @@ -119,7 +119,7 @@ class Loop: self.devname = f"loop{minor}" self.minor = minor - self.on_close: Optional[Callable[["Loop"], None]] = None + self.on_close = None with contextlib.ExitStack() as stack: if not dir_fd: diff --git a/osbuild/objectstore.py b/osbuild/objectstore.py index d8f2cba8..4a19ce9f 100644 --- a/osbuild/objectstore.py +++ b/osbuild/objectstore.py @@ -338,7 +338,7 @@ class ObjectStore(contextlib.AbstractContextManager): @property def active(self) -> bool: - #pylint: disable=protected-access + # pylint: disable=protected-access return self.cache._is_active() @property diff --git a/osbuild/pipeline.py b/osbuild/pipeline.py index 8013df0d..ab43387f 100644 --- a/osbuild/pipeline.py +++ b/osbuild/pipeline.py @@ -383,7 +383,7 @@ class Manifest: def __init__(self): self.pipelines = collections.OrderedDict() - self.sources: List[Source] = [] + self.sources = [] def add_pipeline( self, diff --git a/osbuild/util/ostree.py b/osbuild/util/ostree.py index 70600397..6c5e4232 100644 --- a/osbuild/util/ostree.py +++ b/osbuild/util/ostree.py @@ -6,7 +6,8 @@ import subprocess import sys import tempfile import typing -from typing import Any, List +# pylint doesn't understand the string-annotation below +from typing import Any, List # pylint: disable=unused-import from .types import PathLike @@ -222,7 +223,7 @@ class SubIdsDB: """ def __init__(self) -> None: - self.db: collections.OrderedDict[str, Any] = collections.OrderedDict() + self.db: 'collections.OrderedDict[str, Any]' = collections.OrderedDict() def read(self, fp) -> int: idx = 0 diff --git a/setup.cfg b/setup.cfg index a9e35cb8..cd92f299 100644 --- a/setup.cfg +++ b/setup.cfg @@ -9,7 +9,8 @@ disable=missing-docstring, consider-using-with, consider-using-from-import, line-too-long, - useless-option-value + useless-option-value, + import-error [pylint.TYPECHECK] ignored-classes=osbuild.loop.LoopInfo diff --git a/stages/org.osbuild.dracut b/stages/org.osbuild.dracut index e98a25dc..8fb43761 100755 --- a/stages/org.osbuild.dracut +++ b/stages/org.osbuild.dracut @@ -129,7 +129,7 @@ def yesno(name: str, value: bool) -> str: return f"--{prefix}{name}" -#pylint: disable=too-many-branches +# pylint: disable=too-many-branches def main(tree, options): kernels = options["kernel"] compress = options.get("compress") diff --git a/stages/org.osbuild.first-boot b/stages/org.osbuild.first-boot index 4b02b626..bd6708f5 100755 --- a/stages/org.osbuild.first-boot +++ b/stages/org.osbuild.first-boot @@ -50,7 +50,7 @@ After=network-online.target""" execs = "\n" for command in commands: - execs += f"ExecStart={command}\n" + execs += f"ExecStart={command}\n" # pylint: disable=consider-using-join service = f"""[Unit] Description=OSBuild First Boot Service diff --git a/stages/org.osbuild.grub2 b/stages/org.osbuild.grub2 index 91bea21f..93ccbd35 100755 --- a/stages/org.osbuild.grub2 +++ b/stages/org.osbuild.grub2 @@ -484,7 +484,7 @@ class GrubConfig: return data -#pylint: disable=too-many-statements,too-many-branches +# pylint: disable=too-many-statements,too-many-branches def main(tree, options): root_fs = options.get("rootfs") boot_fs = options.get("bootfs") diff --git a/stages/org.osbuild.oci-archive b/stages/org.osbuild.oci-archive index 8704e50e..64845dee 100755 --- a/stages/org.osbuild.oci-archive +++ b/stages/org.osbuild.oci-archive @@ -309,7 +309,7 @@ def create_oci_dir(inputs, output_dir, options, create_time): blobs = os.path.join(output_dir, "blobs", "sha256") os.makedirs(blobs) - ## layers / rootfs + # layers / rootfs for ip in sorted(inputs.keys()): tree = inputs[ip]["path"] digest, info = blobs_add_layer(blobs, tree) diff --git a/stages/org.osbuild.ovf b/stages/org.osbuild.ovf index 31515e2c..0aadbc33 100755 --- a/stages/org.osbuild.ovf +++ b/stages/org.osbuild.ovf @@ -36,7 +36,7 @@ SCHEMA_2 = """ OVF_TEMPLATE = """ - + Virtual disk information @@ -57,7 +57,7 @@ OVF_TEMPLATE = """ Virtual Hardware Family 0 image - vmx-15 + vmx-15 hertz * 10^6 @@ -88,7 +88,7 @@ OVF_TEMPLATE = """ SCSI Controller SCSI Controller 0 3 - VirtualSCSI + VirtualSCSI 6 diff --git a/test/src/test_pylint.py b/test/src/test_pylint.py deleted file mode 100644 index 1a5aac5f..00000000 --- a/test/src/test_pylint.py +++ /dev/null @@ -1,110 +0,0 @@ -# -# Run `pylint` on all python sources. -# - -import os -import subprocess - -import pytest - -from .. import test - - -@pytest.fixture(name="source_files") -def discover_source_files(): - - # Evaluate whether `path` is a python file. This assumes that the file is - # opened by the caller, anyway, hence it might invoke `file` to detect the - # file type if in doubt. - def is_python_script(path): - if path.endswith(".py"): - return True - - mime_valid = [ - "text/x-python", - "text/x-script.python", - ] - - mime_file = subprocess.check_output( - [ - "file", - "-bi", - "--", - path, - ], - encoding="utf-8", - ) - - return any(mime_file.startswith(v) for v in mime_valid) - - # Enumerate all repository files. This will invoke `git ls-tree` to list - # all files. This also requires a temporary change of directory, since git - # operates on the current directory and does not allow path arguments. - def list_files(path): - cwd = os.getcwd() - os.chdir(path) - files = subprocess.check_output( - [ - "git", - "ls-tree", - "-rz", - "--full-tree", - "--name-only", - "HEAD", - ], - encoding="utf-8", - ) - os.chdir(cwd) - - files = files.split('\x00') - return (os.path.join(path, f) for f in files) - - if not test.TestBase.have_test_checkout(): - pytest.skip("no test-checkout access") - - files = list_files(test.TestBase.locate_test_checkout()) - files = filter(is_python_script, files) - return list(files) - - -def test_pylint(source_files): - # - # Run `pylint` on all python sources. We simply use `find` to locate - # all `*.py` files, and then manually select the reverse-domain named - # modules we have. - # - - r = subprocess.run(["pylint"] + source_files, check=False) - if r.returncode != 0: - pytest.fail("pylint issues detected") - - -@pytest.mark.skipif(not test.TestBase.have_mypy(), reason="mypy not available") -def test_mypy(): - # - # Run `mypy` on osbuild sources. - # - - r = subprocess.run(["mypy", "osbuild/"], check=False) - if r.returncode != 0: - pytest.fail("mypy issues detected") - - -@pytest.mark.skipif(not test.TestBase.have_isort(), reason="isort not available") -def test_isort(source_files): - # - # Run `isort` on all python sources. We simply use `find` to locate - # all `*.py` files, and then manually select the reverse-domain named - # modules we have. - # - - r = subprocess.run(["isort", "--check", "--diff"] + source_files, check=False) - if r.returncode != 0: - pytest.fail("isort issues detected") - - -@pytest.mark.skipif(not test.TestBase.have_autopep8(), reason="autopep8 not available") -def test_autopep8(source_files): - r = subprocess.run(["autopep8-3", "--diff", "--exit-code"] + source_files, check=False) - if r.returncode != 0: - pytest.fail("autopep8 has detected changes (see diff)") diff --git a/tox.ini b/tox.ini new file mode 100644 index 00000000..54d23a19 --- /dev/null +++ b/tox.ini @@ -0,0 +1,67 @@ +[tox] +env_list = + py{36,37,38,39,310,311} + lint + type + +labels = + test = py{36,37,38,39,310,311} + lint = ruff, isort, autopep8, pylint + type = mypy + +[testenv] +description = "run osbuild unit tests" +deps = + pytest + jsonschema + mako + iniparse + pyyaml + +setenv = + LINTABLES = osbuild/ assemblers/* devices/* inputs/* mounts/* runners/* sources/* stages/* + TYPEABLES = osbuild + +passenv = + TEST_CATEGORY + +commands = + bash -c 'python -m pytest --pyargs --rootdir=. {env:TEST_CATEGORY}' + +allowlist_externals = + bash + +[testenv:ruff] +deps = + ruff==0.0.263 + +commands = + bash -c 'python -m ruff {env:LINTABLES}' + +[testenv:isort] +deps = + isort==5.12.0 + +commands = + bash -c 'python -m isort --check --diff {env:LINTABLES}' + +[testenv:autopep8] +deps = + autopep8==2.0.2 + +commands = + bash -c 'python -m autopep8 -r --diff --exit-code {env:LINTABLES}' + +[testenv:pylint] +deps = + pylint==2.17.3 + +commands = + bash -c 'python -m pylint {env:LINTABLES}' + +[testenv:mypy] +deps = + mypy==1.2.0 + +commands = + bash -c 'python -m mypy {env:TYPEABLES}'