From 1b1d56286eb0112fcd1910d5e79f459a6dd83fc0 Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Fri, 5 Jan 2024 12:00:00 +0100 Subject: [PATCH] Add Python3 pre-commit config checks with pylint and mypy Also enables spell checking when pre-commit runs pylint. - Add pyproject.toml with config for mypy, black and isort - Add .pre-commit-config.yaml for https://github.com/pre-commit - Add GitHub action with cache to .github/workflows/build.yml - Fix execute permissions on files found by pre-commit checks. - build/mkinfo.py: Add module docstring as requested by pylint. - tests/python/conftest.py: Add a pytest fixture for unit-tests - tests/python/test_parse_test_instance_string.py: Add an initial unit test for xtf-runner using the pytest fixture. - xtf-runner: - Import json always as Python2.7 is the minium version - Add comments to locally disable checks where needed. - Resolve a line-too-long while resolving a no-else-return Signed-off-by: Bernhard Kaindl --- .github/workflows/build.yml | 40 +++++++ .pre-commit-config.yaml | 108 ++++++++++++++++++ .pylintrc | 40 ++++++- .pylintrc.project-dict.txt | 49 ++++++++ build/mkcfg.py | 2 +- build/mkinfo.py | 35 +++++- include/xen/sysctl.h | 0 tests/python/conftest.py | 55 +++++++++ .../python/test_parse_test_instance_string.py | 35 ++++++ xtf-runner | 21 ++-- 10 files changed, 368 insertions(+), 17 deletions(-) create mode 100644 .pre-commit-config.yaml create mode 100644 .pylintrc.project-dict.txt mode change 100644 => 100755 build/mkcfg.py mode change 100644 => 100755 build/mkinfo.py mode change 100755 => 100644 include/xen/sysctl.h create mode 100644 tests/python/conftest.py create mode 100644 tests/python/test_parse_test_instance_string.py diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fac842d5..025f89f8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -3,6 +3,46 @@ name: build on: [push, pull_request] jobs: + python-tests: + name: "Python Tests" + runs-on: ubuntu-20.04 + steps: + - name: Checkout code + uses: actions/checkout@v3 + with: + # To have access to secrets, use the PR branch: + # https://github.com/orgs/community/discussions/26409 + ref: ${{github.event.pull_request.head.ref}} + repository: ${{github.event.pull_request.head.repo.full_name}} + + - name: pre-commit checks - setup cache + uses: actions/cache@v3 + with: + path: ~/.cache/pre-commit + key: pre-commit|${{ env.pythonLocation }}|${{ hashFiles('.pre-commit-config.yaml') }} + + - name: pre-commit checks - run checks + uses: pre-commit/action@v3.0.0 + env: + # For merging PRs to master, skip the no-commit-to-branch check: + SKIP: no-commit-to-branch + + - name: Install Python2 dependencies + run: | + #: Install Python 2.7 from Ubuntu 20.04 using apt-get install + sudo apt-get update && sudo apt-get install -y python2 + curl -sSL https://bootstrap.pypa.io/pip/2.7/get-pip.py -o get-pip.py + python2 get-pip.py + if [ -f requirements.txt ]; then pip2 install -r requirements.txt; fi + if [ -f requirements-dev.txt ]; then pip2 install -r requirements-dev.txt; fi + pip2 install pytest pylint==1.9.4 + + - name: Run pylint-1.9.4 + run: python2 -m pylint xtf-runner build/*.py + + - name: Run python2 -m pytest to execute all unit and integration tests + run: python2 -m pytest -v -rA + build: strategy: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 00000000..75aa3cf7 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,108 @@ +# +# Installation and activation of pre-commit: +# +# $ pip3 install pre-commit && pre-commit install +# +# With this, you get git pre-commit hook checks like shown in the terminal +# on this page: https://pre-commit.com/index.html#usage +# +# It is easy to handle as a pre-commit hook once you get the hang of it. +# If not skipped using git commit --no-verify, it makes each commit bisect-able, +# whereas a check only run on a GitHub CI push will normally only run its checks +# on the latest commit of a tree. +# +# However, it does not have to be used as a pre-commit hook: +# Of course it can be used just for GitHub CI only: +# +# pre-commit runs in the GitHub Workflow of this project on each push and PR. +# You can run it locally as well on demand after making changes to check them: +# +# $ pip3 install pre-commit +# $ pre-commit run # run only needed checks +# $ pre-commit run -a # run all fixes and checks +# +# You can enable it as git pre-commit hook for your local clone using: +# $ pre-commit install +# +# Then, the fixups and checks defined below run by default when you commit. +# +# To commit with all git pre-commit hooks skipped (for exceptional cases): +# $ git commit --no-verify +# +# To commit with some checks skipped: +# $ SKIP=mypy,pylint git commit -m "unchecked emergency commit" +# +# Documentation: https://pre-commit.com/#temporarily-disabling-hooks +# All hooks: https://pre-commit.com/hooks.html +# +fail_fast: false +default_stages: [commit, push] +repos: +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.5.0 + # https://github.com/pre-commit/pre-commit-hooks/blob/main/README.md: + hooks: + - id: no-commit-to-branch + name: "ensure that you don't commit to the local master branch" + args: [--branch, master] + always_run: true + - id: trailing-whitespace + name: 'check and fix files to have no trailing whitespace' + exclude: install-sh + - id: end-of-file-fixer + name: 'check and fix that files have a trailing newline' + exclude: | + (?x)^( + arch/x86/include/arch/msr-index.h + ) + - id: mixed-line-ending + args: ['--fix=lf'] + name: 'check and fix that line endings are line feeds' + - id: check-added-large-files + args: ['--maxkb=12'] + name: 'check that no large files are added' + - id: check-executables-have-shebangs + - id: debug-statements + name: 'check for debugger imports and breakpoint calls' + - id: check-shebang-scripts-are-executable + - id: check-merge-conflict + - id: check-yaml + name: 'check the syntax of yaml files' +- repo: local + hooks: + - id: pytest + name: run Python3 unit tests + entry: env PYTHONDEVMODE=yes python3 -m pytest -v + pass_filenames: false + language: python + types: [python] + additional_dependencies: [pytest] +- repo: https://github.com/akaihola/darker + rev: 1.7.2 + hooks: + - id: darker + name: format staged changes like black and isort would format them + args: [--skip-string-normalization, --isort, -tpy36] + additional_dependencies: [isort] +- repo: https://github.com/pre-commit/mirrors-mypy + rev: v1.8.0 + hooks: + - id: mypy + name: run mypy +- repo: https://github.com/pylint-dev/pylint + rev: v3.0.3 + hooks: + - id: pylint + name: run pylint + args: [--jobs=4, --spelling-dict=en_US] # Spell checks: See .pylintrc + files: '(^xtf-runner|\.py)$' + log_file: ".git/pre-commit-pylint.log" + additional_dependencies: ['pylint[spelling]', pytest] +- repo: local + hooks: + - id: git-diff # Ref: https://github.com/pre-commit/pre-commit/issues/1712 + name: Show and fail on not staged changes, also fixups may make them + entry: git diff --exit-code + language: system + pass_filenames: false + always_run: true diff --git a/.pylintrc b/.pylintrc index 7c7e50c4..4e2770df 100644 --- a/.pylintrc +++ b/.pylintrc @@ -37,7 +37,7 @@ extension-pkg-whitelist= # Enable the message, report, category or checker with the given id(s). You can # either give multiple identifier separated by comma (,) or put this option # multiple time. See also the "--disable" option for examples. -#enable= +enable=spelling # Disable the message, report, category or checker with the given id(s). You # can either give multiple identifiers separated by comma (,) or put this @@ -48,7 +48,24 @@ extension-pkg-whitelist= # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use"--disable=all --enable=classes # --disable=W" -disable=bad-whitespace, bad-continuation, global-statement, star-args +disable=bad-whitespace, bad-continuation, global-statement, star-args, + # Allow older pylint-1.9.x for Python2 to tolerate newer pylint options: + bad-option-value, + unrecognized-inline-option, + # Not real problems, returns on the same indentation level can be easier: + consider-using-with, + no-else-raise, + no-else-return, + multiple-imports, + len-as-condition, + # For Python3-only projects: + consider-using-f-string, + deprecated-module, + unrecognized-option, + unspecified-encoding, + use-implicit-booleaness-not-len, + useless-object-inheritance, + useless-option-value, [REPORTS] @@ -103,6 +120,23 @@ ignore-docstrings=yes ignore-imports=no +[SPELLING] + +# Spelling dictionary name. Available dictionaries: en (aspell), en_AU +# (aspell), en_CA (aspell), en_GB (aspell), en_US (aspell). +# To support spelling checks for older python2 pylint versions, +# `sudo apt-get install -y libenchant-2-2` would be needed, +# so we enable it for newer Python3 pylint in .pre-commit-config.yaml: +#spelling-dict=en + +# A path to a file that contains the private dictionary; one word per line. +spelling-private-dict-file=.pylintrc.project-dict.txt + +# Tells whether to store unknown words to the private dictionary (see the +# --spelling-private-dict-file option) instead of raising a message. +spelling-store-unknown-words=y + + [TYPECHECK] # Tells whether missing members accessed in mixin class should be ignored. A @@ -337,4 +371,4 @@ max-public-methods=20 # Exceptions that will emit a warning when being caught. Defaults to # "Exception" -overgeneral-exceptions=Exception +overgeneral-exceptions=builtins.Exception diff --git a/.pylintrc.project-dict.txt b/.pylintrc.project-dict.txt new file mode 100644 index 00000000..0c12b174 --- /dev/null +++ b/.pylintrc.project-dict.txt @@ -0,0 +1,49 @@ +CFG +CWD +ENVS +Normalise +O_CREAT +O_RDONLY +VM +arg +basestring +config +conftest +env +dev +dirs +entrypoint +epilog +hvm +init +invlpg +iopl +json +logfile +logline +logpath +mkcfg +nonexisting +pseduo +pv +py +pylintrc +pyright +pytest +reportMissingImports +reportUndefinedVariable +stdout +subproc +substitue +sys +toolstack +unioned +Unrecognised +xenconsole +xenconsoled +xl +xtf +os +pre +src +dir diff --git a/build/mkcfg.py b/build/mkcfg.py old mode 100644 new mode 100755 index 7f8e3d52..631992be --- a/build/mkcfg.py +++ b/build/mkcfg.py @@ -9,7 +9,7 @@ import sys, os # Usage: mkcfg.py $OUT $DEFAULT-CFG $EXTRA-CFG $VARY-CFG -_, out, defcfg, vcpus, extracfg, varycfg = sys.argv +_, out, defcfg, vcpus, extracfg, varycfg = sys.argv # pylint: disable=unbalanced-tuple-unpacking # Evaluate environment and name from $OUT _, env, name = out.split('.')[0].split('-', 2) diff --git a/build/mkinfo.py b/build/mkinfo.py old mode 100644 new mode 100755 index 50819e2c..8df3970d --- a/build/mkinfo.py +++ b/build/mkinfo.py @@ -1,10 +1,43 @@ #!/usr/bin/env python # -*- coding: utf-8 -*- +""" +mkcfg.py - Generate a configuration JSON file based on provided parameters. + +Usage: + python mkcfg.py $OUT $NAME $CATEGORY $ENVS $VARIATIONS + +Arguments: + + $OUT: Path to the output file where the generated JSON configuration + will be saved. + $NAME: Name to be assigned in the configuration. + $CATEGORY: Category designation in the configuration. + $ENVS: Optional space-separated list of environments (can be empty). + $VARIATIONS: Optional space-separated list of variations (can be empty). + +Description: + + This script generates a JSON configuration file using provided parameters + and saves it to the specified output file. The generated JSON structure + includes fields for 'name', 'category', 'environments', and 'variations'. + The 'environments' and 'variations' fields can be populated with + space-separated lists if corresponding arguments ($ENVS and $VARIATIONS) + are provided. + +Example: + + python mkcfg.py config.json ExampleConfig Utilities prod dev test + + This example will create a configuration file named 'config.json' with + 'name' as 'ExampleConfig', + 'category' as 'Utilities', and + 'environments' as ['prod', 'dev', 'test']. +""" import sys, json # Usage: mkcfg.py $OUT $NAME $CATEGORY $ENVS $VARIATIONS -_, out, name, cat, envs, variations = sys.argv +_, out, name, cat, envs, variations = sys.argv # pylint: disable=unbalanced-tuple-unpacking template = { "name": name, diff --git a/include/xen/sysctl.h b/include/xen/sysctl.h old mode 100755 new mode 100644 diff --git a/tests/python/conftest.py b/tests/python/conftest.py new file mode 100644 index 00000000..acf42243 --- /dev/null +++ b/tests/python/conftest.py @@ -0,0 +1,55 @@ +"""pytest fixtures for unit-testing functions in the xtf-runner script""" +import os +import sys + +import pytest + + +def import_script_as_module(relative_script_path): + "Import a Python script without the .py extension as a python module" + + script_path = os.path.join(os.path.dirname(__file__), relative_script_path) + module_name = os.path.basename(script_path) + + if sys.version_info.major == 2: + # Use deprecated imp module because it needs also to run with Python27: + # pylint: disable-next=import-outside-toplevel + import imp # pyright: ignore[reportMissingImports] + + return imp.load_source(module_name, script_path) + else: + # For Python 3.11+: Import Python script without the .py extension: + # https://gist.github.com/bernhardkaindl/1aaa04ea925fdc36c40d031491957fd3: + + # pylint: disable-next=import-outside-toplevel + from importlib import ( # pylint: disable=no-name-in-module + machinery, + util, + ) + + loader = machinery.SourceFileLoader(module_name, script_path) + spec = util.spec_from_loader(module_name, loader) + assert spec + assert spec.loader + module = util.module_from_spec(spec) + # It is probably a good idea to add the imported module to sys.modules: + sys.modules[module_name] = module + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="session") +def imported_xtf_runner(): + """Fixture to import a script as a module for unit testing its functions""" + return import_script_as_module("../../xtf-runner") + + +@pytest.fixture(scope="function") +def xtf_runner(imported_xtf_runner): # pylint: disable=redefined-outer-name + """Test fixture for unit tests: initializes module for each test function""" + # Init the imported xtf-runner, so each unit test function gets it pristine: + # May be used to unit-test xtf-runner with other, different test dirs: + imported_xtf_runner._all_test_info = {} # pylint: disable=protected-access + # The GitHub pre-commit action for does not start the checks in the src dir: + # os.chdir(os.path.join(os.path.dirname(__file__), "../..")) + return imported_xtf_runner diff --git a/tests/python/test_parse_test_instance_string.py b/tests/python/test_parse_test_instance_string.py new file mode 100644 index 00000000..855069e7 --- /dev/null +++ b/tests/python/test_parse_test_instance_string.py @@ -0,0 +1,35 @@ +"""Test xtf-runner.parse_test_instance_string() using tests/python/conftest""" +import pytest + + +def test_parse_test_instance_string(xtf_runner): + """ + Test env, name, variation = xtf-runner.parse_test_instance_string(string): + + Valid argument strings conform to: [[test-]$ENV-]$NAME[~$VARIATION] + Returns a tuple with the environment, name, and variation (if present). + + This test uses the pytest fixture xtf_runner from tests/python/conftest.py + to import and initialize the xtf-runner Python script as module under test. + """ + # Test the argument "pv64-example": + # for arg in ("pv64-example", "test-pv64-example"): + # env, name, variation = xtf_runner.parse_test_instance_string(arg) + # assert env == "pv64" + # assert name == "example" + # assert variation is None + print(xtf_runner.os.getcwd()) + + # Test the argument "xsa-444": + env, name, variation = xtf_runner.parse_test_instance_string("xsa-444") + assert env is None + assert name == "xsa-444" + assert variation is None + + # Test that passing a nonexisting variation argument raises RunnerError: + with pytest.raises(xtf_runner.RunnerError): + xtf_runner.parse_test_instance_string("xsa-444~nonexisting_variation") + + # Test that passing a nonexisting test argument raises RunnerError: + with pytest.raises(xtf_runner.RunnerError): + xtf_runner.parse_test_instance_string("test-nonexisting_test-raises") diff --git a/xtf-runner b/xtf-runner index 94ed1764..022a5dcb 100755 --- a/xtf-runner +++ b/xtf-runner @@ -9,19 +9,16 @@ Currently assumes the presence and availability of the `xl` toolstack. from __future__ import print_function, unicode_literals +import json import sys, os, os.path as path from optparse import OptionParser from subprocess import Popen, PIPE, call as subproc_call - -try: - import json -except ImportError: - import simplejson as json +from typing import Dict # pylint: disable=unused-import # Python 2/3 compatibility if sys.version_info >= (3, ): - basestring = str + basestring = str # pylint: disable=invalid-name,redefined-builtin # All results of a test, keep in sync with C code report.h. # Notes: @@ -80,8 +77,7 @@ class TestInstance(object): def __repr__(self): if not self.variation: return "test-{0}-{1}".format(self.env, self.name) - else: - return "test-{0}-{1}~{2}".format(self.env, self.name, self.variation) + return "test-{0}-{1}~{2}".format(self.env, self.name, self.variation) def __hash__(self): return hash(repr(self)) @@ -92,8 +88,9 @@ class TestInstance(object): def __ne__(self, other): return repr(self) != repr(other) - def __cmp__(self, other): - return cmp(repr(self), repr(other)) + # Should be obsolete for Python2.7 as it can use __eq__() already: + def __cmp__(self, other): # pylint: disable-next=undefined-variable + return cmp(repr(self), repr(other)) # pylint: disable=line-too-long #pyright:ignore[reportUndefinedVariable] class TestInfo(object): @@ -235,7 +232,7 @@ def parse_test_instance_string(arg): # Cached test json from disk -_all_test_info = {} +_all_test_info = {} # type: Dict[str, str] def get_all_test_info(): """ Open and collate each info.json """ @@ -667,7 +664,7 @@ def main(): "\n" " Running all the pv-iopl tests:\n" " ./xtf-runner pv-iopl\n" - " \n" + " \n" " Combined test results:\n" " test-pv64-pv-iopl SUCCESS\n" " test-pv32pae-pv-iopl SUCCESS\n"