diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0c163aa7e32..d65727f9ce7 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -7,30 +7,110 @@ on: # run daily, this refreshes the cache - cron: '13 2 * * *' +# Cancel a currently running workflow from the same PR, branch or tag +# when a new workflow is triggered: +# https://stackoverflow.com/questions/66335225/how-to-cancel-previous-runs-in-the-pr-when-you-push-new-commitsupdate-the-curre +concurrency: + cancel-in-progress: true + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + jobs: - python-test: - name: Python tests + python2-test: + name: Python2 tests runs-on: ubuntu-22.04 - strategy: - fail-fast: false - matrix: - docker-image: # See https://hub.docker.com/_/python/tags for images - - python:2.7.18-alpine3.11 - - python:3.11.4-alpine3.18 - container: ${{ matrix.docker-image }} + container: python:2.7.18-alpine3.11 steps: - name: Checkout code uses: actions/checkout@v3 - - name: Install python 2 dependencies - if: ${{ startsWith(matrix.docker-image, 'python:2.7.18') }} - run: pip install enum + - name: Install Python2 dependencies + run: pip install enum mock pytest-coverage pytest-mock - - name: Install dependencies - run: pip install mock pytest + - name: Run Python2 tests + run: > + pytest --cov scripts scripts/ -vv -rA + --junitxml=.git/pytest27.xml + --cov-report term-missing + --cov-report html:.git/coverage27.html + --cov-report xml:.git/coverage27.xml + + - name: Upload coverage reports to Codecov + uses: codecov/codecov-action@v3 + with: + directory: .git + files: coverage27.xml + env_vars: OS,PYTHON + fail_ci_if_error: false + flags: python2.7 + name: coverage27 + verbose: true + + - name: Add Pytest coverage comment (if write permission is available) + if: ${{ ! github.event.pull_request.head.repo.fork }} + uses: MishaKav/pytest-coverage-comment@main + continue-on-error: true + with: + junitxml-path: .git/pytest.xml + pytest-xml-coverage-path: .git/coverage27.xml + unique-id-for-comment: pre-commit-coverage-python27 + title: > + Python2 coverage comment from + https://github.com/marketplace/actions/pytest-coverage-comment + + python3-test: + name: Python3 tests + runs-on: ubuntu-22.04 + steps: + # https://www.python4data.science/en/latest/productive/git/advanced/hooks/ci.html + - uses: actions/setup-python@v4 + id: python + with: + python-version: '3.10' + cache: 'pip' + + - uses: actions/cache@v3 + name: Setup cache for running pre-commit fast + with: + path: ~/.cache/pre-commit + key: pre-commit|${{ env.pythonLocation }}|${{ hashFiles('.pre-commit-config.yaml') }} + + - run: echo "::add-matcher::.github/workflows/Python-problemMatcher-xen-api.json" + + - uses: pre-commit/action@v3.0.0 + name: Run pre-commit checks + with: + # Show the output of the commands for the problem matcher, see above. + extra_args: --all-files --verbose + env: + PYTHONDEVMODE: yes # Enable Python3 checks. See the comment above. + # Skip the no-commit-to-branch check inside of GitHub CI (for CI on merge to master) + SKIP: no-commit-to-branch + + - name: Upload coverage reports to Codecov + uses: codecov/codecov-action@v3 + with: + directory: .git + env_vars: OS,PYTHON + # Whether the job should fail if Codecov runs into an error during upload. + # Not failing the job in this case is ok because the pre-commit checks + # also contain a diff-cover job which would fail on missing changed lines: + fail_ci_if_error: false + flags: ${{ format('python{0}', steps.python.outputs.python-version ) }} + name: coverage + verbose: true + + - name: Add Pytest coverage comment (if write permission is available) + if: ${{ ! github.event.pull_request.head.repo.fork }} + uses: MishaKav/pytest-coverage-comment@main + continue-on-error: true + with: + junitxml-path: .git/pytest.xml + pytest-xml-coverage-path: .git/coverage.xml + unique-id-for-comment: pre-commit-coverage + title: > + Python3 coverage comment from + https://github.com/marketplace/actions/pytest-coverage-comment - - name: Run python tests - run: pytest scripts ocaml-test: name: Ocaml tests diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 00000000000..e283a4d3764 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,61 @@ +# See https://pre-commit.com for more information +# See https://pre-commit.com/hooks.html for more hooks +# On the initial run, pre-commit downloads its checkers, subsequent runs are fast: +# +# $ pre-commit run # automatically checks which checks are needed. +# $ pre-commit run -a # runs all fixes and checks, even when not needed +# +# When this works, you can enable it as the pre-commit hook for this git clone: +# $ pre-commit install +# $ pre-commit install --hook-type pre-push +repos: +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v3.2.0 + hooks: + - id: no-commit-to-branch + name: "Check against about accidental commits to the master branch" + args: [--branch, master] + always_run: true + - id: trailing-whitespace + files: ^(tests/unit_test|.github) + - id: end-of-file-fixer + files: ^(tests/unit_test|.github) + - id: check-yaml + - id: check-added-large-files + + +- repo: local + hooks: + - id: pytest + name: pytest + entry: env PYTHONDEVMODE=yes python3 -m pytest + --cov scripts scripts + --junitxml=.git/pytest.xml + --cov-report term-missing + --cov-report html:.git/coverage.html + --cov-report xml:.git/coverage.xml + require_serial: true + pass_filenames: false + language: python + types: [python] + additional_dependencies: + - mock + - pytest-coverage + - pytest-mock + + +- repo: https://github.com/pycqa/pylint + rev: v3.0.3 + hooks: + - id: pylint + files: ^scripts/unit_tests/ + log_file: ".git/pre-commit-pylint.log" + additional_dependencies: [mock, pytest] + + +- repo: https://github.com/pre-commit/mirrors-mypy + rev: v1.8.0 + hooks: + - id: mypy + files: ^scripts/unit_tests/ + additional_dependencies: [pytest-mock, types-mock] \ No newline at end of file diff --git a/.pylintrc b/.pylintrc new file mode 100644 index 00000000000..0dbd24cd917 --- /dev/null +++ b/.pylintrc @@ -0,0 +1,58 @@ +[MAIN] + +# Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the +# number of processors available to use, and will cap the count on Windows to +# avoid hangs. +jobs=2 + +# Pickle collected data for later comparisons. +persistent=yes + +# Discover python modules and packages in the file system subtree. +recursive=yes + +# Add paths to the list of the source roots. Supports globbing patterns. The +# source root is an absolute path or a path relative to the current working +# directory used to determine a package namespace for modules located under the +# source root. +source-roots= + ocaml/xapi-storage/python, + scripts, + scripts/examples/python, + +# When enabled, pylint would attempt to guess common misconfiguration and emit +# user-friendly hints instead of false-positive error messages. +suggestion-mode=yes + +# In verbose mode, extra non-checker-related info will be displayed. +verbose=yes + +[MESSAGES CONTROL] + +# Disable the message, report, category or checker with the given id(s). You +# can either give multiple identifiers separated by comma (,) or put this +# option multiple times (only on the command line, not in the configuration +# file where it should appear only once). You can also use "--disable=all" to +# disable everything first and then re-enable specific checks. For example, if +# you want to run only the similarities checker, you can use "--disable=all +# --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-option-value, # old pylint for py2: ignore newer (unknown) pylint options + bad-continuation, # old pylint warns about some modern black formatting + useless-option-value, # new pylint has abaondoned these old options + +[BASIC] + +# Good variable names which should always be accepted, separated by a comma. +good-names=a, + b, + c, + f, + i, + j, + k, + ex, + Run, + _ diff --git a/scripts/unit_tests/conftest.py b/scripts/unit_tests/conftest.py new file mode 100644 index 00000000000..fc9a52ea024 --- /dev/null +++ b/scripts/unit_tests/conftest.py @@ -0,0 +1,66 @@ +"""Pytest conftest module with functions and (in the future) pytest fixtures""" +import logging +import os +import sys + +import mock +import pytest + + +# Add an attribute when running under pytest to make it possible for a module +# to check to know if unittest is used (then skip) or pytest (then run test): + + +def pytest_configure(config): # pylint: disable=unused-argument + """Configure pytest test cases + + This function is called by pytest during its configuration phase. It takes a + single argument 'config', which is an instance of the pytest Config object. + + The purpose of this function is to set a flag in the sys module to indicate + that it is being called from pytest. This flag can be used by other parts + of the code to determine if they are running under pytest or not. + + :param config: The pytest configuration object. + """ + sys._called_from_pytest = True # pylint: disable=protected-access + + +def pytest_unconfigure(config): # pylint: disable=unused-argument + """Perform cleanup or finalization steps after pytest has finished running. + + This function is called by pytest after all tests have been executed and + pytest is about to exit. It can be used to perform any necessary cleanup + or finalization steps. + + :param config: The pytest configuration object. + """ + del sys._called_from_pytest + + +@pytest.fixture(scope="function") +def usb_reset(): + """ + This function is a pytest fixture that sets up the necessary environment + for testing the 'usb_reset' script. + + It mocks the 'xcp' and 'xcp.logger' modules, and adds the directory of the + unit test to the sys.path so that the tested script can be imported. + + The imported 'usb_reset.log' is redirected to the 'logging' module. + + :returns: The imported 'usb_reset' module. + """ + sys.modules["xcp"] = mock.Mock() + sys.modules["xcp.logger"] = mock.Mock() + + # Prepend the directory of the unit test (to import the tested script) to sys.path, + # so we can do an absolute import and absolute patch without adding __init__.py: + sys.path.insert(0, os.path.dirname(os.path.dirname(__file__))) + + # Then, import usb_reset.py from the __file__'s directory (found first there): + import usb_reset as usb_reset_module # pylint: disable=import-outside-toplevel + + # Redirect usb_reset.log (is a mock xcp.logger for import) to logging + usb_reset_module.log = logging + return usb_reset_module diff --git a/scripts/unit_tests/test_usb_reset.py b/scripts/unit_tests/test_usb_reset.py new file mode 100644 index 00000000000..c11d6b3f881 --- /dev/null +++ b/scripts/unit_tests/test_usb_reset.py @@ -0,0 +1,225 @@ +"""Unit test for usb_reset.py, covers only setup_cgroup() and the functions it calls""" +import logging +import os +import shutil +import sys +from io import BytesIO + + +# Module need pytest, python -m unittest will not traverse into this subdir, +# but in case something passes it this directory anyway, send it away: +if hasattr(sys, "_called_from_pytest"): # See conftest.py + import pytest +else: + import unittest + + raise unittest.SkipTest("pytest not installed, skipped") + + +# For mocking open: builtins vs __builtin__ differs between Python2 and Python3: +BUILTIN = "__builtin__" if sys.version_info < (3,) else "builtins" + +if sys.version_info >= (3, 0): + pytest.skip(allow_module_level=True) + + +def assert_error(captured_logs, error_message): + """Assert that the captured logs contain exactly the specified error message. + + :param captured_logs (logging.LogCapture): The captured logs. + :param error_message (str): The expected error message. + :raises AssertionError: If the captured logs differ from the expected message. + """ + assert captured_logs.record_tuples == [ + ( + "root", + logging.ERROR, + error_message, + ), + ] + captured_logs.clear() + + +def assert_valid_populated_cgroup_dir(cg_path): + """Asserts that the cgroup directory is populated with the necessary files. + + The function checks the following: + - The cgroup directory contains "tasks", "devices.allow", and "devices.deny". + - The "tasks" file contains the process ID (PID) of the current process. + - The "devices.allow" file grants read and write access to /dev/null. + - The "devices.deny" file negates the default "a *:* rwm" rule by writing "a". + + :param expected_cg_path (str): The path to the cgroup directory. + :raises AssertionError: When any of the assertions fail. + """ + checks = { + # Assert read and write access to /dev/null in devices.allow + "devices.allow": "c 1:3 rw", + # Assert that default 'a *:* rwm' was negated by writing "a" om devices.deny + "devices.deny": "a", + # Assert the tasks file to have the passed pid: + "tasks": str(os.getpid()), + } + for name, content in checks.items(): + with open(cg_path + "/" + name) as i: # pylint: disable=unspecified-encoding + assert i.read() == content + # Assert that no other files were created: + assert sorted(os.listdir(cg_path)) == sorted(checks.keys()) + + +def test_populate_cgroup_directory(usb_reset, tmp_path, mocker): + """Test creating and populating the cgroup directory for USB pass-through. + + Verify that the necessary files and permissions are set correctly + within the cgroup directory. + + :param usb_reset (object): usb_reset module object as pytest fixture + :param tmp_path (path): The pytest's temporary directory path for testing. + :param mocker: The mocker object for mocking functions (from pytest-mock) + """ + # Prepare the test: Mock usb_reset.get_cg_dir to return a subdir of tmp_path: + tmpdir = tmp_path.as_posix() + mocker.patch("usb_reset.get_cg_dir", return_value=tmpdir + "/qemu-dom_id") + tmp_cgroup_dir = tmpdir + "/qemu-dom_id" + get_ctl = mocker.spy(usb_reset, "get_ctl") + + # Assert that if the qemu-dom_id dir does not exist, it is created with permissions: + usb_reset.setup_cgroup("dom_id", str(os.getpid())) + assert os.stat(tmp_cgroup_dir).st_mode == 0o40755 + assert_valid_populated_cgroup_dir(tmp_cgroup_dir) + + # Assert that usb_reset.get_ctl has been called once with ("/dev/null", "rw") + get_ctl.assert_called_once_with("/dev/null", "rw") + assert get_ctl.spy_return == "c 1:3 rw" + + # Clean the tmp_cgroup_dir for the 2nd part of this test case + shutil.rmtree(tmp_cgroup_dir) + + # Repeat, now check that mkdir(cg_path) causing EEXIST is tolerated properly: + mocker.resetall() + os.mkdir(tmp_cgroup_dir) + usb_reset.setup_cgroup("dom_id", str(os.getpid())) + assert_valid_populated_cgroup_dir(tmp_cgroup_dir) + # Assert that usb_reset.get_ctl has been called once with ("/dev/null", "rw") + get_ctl.assert_called_once_with("/dev/null", "rw") + assert get_ctl.spy_return == "c 1:3 rw" + + +def test_handle_cgroup_directory_creation_error(usb_reset, caplog, mocker): + """Assert usb_reset.setup_cgroup() handling OSError from mkdir of cgroup directory. + + :param usb_reset (object): usb_reset module object as pytest fixture + :param caplog: The pytest caplog fixture capturing log messages during the test + :param mocker: The mocker object for patching functions. + """ + # Define the values we use in the test and expect to see in the test's error: + expected_dom_id = "expected_dom_id" + expected_cgroup_dir = "/sys/fs/cgroup/devices/qemu-" + expected_dom_id + + # Assert that we receive exit(1), caused by os.mkdir() raising OSError: + mocker.patch("os.mkdir", side_effect=OSError) + with pytest.raises(SystemExit) as exc_info: + usb_reset.setup_cgroup(expected_dom_id, "pid is not accessed in this test") + assert exc_info.value.args == (1,) # assert that exit(1) caused the SystemExit + + # Assert an error with the required cgroup path, expected by mkdir raising OSError: + assert_error(caplog, "Failed to create cgroup: " + expected_cgroup_dir) + + +def test_handle_file_open_errors(usb_reset, tmp_path, mocker, caplog): + """Assert handling of OSError while attempting to open error + + :param usb_reset (object): usb_reset module object as pytest fixture + :param tmp_path: The temporary path used for mocking the cgroup directory. + :param mocker: The mocker object used for patching functions. + :param caplog: The pytest fixture object used for capturing log messages. + """ + # Error message we raise and expect to see from mocked open() raising IOError: + open_error = "Error message raised using IOError when opening a cgroup file" + # For mocking open: builtins vs __builtin__ differs between Python2 and Python3: + builtin_open = "__builtin__.open" if sys.version_info < (3,) else "builtins.open" + + # Mock get_cg_dir() to return our temp_path, otherwise we fail before opening files: + mocker.patch("usb_reset.get_cg_dir", return_value=tmp_path.as_posix()) + + # Assert that if open() raises IOError, setup_cgroup() raises SystemExit(1) in turn + for open_side_effects in [ # Parameterize three calls to open() and test each one + (IOError(open_error), BytesIO(), BytesIO()), + (BytesIO(), IOError(open_error), BytesIO()), + (BytesIO(), BytesIO(), IOError(open_error)), + ]: + # Setup open to return IOError for one of the calls and BytesIO() for the others + mocker.patch(builtin_open, side_effect=open_side_effects) + + with pytest.raises(SystemExit) as exc_info: + usb_reset.setup_cgroup("some valid dom_id", "pid not used in this test") + + assert exc_info.value.args == (1,) # assert that exit(1) caused the SystemExit + + # Assert that the expected error with the error message that we raised was logged + assert_error(caplog, "Failed to setup cgroup: " + open_error) + + +def test_write_order(usb_reset, tmp_path, mocker, capfd): + """Test that when writing unbuffered, the writes are + + :param usb_reset (object): usb_reset module object as pytest fixture + :param tmp_path: The temporary path used for mocking the cgroup directory. + :param mocker: The mocker object used for patching functions. + :param capfd: The pytest fixture capturing stdout messages on the file descriptor + """ + + # Mock get_cg_dir() to return our temp_path, otherwise we fail before opening files: + mocker.patch("usb_reset.get_cg_dir", return_value=tmp_path.as_posix()) + + open_side_effects = ( + os.fdopen(os.dup(sys.stdout.fileno()), "wb", 0), + os.fdopen(os.dup(sys.stdout.fileno()), "wb", 0), + os.fdopen(os.dup(sys.stdout.fileno()), "wb", 0), + ) + mocker.patch(BUILTIN + ".open", side_effect=open_side_effects) + usb_reset.setup_cgroup("dom_id is not significant in this this test case", "") + assert capfd.readouterr().out == "ac 1:3 rw" + + +def test_open_unbuffered(usb_reset, tmp_path, mocker): + """Test that when writing unbuffered, the writes are + + :param usb_reset (object): usb_reset module object as pytest fixture + :param tmp_path: The temporary path used for mocking the cgroup directory. + :param mocker: The mocker object used for patching functions. + """ + # Mock get_cg_dir() to return our temp_path, otherwise we fail before opening files: + tmp = tmp_path.as_posix() + mocker.patch("usb_reset.get_cg_dir", return_value=tmp_path.as_posix()) + mock_open_func = mocker.mock_open() + mocker.patch(BUILTIN + ".open", mock_open_func) + usb_reset.setup_cgroup("dom_id is not significant in this this test case", "") + + # Note: The order should not be important, so in case you need to change the + # implementation, change the testcase too. The only important detail is that + # cgroupfs writes should done on unbuffered files, which these checks ensure. + + # Assert that write and open were called with the expected args in expected order + assert mock_open_func().write.call_args_list == [ + mocker.call(b"a"), + mocker.call(b"c 1:3 rw"), + mocker.call(b""), + ] + mock_open_func.assert_has_calls( + [ + mocker.call(tmp + "/tasks", "w", 0), + mocker.call().__enter__(), # pylint: disable=unnecessary-dunder-call + mocker.call(tmp + "/devices.deny", "w", 0), + mocker.call().__enter__(), # pylint: disable=unnecessary-dunder-call + mocker.call(tmp + "/devices.allow", "w", 0), + mocker.call().__enter__(), # pylint: disable=unnecessary-dunder-call + mocker.call().write(b"a"), + mocker.call().write(b"c 1:3 rw"), + mocker.call().write(b""), + mocker.call().__exit__(None, None, None), + mocker.call().__exit__(None, None, None), + mocker.call().__exit__(None, None, None), + mocker.call(), + ] + )