From 0fcba232ce4df26fa1d92b2349781f4e5f5f1f8f Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Thu, 11 Jan 2024 10:42:28 +0100 Subject: [PATCH] CI: Improve unit tests to test the bugtool's main() as well Add tests/unit/test_main.py which also uses test functions which were added for earlier tests to test the main function of bugtool to produce the bugtool output archives for all types: zip, tar, and tar.bz2. The main test checks the output of the bugtool application to ensure that it matches the expected output. It compares the captured output with the expected output and performs various assertions to validate the output. It extracts the output files from the archive and checks that the xAPI db and the inventory.xml. Tested output: - The start and end of the collected bugtool messages from captured.out - The inventory.xml file is checked to be validated using its XML schema. - The xapi-db.xml is checked to have secrets filtered using a dummy xapi db Signed-off-by: Bernhard Kaindl --- .github/workflows/main.yml | 32 ++- .pre-commit-config.yaml | 104 ++++--- .pylintrc | 4 +- requirements-dev.txt | 3 +- tests/integration/sar-file-collection.test.sh | 2 +- tests/unit/conftest.py | 10 +- tests/unit/test_main.py | 259 ++++++++++++++++++ tests/unit/test_process_output.py | 6 +- tests/unit/test_xapidb_filter.py | 23 +- 9 files changed, 384 insertions(+), 59 deletions(-) create mode 100644 tests/unit/test_main.py diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 39315b3a..14d7eb46 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -63,7 +63,37 @@ jobs: - name: Run pylint-1.9.4 for pylint --py3k linting (configured in .pylintrc) run: python2 -m pylint xen-bugtool - name: Run python2 -m pytest to execute all unit and integration tests - run: python2 -m pytest -v -rA + run: > + python2 -m pytest -v -rA + --cov xen-bugtool + --cov tests/ + --junitxml=.git/pytest27.xml + --cov-report term-missing + --cov-report html:.git/coverage27.html + --cov-report xml:.git/coverage27.xml + + - name: Pytest coverage comment for the Python 2.7 tests + if: ${{ github.actor != 'nektos/act' }} + uses: MishaKav/pytest-coverage-comment@main + with: + junitxml-path: .git/pytest27.xml + pytest-xml-coverage-path: .git/coverage27.xml + unique-id-for-comment: pytest-coverage-python27 + title: https://github.com/marketplace/actions/pytest-coverage-comment + + - name: Upload coverage reports to Codecov for the Python 2.7 tests + if: ${{ github.actor != 'nektos/act' }} + uses: codecov/codecov-action@v3 + env: + PYTHON: python2.7 + with: + directory: .git + files: coverage27.xml + flags: unittest, python2.7 + env_vars: OS,PYTHON + fail_ci_if_error: true + name: coverage27 + verbose: true pre-commit: name: "Python3: Pre-Commit Suite" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 22314396..d4897e85 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -44,7 +44,7 @@ repos: name: 'check and fix files to have no trailing whitespace' - id: end-of-file-fixer name: 'check and fix that files have a trailing newline' - exclude: tests/integration/sar-file-collection.test.sh + exclude: tests/integration/dom0-template - id: mixed-line-ending args: ['--fix=lf'] name: 'check and fix that line endings are line feeds' @@ -58,45 +58,10 @@ repos: - id: check-merge-conflict - id: check-yaml name: 'check the syntax of yaml files' -# Run "python3 -m pip install -r requirements-dev.txt" to run pytest or use "git commit --no-verify": -- repo: local - hooks: - - id: pytest - name: check that the Xen-Bugtool Test Environment passes - entry: env PYTHONDEVMODE=yes python3 -m pytest tests/unit - --cov xen-bugtool - --cov tests/unit - --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: [coverage, defusedxml, pytest-coverage, lxml, XenAPI] - - - - id: diff-cover - name: check that that the changed lines are covered by tests - entry: diff-cover --ignore-whitespace --compare-branch=origin/master - --show-uncovered --html-report .git/coverage-diff.html - --fail-under 100 .git/coverage.xml - require_serial: true - pass_filenames: false - language: python - types: [python] - additional_dependencies: [diff-cover] +# The fast checks are first, to fix them first, +# pytest with code coverage at the end. - - id: coverage-report - name: check coverage report for minimum overall coverage - entry: coverage report --fail-under 55 #| tee .git/coverage.txt - require_serial: true - pass_filenames: false - language: python - types: [python] - additional_dependencies: [coverage] - repo: https://github.com/PyCQA/autoflake rev: v2.2.1 hooks: @@ -105,6 +70,8 @@ repos: args: ["--in-place", "--remove-unused-variables", "--remove-all-unused-imports"] language: python files: \.py$ + + - repo: https://github.com/psf/black rev: 23.11.0 hooks: @@ -112,6 +79,8 @@ repos: name: Ensure that all files (excluding xen-bugtool itself) are black-formatted args: [--safe, --quiet] exclude: xen-bugtool + + - repo: https://github.com/akaihola/darker rev: 1.7.2 hooks: @@ -123,20 +92,73 @@ repos: args: [darker-xen-bugtool] additional_dependencies: - isort + + - repo: https://github.com/pre-commit/mirrors-mypy - rev: v1.7.1 + rev: v1.8.0 hooks: - id: mypy name: Run mypy to check e.g. that all expected arguments are passed to functions etc additional_dependencies: [defusedxml, pytest, types-lxml] + + - repo: https://github.com/pycqa/pylint - rev: v3.0.2 + rev: v3.0.3 hooks: - id: pylint name: Run pylint to check that docstrings are added (and all other enabled checks) log_file: ".git/pre-commit-pylint.log" + + +# Run "python3 -m pip install -r requirements-dev.txt" to run pytest or use "git commit --no-verify": +- repo: local + hooks: + - id: pytest + name: check that the Xen-Bugtool Test Environment passes + entry: env PYTHONDEVMODE=yes python3 -m pytest tests/unit + --cov xen-bugtool + --cov tests/unit + --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: + - coverage + - defusedxml + - pytest-coverage + - pytest-mock + - lxml + - XenAPI + + + - id: diff-cover + name: check that that the changed lines are covered by tests + entry: diff-cover --ignore-whitespace --compare-branch=origin/master + --show-uncovered --html-report .git/coverage-diff.html + --fail-under 100 .git/coverage.xml + require_serial: true + pass_filenames: false + language: python + types: [python] + additional_dependencies: [diff-cover] + + + - id: coverage-report + name: check coverage report for minimum overall coverage + entry: coverage report --fail-under 55 #| tee .git/coverage.txt + require_serial: true + pass_filenames: false + language: python + types: [python] + additional_dependencies: [coverage] + + - repo: https://github.com/RobertCraigie/pyright-python - rev: v1.1.339 + rev: v1.1.345 hooks: - id: pyright name: Run pyright to check the unit tests for any typing warnings (use for bugtool later) diff --git a/.pylintrc b/.pylintrc index a9571565..5421e848 100644 --- a/.pylintrc +++ b/.pylintrc @@ -64,7 +64,8 @@ py-version=2.7 [BASIC] # Good variable names which should always be accepted, separated by a comma. -good-names=i, +good-names=b, + i, fd, j, k, @@ -161,6 +162,7 @@ disable=anomalous-backslash-in-string, invalid-name, invalid-name, len-as-condition, + missing-type-doc, # better do typing than type in the param docstring no-else-break, no-else-return, no-name-in-module, diff --git a/requirements-dev.txt b/requirements-dev.txt index cb14c82a..23821c98 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,2 +1,3 @@ lxml -pytest +pytest-coverage +pytest-mock diff --git a/tests/integration/sar-file-collection.test.sh b/tests/integration/sar-file-collection.test.sh index f57a23ee..17999ff9 100755 --- a/tests/integration/sar-file-collection.test.sh +++ b/tests/integration/sar-file-collection.test.sh @@ -74,4 +74,4 @@ popd # Check that the tar and zip outputs are identical (except for date and uptime): sed -i "s/\\<$tar_basename\\>/$zip_basename/" tar/inventory.xml sed -i 's/date="[^"]*"//;s/uptime="[^"]*"//' {tar,zip}/inventory.xml -diff -rNu tar zip \ No newline at end of file +diff -rNu tar zip diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 032a68f3..dc0a6b14 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1,5 +1,6 @@ """tests/unit/conftest.py: pytest fixtures for unit-testing functions in the xen-bugtool script""" import os +import sys import pytest @@ -14,18 +15,15 @@ def testdir(): def dom0_template(testdir): """Test fixture to get the directory of the dom0 template and adding it's /usr/sbin to the PATH""" dom0_root_dir = testdir + "/../integration/dom0-template" - os.environ["PATH"] = dom0_root_dir + "/usr/sbin" # for modinfo, mdadm, etc return dom0_root_dir @pytest.fixture(scope="session") -def imported_bugtool(testdir): +def imported_bugtool(testdir, dom0_template): """Test fixture to import the xen-bugtool script as a module for executing unit tests on functions""" # This uses the deprecated imp module because it needs also to run with Python2.7 for now: def import_from_file(module_name, file_path): - import sys - if sys.version_info.major == 2: # pragma: no cover import imp # pylint: disable=deprecated-module # pyright: ignore[reportMissingImports] @@ -47,6 +45,9 @@ def import_from_file(module_name, file_path): bugtool = import_from_file("xen-bugtool", testdir + "/../../xen-bugtool") bugtool.ProcOutput.debug = True + # Prepend tests/mocks to force the use of our mock xen.lowlevel.xc module: + sys.path.insert(0, testdir + "/../mocks") + os.environ["PATH"] = dom0_template + "/usr/sbin" # for modinfo, mdadm, etc return bugtool @@ -55,4 +56,5 @@ def bugtool(imported_bugtool): """Test fixture for unit tests, initializes the bugtool data dict for each test""" # Init import_bugtool.data, so each unit test function gets it pristine: imported_bugtool.data = {} + sys.argv = ["xen-bugtool", "--unlimited"] return imported_bugtool diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py new file mode 100644 index 00000000..f523bc5f --- /dev/null +++ b/tests/unit/test_main.py @@ -0,0 +1,259 @@ +"""pytest module for unit-testing xen-bugtool's main() function""" +import logging +import os +import sys +import tarfile +import zipfile + +import pytest + +from . import test_xapidb_filter +from .test_output import assert_valid_inventory_schema, parse + + +yes_to_all_warning = """\ +Warning: '--yestoall' argument provided, will not prompt for individual files. +""" +bugtool_banner = """ +This application will collate the Xen dmesg output, details of the +hardware configuration of your machine, information about the build of +Xen that you are using, plus, if you allow it, various logs. + +The collated information will be saved as a .zip for archiving or +sending to a Technical Support Representative. + +The logs may contain private information, and if you are at all +worried about that, you should exit now, or you should explicitly +exclude those logs from the archive. + + +""" + + +def test_no_root_privileges(bugtool, caplog, mocker): + """Test a bugtool module to execute main() and fail for missing root perms + + This minimal test serves as a teaching tool to how these are done: + - patching an imported bugtool module provided by the bugtool pytest fixture + - capturing and checking of log messages using the caplog fixture + + :param bugtool: The imported bugtool module object to be patched. + :param caplog: The caplog pytest fixture for capturing log messages. + :param mocker: The mocker pytest fixture for mocking functions and attributes. + """ + + mocker.patch("os.getuid", return_value=1000) + with caplog.at_level(logging.FATAL): + assert bugtool.main() == 1 + logger, level, message = caplog.record_tuples[0] + assert logger == "root" + assert level == logging.FATAL + assert message == "Error: xen-bugtool must be run as root" + + +def patch_bugtool(bugtool, mocker, dom0_template, report_name, tmp_path): + """Patch a bugtool module with mocks and configurations to execute main() + + :param bugtool: The imported bugtool module object to be patched. + :param mocker: The mocker object for mocking functions and attributes. + :param dom0_template: The path to the dom0 template directory. + :param report_name: The basename of the bug-report file and it's subdir. + :param tmp_path: The temporary path for storing files. + """ + + # Mock os.getuid to simulate the root user: + mocker.patch("os.getuid", return_value=0) + + # Turn the timestamp in the logs into a fixed string: + mocker.patch("time.strftime", return_value="time.strftime") + + # Set the name of the bug-report file and the subdirectory in it: + os.environ["XENRT_BUGTOOL_BASENAME"] = report_name + + # Mock the location of the several path to be in our dom0 template dir: + bugtool.VAR_LOG_DIR = dom0_template + "/var" + bugtool.XENSOURCE_INVENTORY = dom0_template + "/etc/xensource-inventory" + bugtool.SYSTEMD_CONF_DIR = dom0_template + "/etc/systemd" + bugtool.BIN_STATIC_VDIS = dom0_template + "/opt/xensource/bin/static-vdis" + + # These do not have to be exact mocks, they just have to return success: + bugtool.HA_QUERY_LIVESET = dom0_template + "/usr/sbin/mdadm" + bugtool.RPM = dom0_template + "/usr/sbin/multipathd" + tmp = tmp_path.as_posix() + + # Write a simple xAPI DB for bugtool to filter and check its output later + xensource_db_conf = tmp + "/db.conf" + xen_api_state_db = tmp + "/xen_api_state.db" + with open(xen_api_state_db, "w") as db_state: + db_state.write(test_xapidb_filter.original) + with open(xensource_db_conf, "w") as db_conf: + db_conf.write("[" + xen_api_state_db + "]") + + bugtool.DB_CONF = xensource_db_conf + + # Mock input to always answer with "n": + # mocker.patch("xen-bugtool.input", return_value="n") + mocker.patch("xen-bugtool.dump_xapi_rrds", return_value="mocked") + + def mocked_func(_): + return "mocked" + + bugtool.dump_xapi_rrds = mocked_func + bugtool.csl_logs = mocked_func + + bugtool.BUG_DIR = tmp + + +def check_output(bugtool, captured, tmp_path, filename, filetype): + """Check the stdout, db and inventory output of a bugtool's main() function + + This function checks the output of the bugtool application to ensure that + it matches the expected output. It compares the captured output with the + expected output and performs various assertions to validate the output. + + It extracts the output files from the archive and checks that the xAPI db + and the inventory.xml. + + Tested output: + - The start and end of the collected bugtool messages from captured.out + - The inventory.xml file is checked to be validated using its XML schema. + - The xapi-db.xml is checked to have secrets filtered using a dummy xapi db + + :param bugtool: The patched bugtool module object to be executed + :param captured: The captured stdout output from running the bugtool main() + :param tmp_path: The temporary path where the output files are stored. + :param filename: The name of the bug-report output archive. + :param filetype: The type of the output file (zip, tar, or tar.bz2) + + :raises AssertionError: When the output does not match the expected output. + """ + + out_begin = yes_to_all_warning + out_begin += bugtool_banner.replace("zip", filetype) + p = "[time.strftime] " + + if bugtool.ProcOutput.debug: + out_begin += p + "Starting 'mdadm --detail --scan'\n" + + fd = "--outfd=" in " ".join(sys.argv) + if not fd: + out_begin += p + "Creating output file\n" + + out_begin += p + "Running commands to collect data\n" + assert captured.out.startswith(out_begin) + + if bugtool.ProcOutput.debug: + assert p + "Starting '%s list'\n" % bugtool.BIN_STATIC_VDIS in captured.out + for ls in ("/opt/xensource", "/etc/xensource/static-vdis"): + assert p + "Starting 'ls -lR %s'\n" % ls in captured.out + + if not fd: + archive = "tarball" if filetype.startswith("tar") else "archive" + success = "Writing %s %s successful.\n" % (archive, filename) + assert success in captured.out + + # Extract the created output file into the tmp_path + src_dir = os.getcwd() + os.chdir(tmp_path) + if filetype == "zip": + zipfile.ZipFile(filename).extractall() + elif filetype == "tar": + tarfile.TarFile.open(filename).extractall() + elif filetype == "tar.bz2": + tarfile.TarFile.open(filename, "r:bz2").extractall() + + output_directory = filename.replace(".%s" % filetype, "/") + + # Assert that the captured dummy Xen-API database to filtered as expected: + with open(output_directory + "/xapi-db.xml") as xen_api_db: + data = xen_api_db.read() + test_xapidb_filter.assert_xml_str_equiv(data, test_xapidb_filter.expected) + + os.chdir(src_dir) + # Assertion check of the output files is missing an inventory entry: + # Do this check in a future test which runs + assert_valid_inventory_schema(parse(output_directory + "inventory.xml")) + + +def assert_valid_inventory(bugtool, args, capsys, tmp_path, base_path, filetype): + """Run the bugtool module object's main() function and check its output + + This function runs the patched bugtool module object and asserts that its + output matches the expected output. + + :param bugtool: The patched bugtool module object to be executed + :param args: Additional arguments to set for running the bugtool's main() + :param capsys: capsys pytest fixture to validate the bugtool stdout + :param tmp_path: The temporary path where the output files are stored. + :param base_path: The base directory and base name of the output file. + :param filetype: The type of the output file (zip, tar, or tar.bz2) + + :raises AssertionError: When the output does not match the expected output. + """ + sys.argv.extend(args) + sys.argv.append("-y") + + assert bugtool.main() == 0 + filename = base_path + "." + filetype + check_output(bugtool, capsys.readouterr(), tmp_path, filename, filetype) + + +@pytest.fixture(scope="function") +def patched_bugtool(bugtool, mocker, dom0_template, tmp_path): + """PyTest fixture providing a patched bugtool module with mocks and + configurations to execute its main() from a unprivileged unit test process + on a regular Linux host, with test data from the framework's dom0 template. + + :param bugtool: The imported bugtool module object to be patched. + :param mocker: The mocker object for mocking functions and attributes. + :param dom0_template: The path to the dom0 template directory. + :param tmp_path: The temporary path for storing files. + """ + patch_bugtool(bugtool, mocker, dom0_template, "main_mocked", tmp_path) + + base_path = tmp_path / os.environ["XENRT_BUGTOOL_BASENAME"] + return bugtool, base_path.as_posix(), tmp_path.as_posix() + + +def test_main_func_output_to_zipfile(patched_bugtool, capsys): + """Assert creating a zipfile creates a zipfile with a valid XML inventory + + :param patched_bugtool: The patched bugtool module object to be tested + :param capsys: The capsys pytest fixture for capturing sys.stdout + """ + + bugtool, base_path, tmp_path = patched_bugtool + filetype = "zip" + args = ["--output=" + filetype] + assert_valid_inventory(bugtool, args, capsys, tmp_path, base_path, filetype) + + +def test_main_func_output_to_tarball(patched_bugtool, capsys): + """Assert writing a tar fd creates a tarball with a valid XML inventory + + :param patched_bugtool: The patched bugtool module object to be tested + :param capsys: The capsys pytest fixture for capturing sys.stdout + """ + + bugtool, base_path, tmp_path = patched_bugtool + filetype = "tar" + filename = base_path + "." + filetype + # Test writing the tarbal to an opened file descriptor: + fd = os.open(filename, os.O_CREAT | os.O_WRONLY) + args = ["--outfd=%s" % fd, "--output=tar"] + bugtool.ProcOutput.debug = False + assert_valid_inventory(bugtool, args, capsys, tmp_path, base_path, filetype) + + +def test_main_func_output_to_tarball_bzip2(patched_bugtool, capsys): + """Assert creating a tar.bz2 creates a tarball with a valid XML inventory + + :param patched_bugtool: The patched bugtool module object to be tested + :param capsys: The capsys pytest fixture for capturing sys.stdout + """ + + bugtool, base_path, tmp_path = patched_bugtool + filetype = "tar.bz2" + args = ["--output=" + filetype] + bugtool.ProcOutput.debug = False + assert_valid_inventory(bugtool, args, capsys, tmp_path, base_path, filetype) diff --git a/tests/unit/test_process_output.py b/tests/unit/test_process_output.py index eaf1ba5b..ada34e39 100644 --- a/tests/unit/test_process_output.py +++ b/tests/unit/test_process_output.py @@ -3,6 +3,7 @@ def test_mdadm_arrays(bugtool, dom0_template): """Assert mdadm_arrays() returning arrays of the mdadm mockup in the dom0-template""" + bugtool.MDADM = dom0_template + "/usr/sbin/mdadm" assert list(bugtool.mdadm_arrays()) == ["/dev/md0", "/dev/md1"] @@ -10,10 +11,13 @@ def test_module_info(bugtool, dom0_template): """Assert module_info() returning module names from mockup file in the dom0-template""" bugtool.PROC_MODULES = __file__.replace(".py", ".modules") + bugtool.MODINFO = dom0_template + "/usr/sbin/modinfo" output = bugtool.module_info(bugtool.CAP_KERNEL_INFO) assert output == "modinfo for tcp_diag\nmodinfo for udp_diag\nmodinfo for inet_diag\n" def test_multipathd_topology(bugtool, dom0_template): """Assert multipathd_topology() returning the output of the faked multipathd tool""" - assert bugtool.multipathd_topology(bugtool.CAP_MULTIPATH) == bugtool.MULTIPATHD + "-k" + + bugtool.MULTIPATHD = dom0_template + "/usr/sbin/multipathd" + assert bugtool.multipathd_topology(bugtool.CAP_MULTIPATH) == "multipathd-k" diff --git a/tests/unit/test_xapidb_filter.py b/tests/unit/test_xapidb_filter.py index 15ca2a37..8c8f4298 100644 --- a/tests/unit/test_xapidb_filter.py +++ b/tests/unit/test_xapidb_filter.py @@ -1,8 +1,8 @@ """tests/unit/test_xapidb_filter.py: Ensure that the xen-bugtool.DBFilter() filters the XAPI DB properly""" import os import sys -import xml.dom.minidom -import xml.etree.ElementTree as ET +from xml.dom.minidom import parseString +from xml.etree.ElementTree import fromstring testdir = os.path.dirname(__file__) @@ -70,15 +70,20 @@ def assert_xml_element_trees_equiv(a, b): assert_xml_element_trees_equiv(achild, bchild) -def test_xapi_database_filter(bugtool): - """Assert that bugtool.DBFilter().output() filters the xAPI database as expected""" - - filtered = bugtool.DBFilter(original).output() +def assert_xml_str_equiv(filtered, expected): + """Assert that the given dummy Xen-API database to filtered as expected""" - # Works for Python2 equally, so we can use it to check against Python2/3 regressions: - assert_xml_element_trees_equiv(ET.fromstring(filtered), ET.fromstring(expected)) + # Works for Python2 equally, so we can use it to check Python2/3 to work.: + assert_xml_element_trees_equiv(fromstring(filtered), fromstring(expected)) # Double-check with parseString(): Its output will differ between Py2/Py3 # though, so we will use it for one language version at a time: if sys.version_info < (3, 0): # pragma: no cover - assert xml.dom.minidom.parseString(filtered).toprettyxml(indent=" ") == expected + assert parseString(filtered).toprettyxml(indent=" ") == expected + + +def test_xapi_database_filter(bugtool): + """Assert bugtool.DBFilter().output() filters xAPI database like expected""" + + filtered = bugtool.DBFilter(original).output() + assert_xml_str_equiv(filtered, expected)