From 89e6d1bc262f9e06af83f37c6c487252532d5bd3 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. (cherry picked from commit e1ca5c80711b53465ea61ffbfe99785d445627e0) --- 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 b9cda1a53a..d515e8ec36 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 088f0d87b8..326a02ef30 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -105,6 +105,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 --------------------------- @@ -228,8 +231,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.""" @@ -346,8 +357,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") @@ -410,14 +421,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) @@ -452,6 +475,7 @@ else: handler.flush() handler.close() shutil.rmtree(testdir) + unlink(symlink_dst) def _run_script( # pylint: disable=too-many-arguments env, From 4f6ea4fa1bd1da3059195055b83e950b70fc497f 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. (cherry picked from commit f91d0b13e882f2b265412eadb958d2faa64974f8) --- 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 326a02ef30..ad08ab9ead 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -452,7 +452,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: @@ -462,13 +462,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 4b3d0c0f46c618e111ef6b8e5110ed9f2f3f6b7e 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. (cherry picked from commit 83ddca76902bb54735528fa365e4b6c323b640fe) --- 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 ad08ab9ead..338338f1d6 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -379,7 +379,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 303de3e089165784e33b0b50c2da40a84d502a12 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. (cherry picked from commit d66ff81543dee6e07d975d5136d01505d72ac69f) --- 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 f98be0b416..be72b3d41c 100644 --- a/bin/tests/system/Makefile.am +++ b/bin/tests/system/Makefile.am @@ -253,3 +253,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 b6e522ad03d7eeaf628cc205acfd111f39fa23a8 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 (cherry picked from commit 355dc733911fcaf2df7039a6b609c04a09f948ae) --- 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 d515e8ec36..cdf333c5bf 100644 --- a/bin/tests/system/.gitignore +++ b/bin/tests/system/.gitignore @@ -23,4 +23,10 @@ parallel.mk /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