test(man): fix skip failure of `test_10` #529

akinomyoga · 2021-05-29T12:37:08Z

This fixes the test failure of test_man.py. @pytest.mark.complete(require_cmd=True) is referenced only when the fixture completion is requested. Since completion is not requested in the new version of test_10, require_cmd=True is inactive now. This PR explicitly checks the command (non-)existence to properly skip the test.

akinomyoga · 2021-05-31T02:19:38Z

The failure of test_man.py is solved by commit ~~489fc72~~ 71592a8 (rebased), but this revealed another problem in distcheck (ubuntu14). It is only reproduced in distcheck (ubuntu14). There is no problem in the other environments including my local one. I tried to fix the problem, but I'm still not sure what is happening there.

The previous code using bash_{save,resore}_variable passes the test, but the new code bash_env_saved fails if the detection of unwanted OLDPWD changes is turned on, which means that something is wrong in testing. Even though I can make it pass by turning off the detection, it is not the true solution. Anyway, I add changes to make bash_env_saved more robust though they don't seem to solve the problem.

[Bug fix] Correctly restore `cwd` `7f1d8cb` `5264bd1` (rebased)

In the previous version of bash_env_saved, I used bash.cwd to get 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. I instead change to store the real current working directory in a shell variable inside the Bash process.

Try to proceed the restoration process on partial restoration failure `fb41aec` `ece9e79` (rebased)

Although the entire test anyway fails If one of the restoration operations fails, bash_env_saved now tries to perform the remaining restoration process to reduce unnecessary errors caused by the restoration failure.

Support nested call of `bash_env_saved` `ec91250` `4d5a48d` (rebased)

In the previous version of bash_env_saved, when the same variables are saved/restored in nested with bash_env_saved(), the variables are not correctly restored. I'm not sure whether there are real instances of such cases in the testing, but bash_env_saved now supports the nested call of with bash_env_saved() in this commit.

… /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] scop#529

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] scop#529

akinomyoga · 2021-05-31T06:22:24Z

I found that `/dev/null` is a regular file in distcheck (ubuntu14)!

Since we have set HISTFILE=/dev/null,

bash-completion/test/t/conftest.py

Line 205 in 7e0803f

HISTFILE="/dev/null", # to leave user's history file alone

the command history including the history entry "source .../bash_completion" are saved in the file /dev/null. Then, bash_completion tries to source BASH_COMPLETION_USER_FILE=/dev/null (set up in test/config/bashrc).

bash-completion/test/config/bashrc

Line 31 in 7e0803f

export BASH_COMPLETION_USER_FILE=/dev/null

bash-completion/bash_completion

Lines 2371 to 2374 in 7e0803f

    
           # source user completion file 
        
           user_completion=${BASH_COMPLETION_USER_FILE:-~/.bash_completion} 
        
           [[ ${BASH_SOURCE[0]} != "$user_completion" && -r $user_completion && -f $user_completion ]] && 
        
               . $user_completion

This causes an infinite source chain of bash_completion -> File /dev/null -> bash_completion -> File /dev/null -> ...

I have added fixes to this problem 492150b. I have also rebased the commits and added the corresponding description to each commit log. Now it's ready to be reviewed and merged.

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] scop#529

bash_completion

akinomyoga · 2021-06-01T03:21:05Z

I had thought that the Docker image was already broken, but looking at the test log, I guess that /dev/null is broken in the middle of the testing:

