test: detect unexpected OLDPWD change

[akinomyoga]

From #492 (comment)

Changing OLDPWD isn't "allowed", changes in its value are just expected as tests may cd around. We could remove the special treatment for it if someone wants to land work to make that happen. pushd and popd could be an alternative.

OK. If it is not too hard, maybe I'll do that after this PR is settled.

I made changes for this one. There were three places where cd was used to change the working directory for tests. I separated them into two commits.

1. kdvi, kpdf, evince (a22afe0): Use temp_cwd (with refactoring prepare_fixture_dir -> create_dummy_filedirs for dummy files)

One place is the cd into the directory created by prepare_fixture_dir, which was used in test_{kdvi,kpdf,evince}. I changed these tests to use the temporary directory created by temp_cwd to reduce the places cd is called. Now test_{kdvi,kpdf,evince} don't need to change and restore directories by themselves. The dummy files and directories are now created by a new function create_dummy_filedirs, which is a refactored version of prepare_fixture_dir.

I thought maybe I should apply the same prefix of the temporary directories to temp_cwd as prepare_fixture_dir but found that it is already applied in macos-ci branch 41c6fb8, so skipped it this time.

I also noticed that the ordering of dummy-file creation and dummy-directory creation was changed on 328287d, so I followed that ordering in the new create_dummy_filedirs.

2. conftest (013140c): save/restore OLDPWD

pushd and popd could be an alternative.

The second place is assert_complete (conftest.py). I tried pushd and popd, but they turned out to change OLDPWD too. So I decided to just continue to use the plain cd but save and restore the value of OLDPWD in another variable.

The third place is test/t/unit/test_unit_known_hosts_real.py where one test test_no_globbing is performed in the directory test/fixtures/_known_hosts_real. I directly modified this test to save/restore OLDPWD.

[scop]

