From e1ca5c80711b53465ea61ffbfe99785d445627e0 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 10 Aug 2023 16:14:08 +0200 Subject: [PATCH 1/5] Create symlinks to test artifacts for pytest runner While temporary directories are useful for test execution to keep everything clean, they are difficult to work with manually. Create a symlink for each test artifact directory with a stable and predictable path. The symlink always either points to the latest artifacts, or is missing in case the last run succeeded. Ensure these symlinked directories aren't detected as test suites by the pytest runner. --- bin/tests/system/.gitignore | 1 + bin/tests/system/conftest.py | 34 +++++++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/bin/tests/system/.gitignore b/bin/tests/system/.gitignore index 766ed6a800..0488948e4a 100644 --- a/bin/tests/system/.gitignore +++ b/bin/tests/system/.gitignore @@ -2,6 +2,7 @@ .hypothesis .mypy_cache __pycache__ +_last_test_run dig.out* rndc.out* nsupdate.out* diff --git a/bin/tests/system/conftest.py b/bin/tests/system/conftest.py index 29799266e4..fd7510827f 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -104,6 +104,9 @@ else: ] PRIORITY_TESTS_RE = re.compile("|".join(PRIORITY_TESTS)) CONFTEST_LOGGER = logging.getLogger("conftest") + SYSTEM_TEST_DIR_GIT_PATH = "bin/tests/system" + SYSTEM_TEST_NAME_RE = re.compile(f"{SYSTEM_TEST_DIR_GIT_PATH}" + r"/([^/]+)") + SYMLINK_REPLACEMENT_RE = re.compile(r"/tests(_sh(?=_))?(.*)\.py") # ---------------------- Module initialization --------------------------- @@ -227,8 +230,16 @@ else: # bin/tests/system. These temporary directories contain all files # needed for the system tests - including tests_*.py files. Make sure to # ignore these during test collection phase. Otherwise, test artifacts - # from previous runs could mess with the runner. - return "_tmp_" in str(path) + # from previous runs could mess with the runner. Also ignore the + # convenience symlinks to those test directories. In both of those + # cases, the system test name (directory) contains an underscore, which + # is otherwise and invalid character for a system test name. + match = SYSTEM_TEST_NAME_RE.search(str(path)) + if match is None: + CONFTEST_LOGGER.warning("unexpected test path: %s (ignored)", path) + return True + system_test_name = match.groups()[0] + return "_" in system_test_name def pytest_collection_modifyitems(items): """Schedule long-running tests first to get more benefit from parallelism.""" @@ -345,8 +356,8 @@ else: """Dictionary containing environment variables for the test.""" env = os.environ.copy() env.update(ports) - env["builddir"] = f"{env['TOP_BUILDDIR']}/bin/tests/system" - env["srcdir"] = f"{env['TOP_SRCDIR']}/bin/tests/system" + env["builddir"] = f"{env['TOP_BUILDDIR']}/{SYSTEM_TEST_DIR_GIT_PATH}" + env["srcdir"] = f"{env['TOP_SRCDIR']}/{SYSTEM_TEST_DIR_GIT_PATH}" return env @pytest.fixture(scope="module") @@ -409,14 +420,26 @@ else: assert all(res.outcome == "passed" for res in test_results.values()) return "passed" + def unlink(path): + try: + path.unlink() # missing_ok=True isn't available on Python 3.6 + except FileNotFoundError: + pass + # Create a temporary directory with a copy of the original system test dir contents - system_test_root = Path(f"{env['TOP_BUILDDIR']}/bin/tests/system") + system_test_root = Path(f"{env['TOP_BUILDDIR']}/{SYSTEM_TEST_DIR_GIT_PATH}") testdir = Path( tempfile.mkdtemp(prefix=f"{system_test_name}_tmp_", dir=system_test_root) ) shutil.rmtree(testdir) shutil.copytree(system_test_root / system_test_name, testdir) + # Create a convenience symlink with a stable and predictable name + module_name = SYMLINK_REPLACEMENT_RE.sub(r"\2", request.node.name) + symlink_dst = system_test_root / module_name + unlink(symlink_dst) + symlink_dst.symlink_to(os.path.relpath(testdir, start=system_test_root)) + # Configure logger to write to a file inside the temporary test directory mlogger.handlers.clear() mlogger.setLevel(logging.DEBUG) @@ -451,6 +474,7 @@ else: handler.flush() handler.close() shutil.rmtree(testdir) + unlink(symlink_dst) def _run_script( # pylint: disable=too-many-arguments env, From f91d0b13e882f2b265412eadb958d2faa64974f8 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 10 Aug 2023 16:24:38 +0200 Subject: [PATCH 2/5] Improve tempdir logging for pytest runner At the end of the test, display the symlink path to the artifact directory in case it's preserved. Log the full tempdir name in debug log. --- bin/tests/system/conftest.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/bin/tests/system/conftest.py b/bin/tests/system/conftest.py index fd7510827f..63b992eb40 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -451,7 +451,7 @@ else: # System tests are meant to be executed from their directory - switch to it. old_cwd = os.getcwd() os.chdir(testdir) - mlogger.info("switching to tmpdir: %s", testdir) + mlogger.debug("switching to tmpdir: %s", testdir) try: yield testdir # other fixtures / tests will execute here finally: @@ -461,13 +461,27 @@ else: result = get_test_result() # Clean temporary dir unless it should be kept + keep = False if request.config.getoption("--noclean"): - mlogger.debug("--noclean requested, keeping temporary directory") + mlogger.debug( + "--noclean requested, keeping temporary directory %s", testdir + ) + keep = True elif result == "failed": - mlogger.debug("test failure detected, keeping temporary directory") + mlogger.debug( + "test failure detected, keeping temporary directory %s", testdir + ) + keep = True elif not request.node.stash[FIXTURE_OK]: mlogger.debug( - "test setup/teardown issue detected, keeping temporary directory" + "test setup/teardown issue detected, keeping temporary directory %s", + testdir, + ) + keep = True + + if keep: + mlogger.info( + "test artifacts in: %s", symlink_dst.relative_to(system_test_root) ) else: mlogger.debug("deleting temporary directory") From 83ddca76902bb54735528fa365e4b6c323b640fe Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 10 Aug 2023 16:53:10 +0200 Subject: [PATCH 3/5] Silence pylint's refactoring suggestions for system_test_dir() While it'd be fairly easy to split the function up into smaller ones, the readability wouldn't be improved in this case. Silence the suggestions instead. --- bin/tests/system/conftest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bin/tests/system/conftest.py b/bin/tests/system/conftest.py index 63b992eb40..fcdc379c47 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -378,7 +378,9 @@ else: return logging.getLogger(f"{system_test_name}.{request.node.name}") @pytest.fixture(scope="module") - def system_test_dir(request, env, system_test_name, mlogger): + def system_test_dir( + request, env, system_test_name, mlogger + ): # pylint: disable=too-many-statements,too-many-locals """ Temporary directory for executing the test. From d66ff81543dee6e07d975d5136d01505d72ac69f Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Wed, 16 Aug 2023 10:38:09 +0200 Subject: [PATCH 4/5] Add clean-local target to clean pytest runner artifacts The command finds all directories in bin/tests/system which contain an underscore. Underscore indicates either a temporary directory (_tmp_), a symlink to test artifacts (TESTNAME_MODULENAME), or a python-related cache. Using underscore for a system test name is invalid and a hyphen must be used instead. --- bin/tests/system/Makefile.am | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/tests/system/Makefile.am b/bin/tests/system/Makefile.am index 3fa1cd446c..ab716ac0c7 100644 --- a/bin/tests/system/Makefile.am +++ b/bin/tests/system/Makefile.am @@ -238,3 +238,6 @@ AM_LOG_FLAGS = -r $(TESTS): legacy.run.sh test-local: check + +clean-local:: + -find $(builddir) -maxdepth 1 -type d -name "*_*" | xargs rm -rf From 355dc733911fcaf2df7039a6b609c04a09f948ae Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Wed, 16 Aug 2023 13:45:29 +0200 Subject: [PATCH 5/5] .gitignore temporary directories and symlinks in system test dir --- bin/tests/system/.gitignore | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bin/tests/system/.gitignore b/bin/tests/system/.gitignore index 0488948e4a..e27cf5efaa 100644 --- a/bin/tests/system/.gitignore +++ b/bin/tests/system/.gitignore @@ -20,4 +20,10 @@ named.run /start.sh /stop.sh /ifconfig.sh -/*_tmp_* + +# Ignore file names with underscore in their name except python or shell files. +# This is done to ignore the temporary directories and symlinks created by the +# pytest runner, which contain underscore in their file names. +/*_* +!/*_*.py +!/*_*.sh