2021-05-31T02:41:03.5296710Z ../../../test/t/unit/test_unit_filedir.py::TestUnitFiledir::test_25[f] PASSED [ 89%]
2021-05-31T02:41:04.3482533Z ../../../test/t/unit/test_unit_filedir.py::TestUnitFiledir::test_25[f2] PASSED [ 89%]
2021-05-31T02:41:05.1673845Z ../../../test/t/unit/test_unit_filedir.py::TestUnitFiledir::test_26[f] PASSED [ 89%]
2021-05-31T02:41:05.9883034Z ../../../test/t/unit/test_unit_filedir.py::TestUnitFiledir::test_26[f2] PASSED [ 89%]
2021-05-31T02:41:05.9901600Z ../../../test/t/unit/test_unit_filedir.py::TestUnitFiledir::test_27[f] SKIPPED [ 89%]
2021-05-31T02:41:06.2023157Z ../../../test/t/unit/test_unit_filedir.py::TestUnitFiledir::test_27[f2] SKIPPED [ 89%]
2021-05-31T02:41:06.2043100Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_1 PASSED [ 89%]
2021-05-31T02:41:06.2062051Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_2 PASSED [ 89%]
2021-05-31T02:41:06.2079446Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_3 PASSED [ 89%]
2021-05-31T02:41:06.2097433Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_4 PASSED [ 89%]
2021-05-31T02:41:06.2115222Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_5 PASSED [ 89%]
2021-05-31T02:41:06.2133984Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_6 PASSED [ 89%]
2021-05-31T02:41:06.2153268Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_7 PASSED [ 89%]
2021-05-31T02:41:06.2171814Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_8 PASSED [ 89%]
2021-05-31T02:41:06.2195394Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_9 PASSED [ 89%]
2021-05-31T02:41:06.2230009Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_10 PASSED [ 89%]
2021-05-31T02:41:06.2242000Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_11 PASSED [ 90%]
2021-05-31T02:41:06.2258583Z ../../../test/t/unit/test_unit_find_unique_completion_pair.py::TestUnitFindUniqueCompletionPair::test_12 PASSED [ 90%]
2021-05-31T02:41:07.2114565Z ../../../test/t/unit/test_unit_get_comp_words_by_ref.py::TestUnitGetCompWordsByRef::test_1 ERROR [ 90%]
2021-05-31T02:41:07.2443361Z ../../../test/t/unit/test_unit_get_comp_words_by_ref.py::TestUnitGetCompWordsByRef::test_2 ERROR [ 90%]
2021-05-31T02:41:07.2767841Z ../../../test/t/unit/test_unit_get_comp_words_by_ref.py::TestUnitGetCompWordsByRef::test_3 ERROR [ 90%]
2021-05-31T02:41:07.3092224Z ../../../test/t/unit/test_unit_get_comp_words_by_ref.py::TestUnitGetCompWordsByRef::test_4 ERROR [ 90%]
2021-05-31T02:41:07.3411014Z ../../../test/t/unit/test_unit_get_comp_words_by_ref.py::TestUnitGetCompWordsByRef::test_5 ERROR [ 90%]
2021-05-31T02:41:07.3729791Z ../../../test/t/unit/test_unit_get_comp_words_by_ref.py::TestUnitGetCompWordsByRef::test_6 ERROR [ 90%]

The error starts to happen from unit/test_unit_get_comp_words_by_ref.py and all the later tests fail. The test immediately preceding it is unit/test_unit_find_unique_completion_pair.py, but this is unrelated because it only tests conftest's utility find_unique_completion_pair. The culprit seems to be the second previous one unit/test_unit_filedir.py. In fact, the log includes error messages like bash: cd: /tmp/bash-completion_filedirx_kom9mv: No such file or directory which is the directory created by unit/test_unit_filedir.py. Probably, some test in unit/test_unit_filedir.py removes the device /dev/null and then a new regular file of the same name is created on the termination of the Bash process. I'm not sure what causes the removal of /dev/null.

scop · 2021-06-01T04:22:44Z

I'm not sure what causes the removal of /dev/null.

Huh, if it's something triggered by us, then that's something we need to fix. Maybe that old bash removes $HISTFILE in some circumstances.

It would seem safer to unset HISTFILE after shell startup instead of setting it to /dev/null, e.g. test/config/bashrc seems to be an appropriate place.

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.

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.

`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.

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] scop#529

akinomyoga · 2021-06-01T13:17:21Z

I found that it was actually a bug (or a quirk) of Bash 4.3 and lower. I found the following report in bug-bash:

If $HISTFILE is set to /dev/null and you execute more commands than $HISTFILESIZE, /dev/null is deleted.

This means that the number of commands in the history hits the limit $HISTFILESIZE in unit/test_unit_filedir.py. I previously mentioned that dropping the detection of the OLDPWD change made the test pass, but now I understand that it was just because the number of executed commands in the test was reduced. So there is nothing wrong in our test code.

It would seem safer to unset HISTFILE after shell startup instead of setting it to /dev/null, e.g. test/config/bashrc seems to be an appropriate place.

I have rebased the commits on the latest master and added the mentioned workaround 1dbe515. In case HISTFILE is accessed before unset -v HISTFILE in test/config/bashrc is executed, I set HISTFILE=/tmp/bash_completion.bash_history as a trasient value.

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.

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] scop#529

test/t/conftest.py

bash_completion

test/t/conftest.py

[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.

akinomyoga · 2021-06-06T05:45:46Z

@scop Thank you for the patient review on my PR and the related discussions!

Can I delete these two GitHub Workflow actions, #223 and #225? They have gigantic sizes of output log caused by the infinite source chain of bash_completion -> /dev/null -> bash_completion -> /dev/null -> ..., so I'd like to delete them. They have raw sizes of about 4.0GB and 3.4GB (or compressed sizes 500MB and 400MB), respectively. The other actions have "finite" sizes of logs because of test_man failure fortunately or unfortunately.

scop · 2021-06-06T06:00:03Z

No problem, thank you!

By all means, go and delete them, nice catch.

akinomyoga · 2021-06-06T06:11:09Z

Thanks! I've deleted them.

akinomyoga changed the title ~~test(man): fix skip failure of test_10~~ [WIP] test(man): fix skip failure of test_10 May 31, 2021

akinomyoga force-pushed the fix-test_man branch from ec91250 to 492150b Compare May 31, 2021 06:14

akinomyoga changed the title ~~[WIP] test(man): fix skip failure of test_10~~ test(man): fix skip failure of test_10 May 31, 2021

akinomyoga force-pushed the fix-test_man branch from 492150b to 8800658 Compare May 31, 2021 12:14

scop reviewed May 31, 2021

View reviewed changes

bash_completion Show resolved Hide resolved

akinomyoga added 3 commits June 1, 2021 21:36

test(bash_env_saved): continue restoring on restoring error

f469727

`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.

