From 15ab34dfb3531c3e723491471edfa3b4180b3809 Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Fri, 29 Mar 2024 00:33:00 +0100 Subject: [PATCH] CP-48198: close exited stdouts, fix warnings, use runpy, update coveragerc (#98) - Fix a number of unclosed file warnings in tests: close stdout of exited procs - Use runpy.run_path() to execute xen-bugtool as a script: - tests/integration/test_*.py now also collect coverage of xen-bugtool - Update .coveragerc: Run "coverage run && coverage report" to get coverage! - Fix warnings from SonarLint, Codacy (e.g bandit), Sourcery - Update configuration to be compatible with SonarCloud and upload to it - Fix pyproject.toml to be compatible with "ruff check xen-bugtool" Signed-off-by: Bernhard Kaindl --- .coveragerc | 20 +++++++++++++++--- .github/workflows/main.yml | 24 ++++++++++++++++++++++ .pre-commit-config.yaml | 9 ++------ .vscode/ltex.dictionary.en-US.txt | 11 ++++++++++ README.md | 4 ++++ pyproject.toml | 2 +- pytest.ini | 3 +++ sonar-project.properties | 15 ++++++++++++++ tests/integration/test_system_load.py | 2 +- tests/integration/test_xenserver_config.py | 14 ++++++++----- tests/integration/utils.py | 4 ++-- tests/unit/conftest.py | 4 ++-- tests/unit/test_main.py | 10 ++++++--- tests/unit/test_output.py | 17 ++++++++------- xen-bugtool | 9 ++++---- 15 files changed, 113 insertions(+), 35 deletions(-) create mode 100644 sonar-project.properties diff --git a/.coveragerc b/.coveragerc index 640b241b..89fa8df2 100644 --- a/.coveragerc +++ b/.coveragerc @@ -1,7 +1,20 @@ [run] -source = - xen-bugtool - tests/ +command_line = -m pytest --junitxml=.git/pytest.xml --cov-report=term-missing tests/unit +# Default data file for "coverage run": Store coverage data in .git/.coverage +data_file = .git/.coverage +relative_files = True +omit = + tests/integration/__init__.py + tests/mocks/xen/__init__.py + tests/mocks/xen/lowlevel/__init__.py + tests/mocks/xen/lowlevel/xc/__init__.py + tests/unit/__init__.py + +[coverage:html] +directory = .git/coverage.html + +[coverage:xml] +output = .git/coverage.xml [report] # Regular expressions for lines to exclude from consideration @@ -30,5 +43,6 @@ exclude_lines = precision = 1 include = + . xen-bugtool tests/* diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a004d10d..a94f54c0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -178,6 +178,30 @@ jobs: # GitHub PR workflows are already always rebased to the target branch (on run): SKIP: no-commit-to-branch,check-branch-needs-rebase + - uses: frabert/replace-string-action@v2 + id: get_sonarcloud_project_key + with: + pattern: '(\w+)/(\w+)' + replace-with: '$1_$2' + string: ${{ github.repository }} + + - name: SonarCloud Scan + uses: SonarSource/sonarcloud-github-action@v2 + if: ${{ env.SONAR_TOKEN }} + with: + args: > + -Dsonar.organization=${{ github.repository_owner }} + -Dsonar.projectKey=${{ steps.get_sonarcloud_project_key.outputs.replaced }} + -Dsonar.python.version=3.6 + -Dsonar.python.coverage.reportPaths=.git/coverage.xml + -Dsonar.python.xunit.reportPath=.git/pytest.xml + -Dsonar.sources=. + -Dsonar.exclusions=tests/** + -Dsonar.tests=tests + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + - name: Upload coverage reports to Codecov # If CODECOV_TOKEN is set, use the new codecov CLI to upload the coverage reports if: ${{ env.CODECOV_TOKEN && !cancelled() && github.event.pull_request.number }} diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index b642c503..4b9e468f 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -152,13 +152,8 @@ repos: 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 + entry: env PYTHONDEVMODE=yes sh -c + "coverage run && coverage xml && coverage html" require_serial: true pass_filenames: false language: python diff --git a/.vscode/ltex.dictionary.en-US.txt b/.vscode/ltex.dictionary.en-US.txt index fedab1be..35a29041 100644 --- a/.vscode/ltex.dictionary.en-US.txt +++ b/.vscode/ltex.dictionary.en-US.txt @@ -38,6 +38,7 @@ cli CLOEXEC clusterd Codecov +codecovcli conf CONFD config @@ -60,6 +61,7 @@ dirs docstrings dont dp +Dsonar dunder efi efivars @@ -82,6 +84,8 @@ filesystem filetype finalizer firstboot +frabert +frolvlad fs getgid getuid @@ -116,6 +120,7 @@ ltex maxsplit mdadm mdstat +Misha modinfo mountpoint mountpoints @@ -129,6 +134,8 @@ networkd NEWNET NEWNS nonexisting +noninteractive +noqa NOSONAR NRPE nsswitch @@ -162,6 +169,7 @@ pytest pytest's PYTHONDEVMODE PYTHONDONTWRITEBYTECODE +PYTHONWARNINGS pytype rebase reportMissingImports @@ -182,6 +190,7 @@ snmp snmpd softirqs softnet +sonarcloud sourcery src startswith @@ -213,6 +222,7 @@ tmpdata tmpdir tmpdir's tmpfs +tokenless toolstack toplevel typeshed @@ -259,6 +269,7 @@ xml xs xsconfig xsversion +xunit xvda yestoall zipfile diff --git a/README.md b/README.md index 27277ba8..8b960abc 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,9 @@ # Developer Documentation for Xen-Bugtool +[![Quality Gate Status](https://sonarcloud.io/api/project_badges/measure?project=xenserver-next_status-report&metric=alert_status)](https://sonarcloud.io/summary/new_code?id=xenserver-next_status-report) +[![Bugs](https://sonarcloud.io/api/project_badges/measure?project=xenserver-next_status-report&metric=bugs)](https://sonarcloud.io/summary/new_code?id=xenserver-next_status-report) +[![Code Smells](https://sonarcloud.io/api/project_badges/measure?project=xenserver-next_status-report&metric=code_smells)](https://sonarcloud.io/summary/new_code?id=xenserver-next_status-report) + This developer documentation provides detailed information about the development environment for `xen-bugtool`, a tool designed to assist with debugging XenServer issues. diff --git a/pyproject.toml b/pyproject.toml index 997baf96..30fb75c7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,7 +3,7 @@ name = "xenserver-status-report" dynamic = ["version"] description = "Xenserver Status Report" -requires-python = "2.7" +requires-python = ">=2.7" license = "LGPL-2.1-only" keywords = ["xenserver", "xen-project"] authors = [ diff --git a/pytest.ini b/pytest.ini index 25a9f82c..c685640f 100644 --- a/pytest.ini +++ b/pytest.ini @@ -6,6 +6,9 @@ filterwarnings=ignore:the imp module is deprecated # Enable live logging of the python log output, starting with log level INFO by default: log_cli=True log_cli_level=INFO +required_plugins= + pyfakefs + pytest-mock # By default, run the tests in the tests directory: testpaths=tests/ diff --git a/sonar-project.properties b/sonar-project.properties new file mode 100644 index 00000000..8e628a1b --- /dev/null +++ b/sonar-project.properties @@ -0,0 +1,15 @@ +sonar.organization=xenserver-next +sonar.projectKey=xenserver-next_status-report +sonar.python.version=3.6 +sonar.python.coverage.reportPaths=.git/coverage.xml +sonar.python.xunit.reportPath=.git/pytest.xml +# sonar.python.mypy.reportPaths=.git/mypy.xml +# sonar.python.ruff.reportPaths=.git/ruff.xml +# sonar.python.pylint.reportPaths=.git/pylint.xml + +# Patterns used to include some source files and only these ones in analysis. +# By default, all files in project sources are included. +# relative paths to source directories. More details and properties are described +# in https://sonarcloud.io/documentation/project-administration/narrowing-the-focus/ +sonar.sources=. +sonar.tests=tests diff --git a/tests/integration/test_system_load.py b/tests/integration/test_system_load.py index f45844e1..d6b20960 100644 --- a/tests/integration/test_system_load.py +++ b/tests/integration/test_system_load.py @@ -28,7 +28,7 @@ def test_system_load(output_archive_type="zip"): os.environ["PATH"] = "/var:" + os.environ["PATH"] with open("/var/sar", "w") as sar: sar.write("#!/bin/sh\nsleep 1;cat /etc/xensource-inventory\n") - os.chmod("/var/sar", 0o777) + os.chmod("/var/sar", 0o777) # nosec run_bugtool_entry(output_archive_type, entry) diff --git a/tests/integration/test_xenserver_config.py b/tests/integration/test_xenserver_config.py index e23a328f..797dbc12 100644 --- a/tests/integration/test_xenserver_config.py +++ b/tests/integration/test_xenserver_config.py @@ -40,17 +40,21 @@ def test_xenserver_config(output_archive_type): assert_content_from_dom0_template("etc/xensource-inventory") # Sample SNMP config files are currently not in the dom0_template! - # Reading them records the error message in the file content, do we want this? - # I think the "Failed to filter" is redundant in it. - # Maybe decide on a standardized error for missing files in bugtool? - - # TODO: To be clarified or fixed as part of CP-46759 or a follow-up! + # sourcery skip: no-loop-in-tests for input_file in [ "/etc/snmp/snmp.xs.conf", "/etc/snmp/snmpd.xs.conf", "/var/lib/net-snmp/snmpd.conf", ]: + # + # Here, we assert that the filter functions were called and tried to read + # the file (they fail here as the files are not part of the dom0_template). + # + # But this is fine, all we want to test here is that the filter was called: + # The filter functions are tested as part of the unit tests + # in tests/unit/test_snmp.py instead: + # assert_file( input_file.split("/")[-1].replace(".", "_") + ".out", # That's a very long error message an the 1st two parts are redundant: diff --git a/tests/integration/utils.py b/tests/integration/utils.py index 7d392fb1..072f8adc 100644 --- a/tests/integration/utils.py +++ b/tests/integration/utils.py @@ -116,9 +116,9 @@ def extract(zip_or_tar_archive, archive_type): # pragma: no cover if archive_type == "zip": archive = zipfile.ZipFile(zip_or_tar_archive) # type: zipfile.ZipFile|tarfile.TarFile elif archive_type == "tar": - archive = tarfile.open(zip_or_tar_archive) + archive = tarfile.open(zip_or_tar_archive) # NOSONAR elif archive_type == "tar.bz2": - archive = tarfile.open(zip_or_tar_archive, "r:bz2") + archive = tarfile.open(zip_or_tar_archive, "r:bz2") # NOSONAR else: raise RuntimeError("Unsupported output archive type: %s" % archive_type) archive.extractall() diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 9fc65235..c30d173c 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -186,10 +186,10 @@ def isolated_bugtool(bugtool_log): """ # Make the current cwd (a temporary directory) read-only: - os.chmod(".", 0o555) + os.chmod(".", 0o555) # nosec yield bugtool_log # runs the test function in the read-only directory - os.chmod(".", 0o777) # restore write permissions (for cleanup) + os.chmod(".", 0o777) # nosec # restore write permissions (for cleanup) # upon return, bugtool_log continues with its cleanup diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index cb7f1fe2..566799f6 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -196,11 +196,15 @@ def check_output(bugtool, captured, tmp_path, filename, filetype): # Extract the created output file into the tmp_path if filetype == "zip": - zipfile.ZipFile(filename).extractall(tmp_path) + zipfile.ZipFile(filename).extractall(tmp_path) # nosec # noqa: B202 # NOSONAR elif filetype == "tar": - tarfile.TarFile.open(filename).extractall(tmp_path) + tar = tarfile.TarFile.open(filename) + tar.extractall(tmp_path) # nosec + tar.close() elif filetype == "tar.bz2": - tarfile.TarFile.open(filename, "r:bz2").extractall(tmp_path) + tar = tarfile.TarFile.open(filename, "r:bz2") + tar.extractall(tmp_path) # nosec + tar.close() output_directory = filename.replace(".%s" % filetype, "/") diff --git a/tests/unit/test_output.py b/tests/unit/test_output.py index eb5dabb1..ee661354 100644 --- a/tests/unit/test_output.py +++ b/tests/unit/test_output.py @@ -27,6 +27,9 @@ def mock_data_collector(capability): raise Exception("mock data collector failed") +ETC_PASSWD = "/etc/passwd" + + def assert_valid_inventory_schema(inventory_tree): """Assert that the passed inventory validates against the inventory schema""" @@ -55,21 +58,21 @@ def assert_mock_bugtool_plugin_output(temporary_directory, subdir, names): # Will be refactored to be more easy in a separate commit soon: assert_valid_inventory_schema(parse(extracted + "inventory.xml")) with open(extracted + "proc_version.out") as proc_version: - assert proc_version.read()[:14] == "Linux version " + assert proc_version.read().startswith("Linux version ") with open(extracted + "ls-l-%etc.out") as etc: - assert etc.read()[:6] == "total " + assert etc.read().startswith("total ") with open(extracted + "proc/self/status") as status: - assert status.read()[:5] == "Name:" + assert status.read().startswith("Name:") with open(extracted + "proc/sys/fs/epoll/max_user_watches") as max_user_watches: assert int(max_user_watches.read()) > 0 with open(extracted + "etc/group") as group: assert group.readline() == "root:x:0:\n" # Check the contents of the sub-archive "etc/passwd.tar": - with tarfile.TarFile(extracted + "etc/passwd.tar") as tar: - assert tar.getnames() == [subdir + "/etc/passwd"] + with tarfile.TarFile(extracted + ETC_PASSWD + ".tar") as tar: + assert tar.getnames() == [subdir + ETC_PASSWD] # TarFile.extractfile() does not support context managers on Python2: - passwd = tar.extractfile(subdir + "/etc/passwd") + passwd = tar.extractfile(subdir + ETC_PASSWD) assert passwd assert passwd.readline() == b"root:x:0:0:root:/root:/bin/bash\n" passwd.close() @@ -82,7 +85,7 @@ def minimal_bugtool(bugtool, dom0_template, archive, subdir, mocker): # Load the mock plugin from dom0_template and process the plugin's caps: bugtool.PLUGIN_DIR = dom0_template + "/etc/xensource/bugtool" bugtool.entries = ["mock"] - archive.declare_subarchive("/etc/passwd", subdir + "/etc/passwd.tar") + archive.declare_subarchive(ETC_PASSWD, subdir + ETC_PASSWD + ".tar") # For code coverage: This sub-archive will not be created as it has no file archive.declare_subarchive("/not/existing", subdir + "/not_created.tar") bugtool.load_plugins(just_capabilities=False) diff --git a/xen-bugtool b/xen-bugtool index a71a4709..e5a779c1 100755 --- a/xen-bugtool +++ b/xen-bugtool @@ -515,9 +515,7 @@ class StringIOmtime(io.BytesIO): def no_unicode(x): - if isinstance(x, unicode_type): - return x.encode('utf-8') - return x + return x.encode("utf-8") if isinstance(x, unicode_type) else x @contextmanager @@ -2184,7 +2182,7 @@ def disk_list(): with open(PROC_PARTITIONS, "r") as f: f.readline() f.readline() - for line in f.readlines(): + for line in f: major, _, _, name = line.split() if int(major) < 254 and not partition_re.match(name): disks.append(name) @@ -2251,9 +2249,12 @@ class ProcOutput: def read_line(self): assert self.running + assert self.proc + assert self.proc.stdout line = self.proc.stdout.readline() if not line: # process exited + self.proc.stdout.close() self.status = self.proc.wait() self.proc = None self.running = False