Makes sense in principle. Unfortunately I had made a bunch of changes yesterdayish and failed to push them, did that now so there are some conflicts to resolved. There are some other unrelated changes here such as changing cd to builtin cd and naive singlequoting changed to use shlex some of which are fine in principle, but not really related to this change, and thus would be better placed in another PR. (Example: https://github.com/scop/bash-completion/pull/527/files#diff-191cf488d891a8f1d47930268d94548184bfdf58c7cb898d59327e757c83be2bL497)

The detail of your explanations in PR's is great! Including the elaborations in the actual corresponding commit messages would be even better :) GitHub might be a thing of the past one day, but I don't think the project would ever change to a VCS that doesn't preserve commit history and messages.

[scop]

Seems I've broken the man test with my said changes. Doesn't reproduce locally for me though.

[akinomyoga]

Unfortunately I had made a bunch of changes yesterdayish and failed to push them, did that now so there are some conflicts to resolved. There are some other unrelated changes here such as changing cd to builtin cd and naive singlequoting changed to use shlex some of which are fine in principle, but not really related to this change, and thus would be better placed in another PR.

Thank you for the comments. I have rebased the commits, squashed one commit, dropped unrelated changes, and force-pushed. The changes to cd and shlex will be handled in other PRs.

The detail of your explanations in PR's is great! Including the elaborations in the actual corresponding commit messages would be even better :) GitHub might be a thing of the past one day, but I don't think the project would ever change to a VCS that doesn't preserve commit history and messages.

OK, I have added descriptions in the commit messages.

[akinomyoga]

Seems I've broken the man test with my said changes. Doesn't reproduce locally for me though.

bash-completion/test/t/test_man.py

Lines 104 to 115 in 72900ba

	@pytest.mark.complete(require_cmd=True)
	def test_10(self, request, bash, colonpath):
	assert_bash_exec(
	bash,
	'manpath=${MANPATH-}; export MANPATH="%s:%s/man"'
	% (TestMan.manpath, colonpath),
	)
	request.addfinalizer(
	lambda: assert_bash_exec(bash, "MANPATH=$manpath")
	)
	completion = assert_complete(bash, "man Bash::C")
	assert completion == "ompletion"

I think this is because the lifetime of request is longer than bash. The finalizer will be called after bash terminates. I guess we here want to call MANPATH=$manpath at the end of this test item (before colonpath expires), so we may perform it in the same function (after assert_complete).

I have found the error messages on the bottom. This is unrelated to the finalizer. Sorry for the noise.

Commit 18ab3b7: New functions bash_{save,restore}_variable

It seems there are many places that we want to save the variable state and later restore it, so I decided to make functions (bash_{save,restore}_variable) to save and restore shell variables in the Bash process. Now conftest.py, test_util_known_hosts_real.py, and test_man.py use these functions.

Edit: If we should separate the PR for this one too, please let me know. I'm ready to separate the PR if necessary.

[scop]

This is starting to look excellent, thanks!

In addition to the one cosmetic regex comment below, just one question: what do you think, should we turn the save/restore variable functionality to a context manager, for more succinct code, and so that restoring couldn't be forgotten, and it'd happen also on errors? I think it would be a good idea, but it doesn't necessarily have to be in this PR, we can do it in a followup one.

[akinomyoga]

what do you think, should we turn the save/restore variable functionality to a context manager, for more succinct code, and so that restoring couldn't be forgotten, and it'd happen also on errors?

I have implemented. 58f02b9

Actually, I recognized that restoration could be skipped when the error happens, but I didn't take it seriously because it only happens when there are other errors so just causes an additional failure in the later check of environment changes. But if we can properly handle them without messing up the codes, that would be nice. It is a bit big enhancement but I created a new context manager, which finally became relatively large.

The simplest context manager might have been something like this commit 2c337a8 which creates one context manager for one saved variable, but I soon gave up this simple one because it turned out to be practically not useful.

[The problem is that ...]

1. it cannot handle the saving and restoring of a variable that depends on a dynamic condition. For example,

save()
do_something()
restore()

can be easily turned into a context manager:

with var_saved():
  do_something()

but

if condition1:
    save1()

do_something()

if condition2:
    restore2()

can't be

if condition1:
    with var_saved():

do_something()

2. Also, if there are many variables, the indentation becomes as deep as the number of saved variables, which is not useful.

with var_saved1():
    with var_saved2():
        with var_saved3():
            do_something()

Instead, I decided to create one real context manager that handles multiple variables as well as the current working directory and shopt settings. Now we can write as

with bash_env_saved(bash) as bash_env:
    bash_env.write_variable(var1, value1)
    bash_env.write_variable(var2, value2) # multiple variables
    bash_env.write_env(var3, value3) # exported variables
    bash_env.chdir(path1) # change directory

    do_something()

    bash_env.chdir(path2) # It is possible to again change the directory
    bash_env.write_variable(var1, value1a) # again change the variable
    bash_env.write_variable(var4, value4)
    bash_env.shopt("failglob", True) # change shopt settings

    do_another_thing()

Remarks

• Existing codes of saving/restoring environment variables were absorbed into this new context manager.
• In the implementation, I noticed that there are already variables prefixed by _BASHCOMP_TEST_ for the save/restore purpose. So I switched from the previous prefix _bash_completion_test_ to follow _BASHCOMP_TEST_.
• Previously, we haven't been testing whether the saved variables are unmodified by the tested code, but now we can also detect the changes of saved/restored variables.
• bash_save_variable/bash_restore_variable are no longer useful compared to the new context manager so were removed.

[scop]

Great stuff, thanks a lot!

BTW I sent you mail about contributor access a couple of days ago but haven't received a reply, I wonder if you received it?

[akinomyoga]

Thank you!

BTW I sent you mail about contributor access a couple of days ago but haven't received a reply, I wonder if you received it?

Yes, I received it! Sorry for the late reply, and thank you for the invitation (though I guess it is Collaborator access in GitHub term? I'm already Contributor in the GitHub terminology.). I'm willing to continue to contribute to bash-completion, but I wanted to take time to read existing issues and PRs to judge whether I can really contribute to bash-completion as one of the collaborators.

Of course, I don't have enough time to process all of the issues and PRs because, for example, I'm not necessarily familiar with the related commands (such as mutt). Also, many issues and PRs seemed to have some plausible reason for blocking. Nevertheless, I think I can take over the modification of some PRs in which I'm interested. Or, if you specify the issues/PRs with higher priority, I think I can focus on them. Also, I may review future issues and PRs when it is relevant for me, but I think it will be less than 10% of all the issues and PRs (in particular, that are related to the completion of each command). But at least, I think I can always review the modification to the main script bash_completion. To summarize, I think my contribution would be limited to some extent. Taking account of these considerations, I'd like to leave the final decision up to you.

[scop]

Collaborator is what I meant indeed, sorry about the confusion. Invitation sent, welcome! Don't worry about it, there's no expectation to be involved with everything. And nobody else is familiar with all the commands either.

I've enabled the discussions project feature, we can talk about things not related to specific issues/PR's there. First post too: #530

[akinomyoga]

Thank you! I have accepted the invitation. Thanks also for the Discussion feature.

Maybe I can open a discussion on the design policy of this project when I have questions. For example, in #491, new variables BASH_COMPLETION_MAKE_* are added for the customization of the completion for make, but I don't see other completions use the same approach for the customization. I'm not even sure if any provides customization points. Maybe, a general design for the customization of each completion can be discussed. I also think it is good to normalize the global variable names used by each completion, such as BASH_COMPLETION_{cmdName}_{configName} or _BASHCOMP_{cmdName}_{internalDataName}.

[scop]

Please do open such a discussion. I'll refrain from replying here :)