From 259bfc04d41fabfe2e2fcb7cde1897afbdc60fd6 Mon Sep 17 00:00:00 2001 From: John Alex Date: Wed, 22 Feb 2023 22:16:13 +0000 Subject: [PATCH 1/2] test_sys_checkout: use actual paths in on-the-fly configs rather than MANIC_TEST_BARE_REPO_ROOT env var. This will make it easier to test (in the near future) that checkout_externals actually checked out the desired repo dir. The env-var path is still tested by two tests which rely on a checked-in externals.cfg that relies on an environment variable. More clearly document that env vars are passed to checkout_externals via os.environ, and that the command line assembled in _execute_checkout_in_dir is just for manual reproducibility. --- test/test_sys_checkout.py | 95 ++++++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 37 deletions(-) diff --git a/test/test_sys_checkout.py b/test/test_sys_checkout.py index 58066ae78..538667719 100644 --- a/test/test_sys_checkout.py +++ b/test/test_sys_checkout.py @@ -80,10 +80,9 @@ module_tmp_root_dir = None TMP_REPO_DIR_NAME = 'tmp' # subdir under $CWD -# 'bare repo root' is the test/repos/ subdir that holds all of our -# checked-in repositories. -MANIC_TEST_BARE_REPO_ROOT = 'MANIC_TEST_BARE_REPO_ROOT' # env var name -BARE_REPO_ROOT_NAME = 'repos' # subdir name +# subdir under test/ that holds all of our checked-in repositories (which we +# will clone for these tests). +BARE_REPO_ROOT_NAME = 'repos' # Subdirs under bare repo root, each holding a repository. For more info # on the contents of these repositories, see test/repos/README.md. In these @@ -234,10 +233,14 @@ class GenerateExternalsDescriptionCfgV1(object): Optionally after that: write_with_*(). """ - def __init__(self): + def __init__(self, bare_root): self._schema_version = '1.1.0' self._config = None + # directory where we have test repositories (which we will clone for + # tests) + self._bare_root = bare_root + def write_config(self, dest_dir, filename=CFG_NAME): """Write self._config to disk @@ -299,7 +302,7 @@ def create_section(self, repo_path, name, tag='', branch='', if repo_path_abs is not None: repo_url = repo_path_abs else: - repo_url = os.path.join('${MANIC_TEST_BARE_REPO_ROOT}', repo_path) + repo_url = os.path.join(self._bare_root, repo_path) if not from_submodule: self._config.set(name, ExternalsDescription.REPO_URL, repo_url) @@ -376,8 +379,7 @@ def write_with_git_branch(self, dest_dir, name, branch, new_remote_repo_path=Non if new_remote_repo_path == SIMPLE_LOCAL_ONLY_NAME: repo_url = SIMPLE_LOCAL_ONLY_NAME else: - repo_url = os.path.join('${MANIC_TEST_BARE_REPO_ROOT}', - new_remote_repo_path) + repo_url = os.path.join(self._bare_root, new_remote_repo_path) self._config.set(name, ExternalsDescription.REPO_URL, repo_url) try: @@ -415,7 +417,7 @@ def write_with_tag_and_remote_repo(self, dest_dir, name, tag, new_remote_repo_pa self._config.set(name, ExternalsDescription.TAG, tag) if new_remote_repo_path: - repo_url = os.path.join('${MANIC_TEST_BARE_REPO_ROOT}', new_remote_repo_path) + repo_url = os.path.join(self._bare_root, new_remote_repo_path) self._config.set(name, ExternalsDescription.REPO_URL, repo_url) try: @@ -464,19 +466,24 @@ def write_with_protocol(self, dest_dir, name, protocol, repo_path=None): self._config.set(name, ExternalsDescription.PROTOCOL, protocol) if repo_path: - repo_url = os.path.join('${MANIC_TEST_BARE_REPO_ROOT}', repo_path) + repo_url = os.path.join(self._bare_root, repo_path) self._config.set(name, ExternalsDescription.REPO_URL, repo_url) self.write_config(dest_dir) -def _execute_checkout_in_dir(dirname, args): +def _execute_checkout_in_dir(dirname, args, debug_env=''): """Execute the checkout command in the appropriate repo dir with the - specified additional args - + specified additional args. + + args should be a list of strings. + debug_env shuld be a string of the form 'FOO=bar' or the empty string. + Note that we are calling the command line processing and main routines and not using a subprocess call so that we get code - coverage results! + coverage results! Note this means that environment variables are passed + to checkout_externals via os.environ; debug_env is just used to aid + manual reproducibility of a given call. Returns (overall_status, tree_status) where overall_status is 0 for success, nonzero otherwise. @@ -486,15 +493,15 @@ def _execute_checkout_in_dir(dirname, args): necessarily do any checking out (e.g. if --status is passed in). """ cwd = os.getcwd() + + # Construct a command line for reproducibility; this command is not actually + # executed in the test. checkout_path = os.path.abspath('{0}/../../checkout_externals') os.chdir(dirname) cmdline = ['--externals', CFG_NAME, ] cmdline += args - repo_root = 'MANIC_TEST_BARE_REPO_ROOT={root}'.format( - root=os.environ[MANIC_TEST_BARE_REPO_ROOT]) manual_cmd = ('Test cmd:\npushd {cwd}; {env} {checkout} {args}'.format( - cwd=dirname, env=repo_root, checkout=checkout_path, - args=' '.join(cmdline))) + cwd=dirname, checkout=checkout_path, env=debug_env, args=' '.join(cmdline))) printlog(manual_cmd) options = checkout.commandline_arguments(cmdline) overall_status, tree_status = checkout.main(options) @@ -543,24 +550,19 @@ def setUp(self): # path to the executable self._checkout = os.path.join(root_dir, 'checkout_externals') - # directory where we have test repositories + # directory where we have test repositories (which we will clone for + # tests) self._bare_root = os.path.abspath( os.path.join(root_dir, 'test', BARE_REPO_ROOT_NAME)) - # set into the environment so var will be expanded in externals files - os.environ[MANIC_TEST_BARE_REPO_ROOT] = self._bare_root - # set the input file generator - self._generator = GenerateExternalsDescriptionCfgV1() + self._generator = GenerateExternalsDescriptionCfgV1(self._bare_root) # set the input file generator for secondary externals - self._sub_generator = GenerateExternalsDescriptionCfgV1() + self._sub_generator = GenerateExternalsDescriptionCfgV1(self._bare_root) def tearDown(self): """Tear down for individual tests """ - # remove the env var we added in setup - del os.environ[MANIC_TEST_BARE_REPO_ROOT] - # return to our common starting point os.chdir(self._return_dir) @@ -569,18 +571,20 @@ def clone_test_repo(self, parent_repo_name, dest_dir_in=None): return RepoUtils.clone_test_repo(self._bare_root, self._test_id, parent_repo_name, dest_dir_in) - def execute_checkout_in_dir(self, dirname, args): - overall_status, tree_status = _execute_checkout_in_dir(dirname, args) + def execute_checkout_in_dir(self, dirname, args, debug_env=''): + overall_status, tree_status = _execute_checkout_in_dir(dirname, args, + debug_env=debug_env) self.assertEqual(overall_status, 0) return tree_status - def execute_checkout_with_status(self, dirname, args): + def execute_checkout_with_status(self, dirname, args, debug_env=''): """Calls checkout a second time to get status if needed.""" tree_status = self.execute_checkout_in_dir( - dirname, args) + dirname, args, debug_env=debug_env) if tree_status is None: tree_status = self.execute_checkout_in_dir(dirname, - self.status_args) + self.status_args, + debug_env=debug_env) self.assertNotEqual(tree_status, None) return tree_status @@ -620,7 +624,7 @@ def test_required_bytag(self): cloned_repo_dir = self.clone_test_repo(CONTAINER_REPO) self._generator.create_config() self._generator.create_section(SIMPLE_REPO, TAG_SECTION, - tag='tag1') + tag='tag1') self._generator.write_config(cloned_repo_dir) # externals start out 'empty' aka not checked out. @@ -652,6 +656,7 @@ def test_required_bytag(self): self._check_file_absent(cloned_repo_dir, os.path.join(tag_path, 'simple_subdir', 'subdir_file.txt')) + def test_required_bybranch(self): """Check out a required external pointing to a git branch.""" cloned_repo_dir = self.clone_test_repo(CONTAINER_REPO) @@ -1135,9 +1140,14 @@ def test_container_mixed_subrepo(self): branch='master', sub_externals=CFG_SUB_NAME) self._generator.write_config(cloned_repo_dir) + # The subrepo has a repo_url that uses this environment variable. + os.environ['MANIC_TEST_BARE_REPO_ROOT'] = self._bare_root + debug_env = 'MANIC_TEST_BARE_REPO_ROOT=' + self._bare_root + # inital checkout: all requireds are clean, and optional is empty. tree = self.execute_checkout_with_status(cloned_repo_dir, - self.checkout_args) + self.checkout_args, + debug_env=debug_env) mixed_req_path = self._external_path('mixed_req') self._check_sync_clean(tree[mixed_req_path], ExternalStatus.STATUS_OK, @@ -1154,7 +1164,8 @@ def test_container_mixed_subrepo(self): self._generator.write_with_git_branch(cloned_repo_dir, name='mixed_req', branch='new-feature', new_remote_repo_path=MIXED_REPO) - tree = self.execute_checkout_in_dir(cloned_repo_dir, self.status_args) + tree = self.execute_checkout_in_dir(cloned_repo_dir, self.status_args, + debug_env=debug_env) self._check_sync_clean(tree[mixed_req_path], ExternalStatus.MODEL_MODIFIED, ExternalStatus.STATUS_OK) @@ -1163,13 +1174,16 @@ def test_container_mixed_subrepo(self): ExternalStatus.STATUS_OK) # run the checkout. Now the mixed use external and its sub-externals should be clean. - tree = self.execute_checkout_with_status(cloned_repo_dir, self.checkout_args) + tree = self.execute_checkout_with_status(cloned_repo_dir, self.checkout_args, + debug_env=debug_env) self._check_sync_clean(tree[mixed_req_path], ExternalStatus.STATUS_OK, ExternalStatus.STATUS_OK) self._check_sync_clean(tree[self._external_path('simp_branch', base_path=sub_ext_base_path)], ExternalStatus.STATUS_OK, ExternalStatus.STATUS_OK) + # Don't pollute environment of other tests. + del os.environ['MANIC_TEST_BARE_REPO_ROOT'] def test_container_component(self): """Verify that optional component checkout works @@ -1270,15 +1284,22 @@ def test_subexternal(self): self._generator.create_section_reference_to_subexternal('mixed_base') self._generator.write_config(cloned_repo_dir) + # The subrepo has a repo_url that uses this environment variable. + os.environ['MANIC_TEST_BARE_REPO_ROOT'] = self._bare_root + debug_env = 'MANIC_TEST_BARE_REPO_ROOT=' + self._bare_root + # After checkout, confirm required's are clean and the referenced # subexternal's contents are also clean. tree = self.execute_checkout_with_status(cloned_repo_dir, - self.checkout_args) + self.checkout_args, + debug_env=debug_env) self._check_sync_clean( tree[self._external_path(BRANCH_SECTION, base_path=SUB_EXTERNALS_PATH)], ExternalStatus.STATUS_OK, ExternalStatus.STATUS_OK) + # Don't pollute environment of other tests. + del os.environ['MANIC_TEST_BARE_REPO_ROOT'] def test_container_sparse(self): """Verify that 'full' container with simple subrepo From 4c96e824e366562e339e4201930fe4115c160304 Mon Sep 17 00:00:00 2001 From: John Alex Date: Thu, 23 Feb 2023 01:30:18 +0000 Subject: [PATCH 2/2] Make the MANIC_TEST_BARE_REPO_ROOT env var special - give it a constant for easy tracking, and automatically tear it down after each test. --- test/test_sys_checkout.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/test/test_sys_checkout.py b/test/test_sys_checkout.py index 538667719..37a637ad7 100644 --- a/test/test_sys_checkout.py +++ b/test/test_sys_checkout.py @@ -82,7 +82,12 @@ # subdir under test/ that holds all of our checked-in repositories (which we # will clone for these tests). -BARE_REPO_ROOT_NAME = 'repos' +BARE_REPO_ROOT_NAME = 'repos' + +# Environment var referenced by checked-in externals file in mixed-cont-ext.git, +# which should be pointed to the fully-resolved BARE_REPO_ROOT_NAME directory. +# We explicitly clear this after every test, via tearDown(). +MIXED_CONT_EXT_ROOT_ENV_VAR = 'MANIC_TEST_BARE_REPO_ROOT' # Subdirs under bare repo root, each holding a repository. For more info # on the contents of these repositories, see test/repos/README.md. In these @@ -565,6 +570,10 @@ def tearDown(self): """ # return to our common starting point os.chdir(self._return_dir) + + # (in case this was set) Don't pollute environment of other tests. + os.environ.pop(MIXED_CONT_EXT_ROOT_ENV_VAR, + None) # Don't care if key wasn't set. def clone_test_repo(self, parent_repo_name, dest_dir_in=None): """Clones repo under self._bare_root""" @@ -1141,8 +1150,9 @@ def test_container_mixed_subrepo(self): self._generator.write_config(cloned_repo_dir) # The subrepo has a repo_url that uses this environment variable. - os.environ['MANIC_TEST_BARE_REPO_ROOT'] = self._bare_root - debug_env = 'MANIC_TEST_BARE_REPO_ROOT=' + self._bare_root + # It'll be cleared in tearDown(). + os.environ[MIXED_CONT_EXT_ROOT_ENV_VAR] = self._bare_root + debug_env = MIXED_CONT_EXT_ROOT_ENV_VAR + '=' + self._bare_root # inital checkout: all requireds are clean, and optional is empty. tree = self.execute_checkout_with_status(cloned_repo_dir, @@ -1182,8 +1192,6 @@ def test_container_mixed_subrepo(self): self._check_sync_clean(tree[self._external_path('simp_branch', base_path=sub_ext_base_path)], ExternalStatus.STATUS_OK, ExternalStatus.STATUS_OK) - # Don't pollute environment of other tests. - del os.environ['MANIC_TEST_BARE_REPO_ROOT'] def test_container_component(self): """Verify that optional component checkout works @@ -1285,8 +1293,9 @@ def test_subexternal(self): self._generator.write_config(cloned_repo_dir) # The subrepo has a repo_url that uses this environment variable. - os.environ['MANIC_TEST_BARE_REPO_ROOT'] = self._bare_root - debug_env = 'MANIC_TEST_BARE_REPO_ROOT=' + self._bare_root + # It'll be cleared in tearDown(). + os.environ[MIXED_CONT_EXT_ROOT_ENV_VAR] = self._bare_root + debug_env = MIXED_CONT_EXT_ROOT_ENV_VAR + '=' + self._bare_root # After checkout, confirm required's are clean and the referenced # subexternal's contents are also clean. @@ -1298,8 +1307,6 @@ def test_subexternal(self): tree[self._external_path(BRANCH_SECTION, base_path=SUB_EXTERNALS_PATH)], ExternalStatus.STATUS_OK, ExternalStatus.STATUS_OK) - # Don't pollute environment of other tests. - del os.environ['MANIC_TEST_BARE_REPO_ROOT'] def test_container_sparse(self): """Verify that 'full' container with simple subrepo