Changeset View
Standalone View
swh/loader/mercurial/from_disk.py
| Show First 20 Lines • Show All 161 Lines • ▼ Show 20 Lines | ): | ||||
| # hg node id of the latest snapshot branch heads | # hg node id of the latest snapshot branch heads | ||||
| # used to find what are the new revisions since last snapshot | # used to find what are the new revisions since last snapshot | ||||
| self._latest_heads: List[bytes] = [] | self._latest_heads: List[bytes] = [] | ||||
| self._load_status = "eventful" | self._load_status = "eventful" | ||||
| # If set, will override the default value | # If set, will override the default value | ||||
| self._visit_status = None | self._visit_status = None | ||||
| if "PYTEST_CURRENT_TEST" not in os.environ: | |||||
| # Do not execute in tests, since the tests already have a clean environment. | |||||
| # Loading multiple loaders in a test will cause other tests that rely on | |||||
| # environment variables to fail. | |||||
ardumont: do they have a clean environment because we clean it for the tests already?
(if yes, have you… | |||||
ardumontUnsubmitted Not Done Inline Actions
ping? My trail of thought there is why bypass this in test [1] and not remove the cleaning up done in the test context altogether instead. [1] opening divergent behavior for test in production code sounds wrong to me. ardumont: > do they have a clean environment because we clean it for the tests already?
> (if yes, have… | |||||
AlphareAuthorUnsubmitted Done Inline ActionsSorry, I must have misunderstood part of your comment and thought only the code change would suffice. I've figured out the underlying issue. I had a loader that I was initializing without calling .load(), which caused the environment to be left in a broken state. I'm updating this patch to instead do this in pre_cleanup. The full explanation is in the commit message. Alphare: Sorry, I must have misunderstood part of your comment and thought only the code change would… | |||||
ardumontUnsubmitted Not Done Inline Actions\o/ ardumont: \o/ | |||||
| # Playing with `os.environ` is all kinds of terrible, but unfortunately | |||||
| # `billiard` (which we use for subprocesses) does not appear to have a way | |||||
| # of handling this any better. | |||||
| self.old_environ = os.environ.copy() | self.old_environ = os.environ.copy() | ||||
| os.environ.clear() | os.environ.clear() | ||||
| os.environ.update(get_minimum_env()) | os.environ.update(get_minimum_env()) | ||||
| def pre_cleanup(self) -> None: | def pre_cleanup(self) -> None: | ||||
| """As a first step, will try and check for dangling data to cleanup. | """As a first step, will try and check for dangling data to cleanup. | ||||
| This should do its best to avoid raising issues. | This should do its best to avoid raising issues. | ||||
| """ | """ | ||||
| clean_dangling_folders( | clean_dangling_folders( | ||||
| self._temp_directory, | self._temp_directory, | ||||
| pattern_check=TEMPORARY_DIR_PREFIX_PATTERN, | pattern_check=TEMPORARY_DIR_PREFIX_PATTERN, | ||||
| log=self.log, | log=self.log, | ||||
| ) | ) | ||||
| def cleanup(self) -> None: | def cleanup(self) -> None: | ||||
| """Last step executed by the loader.""" | """Last step executed by the loader.""" | ||||
| if "PYTEST_CURRENT_TEST" not in os.environ: | |||||
| # See comment in `__init__` about this. | |||||
| os.environ.clear() | os.environ.clear() | ||||
| os.environ.update(self.old_environ) | os.environ.update(self.old_environ) | ||||
| if self._repo_directory and os.path.exists(self._repo_directory): | if self._repo_directory and os.path.exists(self._repo_directory): | ||||
| self.log.debug(f"Cleanup up repository {self._repo_directory}") | self.log.debug(f"Cleanup up repository {self._repo_directory}") | ||||
| rmtree(self._repo_directory) | rmtree(self._repo_directory) | ||||
| def prepare_origin_visit(self) -> None: | def prepare_origin_visit(self) -> None: | ||||
| """First step executed by the loader to prepare origin and visit | """First step executed by the loader to prepare origin and visit | ||||
| references. Set/update self.origin, and | references. Set/update self.origin, and | ||||
| ▲ Show 20 Lines • Show All 480 Lines • Show Last 20 Lines | |||||
do they have a clean environment because we clean it for the tests already?
(if yes, have you tried to remove that setup ^and check the behavior without the conditional?)
If we absolutely need this, at least, centralize the conditional in a function:
def testing_environment(): return "PYTEST_CURRENT_TEST" in os.environ ... # in runtime code if not testing_environment(): ...but if you prefer returning the conditional (make it negative in the function, positive at call time), fine with me as well ;)