From 6d0c0f585f9c83c702186444bc7418794bf5a4ad Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Sat, 29 May 2021 21:07:25 +0900 Subject: [PATCH 1/6] test(man): fix inactive "required_cmd" Fixes the test failure of `test_10' (test_man.py). The decorator `@pytest.mark.complete(require_cmd=True)' is referenced only when the fixture `completion' is requested. Since `completion' was not requested by `test_10', `require_cmd=True' was ignored. Now `test_10' checks the command (non-)existence on its own for test skipping. --- test/t/test_man.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/t/test_man.py b/test/t/test_man.py index 366b54d29cc..bd891df0e96 100644 --- a/test/t/test_man.py +++ b/test/t/test_man.py @@ -4,6 +4,7 @@ assert_bash_exec, assert_complete, bash_env_saved, + is_bash_type, prepare_fixture_dir, ) @@ -105,8 +106,9 @@ def test_8(self, completion): def test_9(self, bash, completion): assert self.assumed_present in completion - @pytest.mark.complete(require_cmd=True) def test_10(self, request, bash, colonpath): + if not is_bash_type(bash, "man"): + pytest.skip("Command not found") with bash_env_saved(bash) as bash_env: bash_env.write_env( "MANPATH", From ebe9ad67496484982179d82f9d31a5f46e42bcbe Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Sun, 30 May 2021 20:32:56 +0900 Subject: [PATCH 2/6] test(bash_env_saved): correctly restore "cwd" In the previous version of `bash_env_saved', `bash.cwd' was used to retrieve the current working directory of the Bash process, but it turned out that `bash.cwd' just retains the value of the startup time of the process but not the current one. Instead we shall store the real current working directory in a shell variable inside the Bash process. --- test/t/conftest.py | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/test/t/conftest.py b/test/t/conftest.py index 020b5c3fe7f..3ff47ea66e7 100644 --- a/test/t/conftest.py +++ b/test/t/conftest.py @@ -401,7 +401,7 @@ def assert_bash_exec( class bash_env_saved: def __init__(self, bash: pexpect.spawn, sendintr: bool = False): self.bash = bash - self.cwd: Optional[str] = None + self.cwd_changed: bool = False self.saved_shopt: Dict[str, int] = {} self.saved_variables: Dict[str, int] = {} self.sendintr = sendintr @@ -429,8 +429,9 @@ def _unset_variable(self, varname: str): assert_bash_exec(self.bash, "unset -v %s" % varname) def _save_cwd(self): - if not self.cwd: - self.cwd = self.bash.cwd + if not self.cwd_changed: + self.cwd_changed = True + self._copy_variable("PWD", "_BASHCOMP_TEST_OLDPWD") def _check_shopt(self, name: str): assert_bash_exec( @@ -457,11 +458,15 @@ def _protect_shopt(self, name: str): ) def _check_variable(self, varname: str): - assert_bash_exec( - self.bash, - '[[ ${%s-%s} == "${_BASHCOMP_TEST_NEWVAR_%s-%s}" ]]' - % (varname, MAGIC_MARK2, varname, MAGIC_MARK2), - ) + try: + assert_bash_exec( + self.bash, + '[[ ${%s-%s} == "${_BASHCOMP_TEST_NEWVAR_%s-%s}" ]]' + % (varname, MAGIC_MARK2, varname, MAGIC_MARK2), + ) + except AssertionError: + self._copy_variable("_BASHCOMP_TEST_NEWVAR_" + varname, varname) + raise def _unprotect_variable(self, varname: str): if varname not in self.saved_variables: @@ -480,13 +485,14 @@ def _restore_env(self): # We first go back to the original directory before restoring # variables because "cd" affects "OLDPWD". - if self.cwd: + if self.cwd_changed: self._unprotect_variable("OLDPWD") assert_bash_exec( - self.bash, "command cd -- %s" % shlex.quote(str(self.cwd)) + self.bash, 'command cd -- "$_BASHCOMP_TEST_OLDPWD"' ) self._protect_variable("OLDPWD") - self.cwd = None + self._unset_variable("_BASHCOMP_TEST_OLDPWD") + self.cwd_changed = False for name in self.saved_shopt: self._check_shopt(name) @@ -506,6 +512,7 @@ def _restore_env(self): def chdir(self, path: str): self._save_cwd() + self.cwd_changed = True self._unprotect_variable("OLDPWD") assert_bash_exec(self.bash, "command cd -- %s" % shlex.quote(path)) self._protect_variable("OLDPWD") From f469727f470c50bec504168975e2ad182becf017 Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Sun, 30 May 2021 20:49:27 +0900 Subject: [PATCH 3/6] test(bash_env_saved): continue restoring on restoring error `bash_env_saved' now tries to proceed the process of the restoration of the shell environment even when some of restoration operations fail in order to reduce later errors caused by incomplete restoration. --- test/t/conftest.py | 90 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 63 insertions(+), 27 deletions(-) diff --git a/test/t/conftest.py b/test/t/conftest.py index 3ff47ea66e7..e9188c2afcf 100644 --- a/test/t/conftest.py +++ b/test/t/conftest.py @@ -406,6 +406,9 @@ def __init__(self, bash: pexpect.spawn, sendintr: bool = False): self.saved_variables: Dict[str, int] = {} self.sendintr = sendintr + self.noexcept: bool = False + self.captured_error: Optional[Exception] = None + def __enter__(self): return self @@ -418,15 +421,46 @@ def __exit__( self._restore_env() return None + def _safe_sendintr(self): + try: + self.bash.sendintr() + self.bash.expect_exact(PS1) + except Exception as e: + if self.noexcept: + self.captured_error = e + else: + raise + + def _safe_exec(self, cmd: str): + try: + self.bash.sendline(cmd) + self.bash.expect_exact(cmd) + self.bash.expect_exact("\r\n" + PS1) + except Exception as e: + if self.noexcept: + self._safe_sendintr() + self.captured_error = e + else: + raise + + def _safe_assert(self, cmd: str): + try: + assert_bash_exec(self.bash, cmd, want_output=None) + except Exception as e: + if self.noexcept: + self._safe_sendintr() + self.captured_error = e + else: + raise + def _copy_variable(self, src_var: str, dst_var: str): - assert_bash_exec( - self.bash, + self._safe_exec( "if [[ ${%s+set} ]]; then %s=${%s}; else unset -v %s; fi" % (src_var, dst_var, src_var, dst_var), ) def _unset_variable(self, varname: str): - assert_bash_exec(self.bash, "unset -v %s" % varname) + self._safe_exec("unset -v %s" % varname) def _save_cwd(self): if not self.cwd_changed: @@ -434,8 +468,7 @@ def _save_cwd(self): self._copy_variable("PWD", "_BASHCOMP_TEST_OLDPWD") def _check_shopt(self, name: str): - assert_bash_exec( - self.bash, + self._safe_assert( '[[ $(shopt -p %s) == "${_BASHCOMP_TEST_NEWSHOPT_%s}" ]]' % (name, name), ) @@ -443,30 +476,32 @@ def _check_shopt(self, name: str): def _unprotect_shopt(self, name: str): if name not in self.saved_shopt: self.saved_shopt[name] = 1 - assert_bash_exec( - self.bash, - "_BASHCOMP_TEST_OLDSHOPT_%s=$(shopt -p %s; true)" + self._safe_exec( + "_BASHCOMP_TEST_OLDSHOPT_%s=$(shopt -p %s || true)" % (name, name), ) else: self._check_shopt(name) def _protect_shopt(self, name: str): - assert_bash_exec( - self.bash, - "_BASHCOMP_TEST_NEWSHOPT_%s=$(shopt -p %s; true)" % (name, name), + self._safe_exec( + "_BASHCOMP_TEST_NEWSHOPT_%s=$(shopt -p %s || true)" % (name, name), ) def _check_variable(self, varname: str): try: - assert_bash_exec( - self.bash, + self._safe_assert( '[[ ${%s-%s} == "${_BASHCOMP_TEST_NEWVAR_%s-%s}" ]]' % (varname, MAGIC_MARK2, varname, MAGIC_MARK2), ) - except AssertionError: + except Exception: self._copy_variable("_BASHCOMP_TEST_NEWVAR_" + varname, varname) raise + else: + if self.noexcept and self.captured_error: + self._copy_variable( + "_BASHCOMP_TEST_NEWVAR_" + varname, varname + ) def _unprotect_variable(self, varname: str): if varname not in self.saved_variables: @@ -479,26 +514,23 @@ def _protect_variable(self, varname: str): self._copy_variable(varname, "_BASHCOMP_TEST_NEWVAR_" + varname) def _restore_env(self): + self.noexcept = True + if self.sendintr: - self.bash.sendintr() - self.bash.expect_exact(PS1) + self._safe_sendintr() # We first go back to the original directory before restoring # variables because "cd" affects "OLDPWD". if self.cwd_changed: self._unprotect_variable("OLDPWD") - assert_bash_exec( - self.bash, 'command cd -- "$_BASHCOMP_TEST_OLDPWD"' - ) + self._safe_exec('command cd -- "$_BASHCOMP_TEST_OLDPWD"') self._protect_variable("OLDPWD") self._unset_variable("_BASHCOMP_TEST_OLDPWD") self.cwd_changed = False for name in self.saved_shopt: self._check_shopt(name) - assert_bash_exec( - self.bash, 'eval "$_BASHCOMP_TEST_OLDSHOPT_%s"' % name - ) + self._safe_exec('eval "$_BASHCOMP_TEST_OLDSHOPT_%s"' % name) self._unset_variable("_BASHCOMP_TEST_OLDSHOPT_" + name) self._unset_variable("_BASHCOMP_TEST_NEWSHOPT_" + name) self.saved_shopt = {} @@ -510,26 +542,30 @@ def _restore_env(self): self._unset_variable("_BASHCOMP_TEST_NEWVAR_" + varname) self.saved_variables = {} + self.noexcept = False + if self.captured_error: + raise self.captured_error + def chdir(self, path: str): self._save_cwd() self.cwd_changed = True self._unprotect_variable("OLDPWD") - assert_bash_exec(self.bash, "command cd -- %s" % shlex.quote(path)) + self._safe_exec("command cd -- %s" % shlex.quote(path)) self._protect_variable("OLDPWD") def shopt(self, name: str, value: bool): self._unprotect_shopt(name) if value: - assert_bash_exec(self.bash, "shopt -s %s" % name) + self._safe_exec("shopt -s %s" % name) else: - assert_bash_exec(self.bash, "shopt -u %s" % name) + self._safe_exec("shopt -u %s" % name) self._protect_shopt(name) def write_variable(self, varname: str, new_value: str, quote: bool = True): if quote: new_value = shlex.quote(new_value) self._unprotect_variable(varname) - assert_bash_exec(self.bash, "%s=%s" % (varname, new_value)) + self._safe_exec("%s=%s" % (varname, new_value)) self._protect_variable(varname) # TODO: We may restore the "export" attribute as well though it is @@ -538,7 +574,7 @@ def write_env(self, envname: str, new_value: str, quote: bool = True): if quote: new_value = shlex.quote(new_value) self._unprotect_variable(envname) - assert_bash_exec(self.bash, "export %s=%s" % (envname, new_value)) + self._safe_exec("export %s=%s" % (envname, new_value)) self._protect_variable(envname) From cd39105d934f178b136fcfe04928f70c3f3d982f Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Mon, 31 May 2021 06:35:01 +0900 Subject: [PATCH 4/6] test(bash_env_saved): work around nested bash_env In the previous version of `bash_env_saved', when the same variables are saved or restored in the nested `with bash_env_saved()' statement, the variables are not correctly restored. It is not clear whether there are real instances of such cases in existing tests, but `bash_env_saved' shall support the nested call of `with bash_env_saved()' to avoid future troubles. --- test/t/conftest.py | 54 ++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/test/t/conftest.py b/test/t/conftest.py index e9188c2afcf..69fa45817a9 100644 --- a/test/t/conftest.py +++ b/test/t/conftest.py @@ -399,7 +399,12 @@ def assert_bash_exec( class bash_env_saved: + counter: int = 0 + def __init__(self, bash: pexpect.spawn, sendintr: bool = False): + bash_env_saved.counter += 1 + self.prefix: str = "_BASHCOMP_TEST%d" % bash_env_saved.counter + self.bash = bash self.cwd_changed: bool = False self.saved_shopt: Dict[str, int] = {} @@ -465,53 +470,58 @@ def _unset_variable(self, varname: str): def _save_cwd(self): if not self.cwd_changed: self.cwd_changed = True - self._copy_variable("PWD", "_BASHCOMP_TEST_OLDPWD") + self._copy_variable("PWD", "%s_OLDPWD" % self.prefix) def _check_shopt(self, name: str): self._safe_assert( - '[[ $(shopt -p %s) == "${_BASHCOMP_TEST_NEWSHOPT_%s}" ]]' - % (name, name), + '[[ $(shopt -p %s) == "${%s_NEWSHOPT_%s}" ]]' + % (name, self.prefix, name), ) def _unprotect_shopt(self, name: str): if name not in self.saved_shopt: self.saved_shopt[name] = 1 self._safe_exec( - "_BASHCOMP_TEST_OLDSHOPT_%s=$(shopt -p %s || true)" - % (name, name), + "%s_OLDSHOPT_%s=$(shopt -p %s || true)" + % (self.prefix, name, name), ) else: self._check_shopt(name) def _protect_shopt(self, name: str): self._safe_exec( - "_BASHCOMP_TEST_NEWSHOPT_%s=$(shopt -p %s || true)" % (name, name), + "%s_NEWSHOPT_%s=$(shopt -p %s || true)" + % (self.prefix, name, name), ) def _check_variable(self, varname: str): try: self._safe_assert( - '[[ ${%s-%s} == "${_BASHCOMP_TEST_NEWVAR_%s-%s}" ]]' - % (varname, MAGIC_MARK2, varname, MAGIC_MARK2), + '[[ ${%s-%s} == "${%s_NEWVAR_%s-%s}" ]]' + % (varname, MAGIC_MARK2, self.prefix, varname, MAGIC_MARK2), ) except Exception: - self._copy_variable("_BASHCOMP_TEST_NEWVAR_" + varname, varname) + self._copy_variable( + "%s_NEWVAR_%s" % (self.prefix, varname), varname + ) raise else: if self.noexcept and self.captured_error: self._copy_variable( - "_BASHCOMP_TEST_NEWVAR_" + varname, varname + "%s_NEWVAR_%s" % (self.prefix, varname), varname ) def _unprotect_variable(self, varname: str): if varname not in self.saved_variables: self.saved_variables[varname] = 1 - self._copy_variable(varname, "_BASHCOMP_TEST_OLDVAR_" + varname) + self._copy_variable( + varname, "%s_OLDVAR_%s" % (self.prefix, varname) + ) else: self._check_variable(varname) def _protect_variable(self, varname: str): - self._copy_variable(varname, "_BASHCOMP_TEST_NEWVAR_" + varname) + self._copy_variable(varname, "%s_NEWVAR_%s" % (self.prefix, varname)) def _restore_env(self): self.noexcept = True @@ -523,23 +533,25 @@ def _restore_env(self): # variables because "cd" affects "OLDPWD". if self.cwd_changed: self._unprotect_variable("OLDPWD") - self._safe_exec('command cd -- "$_BASHCOMP_TEST_OLDPWD"') + self._safe_exec('command cd -- "$%s_OLDPWD"' % self.prefix) self._protect_variable("OLDPWD") - self._unset_variable("_BASHCOMP_TEST_OLDPWD") + self._unset_variable("%s_OLDPWD" % self.prefix) self.cwd_changed = False for name in self.saved_shopt: self._check_shopt(name) - self._safe_exec('eval "$_BASHCOMP_TEST_OLDSHOPT_%s"' % name) - self._unset_variable("_BASHCOMP_TEST_OLDSHOPT_" + name) - self._unset_variable("_BASHCOMP_TEST_NEWSHOPT_" + name) + self._safe_exec('eval "$%s_OLDSHOPT_%s"' % (self.prefix, name)) + self._unset_variable("%s_OLDSHOPT_%s" % (self.prefix, name)) + self._unset_variable("%s_NEWSHOPT_%s" % (self.prefix, name)) self.saved_shopt = {} for varname in self.saved_variables: self._check_variable(varname) - self._copy_variable("_BASHCOMP_TEST_OLDVAR_" + varname, varname) - self._unset_variable("_BASHCOMP_TEST_OLDVAR_" + varname) - self._unset_variable("_BASHCOMP_TEST_NEWVAR_" + varname) + self._copy_variable( + "%s_OLDVAR_%s" % (self.prefix, varname), varname + ) + self._unset_variable("%s_OLDVAR_%s" % (self.prefix, varname)) + self._unset_variable("%s_NEWVAR_%s" % (self.prefix, varname)) self.saved_variables = {} self.noexcept = False @@ -602,7 +614,7 @@ def diff_env(before: List[str], after: List[str], ignore: str): if not re.search(r"^(---|\+\+\+|@@ )", x) # Ignore variables expected to change: and not re.search( - r"^[-+](_|PPID|BASH_REMATCH|_BASHCOMP_TEST_\w+)=", + r"^[-+](_|PPID|BASH_REMATCH|_BASHCOMP_TEST\w+)=", x, re.ASCII, ) From a6bb6f4b743336ae4b944f1f2bf7c24ae5d27e2b Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Mon, 31 May 2021 14:18:11 +0900 Subject: [PATCH 5/6] fix(BASH_COMPLETION_USER_FILE): explicitly check if it is not /dev/null Users often set `BASH_COMPLETION_USER_FILE=/dev/null' to explicitly express that there is no user configuration file. However, in broken systems, /dev/null may be a regular file that contains random outputs from arbitary commands whose outputs are discarded. We shall explicitly confirm that the path is not `/dev/null' before sourcing because we do not want to source this unexpected `/dev/null' file. In fact, this caused a problem in the CI testing on GitHub [1] where the command history is output to `HISTFILE=/dev/null'. Here, `/dev/null' gets the history entry `source bash_completion' and then sourced from `bash_completion' itself, which results in an infinite source chain of `bash_completion' -> `/dev/null' -> `bash_completion' -> `/dev/null' -> ... while invoking random commands from the command history. [1] https://github.com/scop/bash-completion/pull/529 --- bash_completion | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bash_completion b/bash_completion index b34db1d81a9..f4ac56c9bf0 100644 --- a/bash_completion +++ b/bash_completion @@ -2369,8 +2369,12 @@ fi unset compat_dir i _blacklist_glob # source user completion file +# +# Remark: We explicitly check that $user_completion is not '/dev/null' since +# /dev/null may be a regular file in broken systems and can contain arbitrary +# garbages of suppressed command outputs. user_completion=${BASH_COMPLETION_USER_FILE:-~/.bash_completion} -[[ ${BASH_SOURCE[0]} != "$user_completion" && -r $user_completion && -f $user_completion ]] && +[[ $user_completion != "${BASH_SOURCE[0]}" && $user_completion != /dev/null && -r $user_completion && -f $user_completion ]] && . $user_completion unset user_completion From 7e65989f49fbe764ce41e073f86fb9e2c5af028f Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Tue, 1 Jun 2021 21:46:33 +0900 Subject: [PATCH 6/6] test(conftest): prepare temporary file for HISTFILE [Problem] We have previously set HISTFILE=/dev/null to leave user's history file alone in the test, but it turned out to break the system. When the number of history entries reach HISTFILESIZE, Bash tries to replace the entity at $HISTFILE with a regular file. HISTFILE=/dev/null causes the removal of the device /dev/null and creation of a regular file at /dev/null. This Bash behavior was fixed in Bash 4.4 after the following bug-bash discussion: https://lists.gnu.org/archive/html/bug-bash/2015-01/msg00138.html [Solution] As a workaround, we prepare an empty temporary file for each test. [Remark] Another possible solution was to unset HISTFILE in test/config/bashrc. However test/config/bashrc is sourced after the first prompt is shown, i.e., after the user's history file is loaded. This doesn't necessarily cause problems, but we rather use an empty file for the history to perform tests in a unqiue condition. --- test/t/conftest.py | 57 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/test/t/conftest.py b/test/t/conftest.py index 69fa45817a9..c77cc44481b 100644 --- a/test/t/conftest.py +++ b/test/t/conftest.py @@ -186,29 +186,55 @@ def partialize( def bash(request) -> pexpect.spawn: logfile = None + histfile = None + tmpdir = None + bash = None + if os.environ.get("BASHCOMP_TEST_LOGFILE"): logfile = open(os.environ["BASHCOMP_TEST_LOGFILE"], "w") elif os.environ.get("CI"): logfile = sys.stdout + testdir = os.path.abspath( os.path.join(os.path.dirname(__file__), os.pardir) ) - env = os.environ.copy() - env.update( - dict( - SRCDIR=testdir, # TODO needed at least by bashrc - SRCDIRABS=testdir, - PS1=PS1, - INPUTRC="%s/config/inputrc" % testdir, - TERM="dumb", - LC_COLLATE="C", # to match Python's default locale unaware sort - HISTFILE="/dev/null", # to leave user's history file alone - ) + + # Create an empty temporary file for HISTFILE. + # + # To prevent the tested Bash processes from writing to the user's + # history file or any other files, we prepare an empty temporary + # file for each test. + # + # - Note that HISTFILE=/dev/null may not work. It results in the + # removal of the device /dev/null and the creation of a regular + # file at /dev/null when the number of commands reach + # HISTFILESIZE by Bash-4.3's bug. This causes the execution of + # garbages through BASH_COMPLETION_USER_FILE=/dev/null. - Note + # also that "unset -v HISTFILE" in "test/config/bashrc" was not + # adopted because "test/config/bashrc" is loaded after the + # history is read from the history file. + # + histfile = tempfile.NamedTemporaryFile( + prefix="bash-completion-test_", delete=False ) - tmpdir = None - bash = None try: + # release the file handle so that Bash can open the file. + histfile.close() + + env = os.environ.copy() + env.update( + dict( + SRCDIR=testdir, # TODO needed at least by bashrc + SRCDIRABS=testdir, + PS1=PS1, + INPUTRC="%s/config/inputrc" % testdir, + TERM="dumb", + LC_COLLATE="C", # to match Python's default locale unaware sort + HISTFILE=histfile.name, + ) + ) + marker = request.node.get_closest_marker("bashcomp") # Set up the current working directory @@ -306,6 +332,11 @@ def bash(request) -> pexpect.spawn: bash.close() if tmpdir: tmpdir.cleanup() + if histfile: + try: + os.remove(histfile.name) + except OSError: + pass if logfile and logfile != sys.stdout: logfile.close()