akinomyoga force-pushed the fix-test_man branch from 8800658 to 1dbe515 Compare June 1, 2021 13:12

akinomyoga added 2 commits June 1, 2021 22:55

akinomyoga force-pushed the fix-test_man branch from 1dbe515 to 68bec50 Compare June 1, 2021 13:55

scop requested changes Jun 1, 2021

View reviewed changes

test/t/conftest.py Outdated Show resolved Hide resolved

bash_completion Show resolved Hide resolved

akinomyoga force-pushed the fix-test_man branch from 68bec50 to 60c5543 Compare June 2, 2021 01:40

scop requested changes Jun 2, 2021

View reviewed changes

test/t/conftest.py Outdated Show resolved Hide resolved

test/t/conftest.py Outdated Show resolved Hide resolved

akinomyoga force-pushed the fix-test_man branch 2 times, most recently from f507e2a to ce3583f Compare June 2, 2021 07:05

scop requested changes Jun 4, 2021

View reviewed changes

test/t/conftest.py Show resolved Hide resolved

akinomyoga force-pushed the fix-test_man branch from ce3583f to 0ee13b2 Compare June 4, 2021 23:22

akinomyoga force-pushed the fix-test_man branch from 0ee13b2 to 7e65989 Compare June 5, 2021 00:38

scop approved these changes Jun 6, 2021

View reviewed changes

scop merged commit d1c13da into scop:master Jun 6, 2021

akinomyoga deleted the fix-test_man branch June 6, 2021 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(man): fix skip failure of `test_10` #529

test(man): fix skip failure of `test_10` #529

akinomyoga commented May 29, 2021

akinomyoga commented May 31, 2021 •

edited

Loading

akinomyoga commented May 31, 2021 •

edited

Loading

akinomyoga commented Jun 1, 2021

scop commented Jun 1, 2021

akinomyoga commented Jun 1, 2021

akinomyoga commented Jun 6, 2021

scop commented Jun 6, 2021

akinomyoga commented Jun 6, 2021

test(man): fix skip failure of test_10 #529

test(man): fix skip failure of test_10 #529

Conversation

akinomyoga commented May 29, 2021

akinomyoga commented May 31, 2021 • edited Loading

[Bug fix] Correctly restore cwd 7f1d8cb 5264bd1 (rebased)

Try to proceed the restoration process on partial restoration failure fb41aec ece9e79 (rebased)

Support nested call of bash_env_saved ec91250 4d5a48d (rebased)

akinomyoga commented May 31, 2021 • edited Loading

I found that /dev/null is a regular file in distcheck (ubuntu14)!

akinomyoga commented Jun 1, 2021

scop commented Jun 1, 2021

akinomyoga commented Jun 1, 2021

akinomyoga commented Jun 6, 2021

scop commented Jun 6, 2021

akinomyoga commented Jun 6, 2021

test(man): fix skip failure of `test_10` #529

test(man): fix skip failure of `test_10` #529

akinomyoga commented May 31, 2021 •

edited

Loading

[Bug fix] Correctly restore `cwd` `7f1d8cb` `5264bd1` (rebased)

Try to proceed the restoration process on partial restoration failure `fb41aec` `ece9e79` (rebased)

Support nested call of `bash_env_saved` `ec91250` `4d5a48d` (rebased)

akinomyoga commented May 31, 2021 •

edited

Loading

I found that `/dev/null` is a regular file in distcheck (ubuntu14)!