1.3.17: CompletionTest fails

[Profpatsch]

======================================================================
FAIL: test_completion (test.test_ui.CompletionTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/nix-build-beets-1.3.17.drv-0/beets-v1.3.17-src/test/test_ui.py", line 1130, in test_completion
    self.fail('test/test_completion.sh did not execute properly')
AssertionError: test/test_completion.sh did not execute properly

----------------------------------------------------------------------

A not-so-very-helpful error, I don’t know how to debug this.

[sampsyo]

Hmm, I can't seem to reproduce this on any of my machines. Does this still happen in the latest source?

[Profpatsch]

Yes. How do I debug it?

[sampsyo]

Maybe try adding some print statements to the test to check exactly where it goes wrong?

[Profpatsch]

I traced it down to:

+ completes .git beets beetsplug docs extra test build beets.egg-info dist
+ for word in '"$@"'
+ [[    == *[[:space:]].git[[:space:]]* ]]
+ return 1
+ fail=1
+ echo 'test_fields_command failed'

test_fields_command at line 96 (the second completes call) and completes at line 12; I’m at a loss at how COMPREPLY[@] works, but it is kind of empty.

Compare the output of the first completes call:

+ completes -h --help
+ for word in '"$@"'
+ [[  -h --help  == *[[:space:]]-h[[:space:]]* ]]
+ for word in '"$@"'
+ [[  -h --help  == *[[:space:]]--help[[:space:]]* ]]

[sampsyo]

Huh! Thanks for investigating -- I'm so unfamiliar with bash-completion that I can't really interpret what's going wrong here. (I also don't know why it's working on my setup...)

Can somebody who knows more about the system take a look? @geigerzaehler, perhaps?

[aszlig]

Spoiler: We're both on Nix(OS), so the failures are not only reproducible, but I've also disabled the tests in the past (NixOS/nixpkgs@2acc258dff1a37974edd6475851e218bb09e281a) because I didn't have time to find out why they fail.

The failure reappeared because 903e88a228d6bd93bd1884c59dd23dd9f04a1199 was moving the shell script responsible for testing the bash completion to another location.

We have disabled the completion tests using echo echo completion tests passed > test/test_completion.sh but unfortunately the update to Beets 1.3.17 in NixOS/nixpkgs@22818c6ec4a77b640b15050dd48a1aef244075f7 re-enabled the tests, because the overwrite of test/test_completion.sh suddenly doesn't anything useful anymore.

For now I've simply changed that to use the new path (NixOS/nixpkgs@b6595185f6505e198ae4270e92e1ff86c34a2a53), but I've also dug through the gory details about why this happens:

First of all, the following are the completion tests that fail:

• test_fields_command
• test_global_dir_opts
• test_global_file_opts
• test_import_files

Now what all of these tests have in common is the following:

initcli ... && completes $(compgen -d)

The compgen -d here lists directory (-d) completions for a given word. However there is no such word given, so it is roughly equivalent to returning a list of directories in the current working directory.

In <nixpkgs> we run test suite while the current working directory is the source tree of beets, so the directories printed by compgen -d are beets.egg-info, beets, test, beetsplug, extra, docs, build and dist.

So looking at the completes function, it checks whether the output of the completion matches that of the given words (in all of these cases the output of compgen -d).

Once compgen -d doesn't reply with anything (if the current working directory is empty for example) it essentially turns the completes function into a dummy which always returns successfully because there are no words to check for.

So while this may have happened on Nix(OS) only, it really is an upstream issue and instead of testing against compgen -d, we should really check whether for example the right fields are returned.

For example the test_field_commands test gets the completion words for beet fields, which in turn should be something like this:

lyrics album_id albumstatus disctitle year month channels genre original_day disc mb_trackid composer mtime albumdisambig samplerate albumartist_sort id album mb_artistid bitdepth disctotal title media artist_sort mb_albumid comments acoustid_fingerprint rg_album_gain script mb_releasegroupid acoustid_id rg_album_peak albumartist_credit catalognum added original_month format track comp artpath encoder initial_key rg_track_gain path bitrate day original_year tracktotal language artist asin mb_albumartistid bpm label length albumartist albumtype artist_credit country rg_track_peak grouping

Of course this doesn't necessarily match the directories of the current working directory :wink: and of course only works if the current working directory contains no subdirectories.

Summary: The test_completion.sh shouldn't use compgen -d but check for a fixed list of words that are guaranteed to exist for a particular subcommand/flag.

Disclaimer: I didn't look into the bash sources whether compgen -d does something magical I didn't account for here, but if it does I'd like to know.

[sampsyo]

Wow! Thanks for doing all the digging; this definitely does sound like a mistake. In fact, this is one of those situations where I can't really see why this ever worked.

I'm not the right person to fix this confidently, so I'd love some help from anyone more familiar with the completion infrastructure. And BTW, here's hoping we can get rid of this custom completion stuff once we switch to click... c.f. #1484.

[stefanor]

Had a brief look into this, and I think it's something more subtle. I'd never run into it before, because it requires bash-completion to be installed, and I never had that in the Debian build environment.

I can reproduce it with tox on Debian unstable: tox -e test -- -k test_completion. Not sure why it isn't showing up on the CI, maybe a new bash version broke the completions?

With debugging prints:

COMPREPLY: list
@: list
COMPREPLY: list
@: ls
COMPREPLY: import
@: import
COMPREPLY: fields help import list update remove stats version modify move write config completion test plugin
@: fields import list update remove stats version modify move write help
COMPREPLY: fields help import list update remove stats version modify move write config completion test plugin
@: fields import list update remove stats version modify move write help
COMPREPLY: fields help import list update remove stats version modify move write config completion test plugin
@: fields import list update remove stats version modify move write help
COMPREPLY: fields help import list update remove stats version modify move write config completion test plugin
@: fields import list update remove stats version modify move write help
COMPREPLY: fields help import list update remove stats version modify move write config completion test plugin
@: fields import list update remove stats version modify move write help
COMPREPLY: -h --help
@: -h --help
COMPREPLY: 
@: beets beetsplug test
test_fields_command failed
COMPREPLY: beets beetsplug test
@: beets beetsplug test
COMPREPLY: beets beetsplug test
@: beets beetsplug test
COMPREPLY: 
@: beets beetsplug test
test_global_file_opts failed
COMPREPLY: -l --library -c --config -d --directory -h --help -v --verbose
@: -l --library -d --directory -h --help -c --config -v --verbose
COMPREPLY: fields help import list update remove stats version modify move write config completion test plugin
@: fields import list update remove stats version modify move write help
COMPREPLY: 
@: beets beetsplug test
test_import_files failed
COMPREPLY: -h --help -l --log -S --search-id --set -c --copy -C --nocopy -m --move -w --write -W --nowrite -a --autotag -A --noautotag -p --resume -P --noresume -q --quiet -s --singletons -t --timid -L --library -i --incremental -I --noincremental --from-scratch --flat -g --group-albums --pretend
@: -h --help -c --copy -C --nocopy -w --write -W --nowrite -a --autotag -A --noautotag -p --resume -P --noresume -l --log --flat
COMPREPLY: -h --help -p --path -f --format -a --album
@: -h --help -a --album -p --path
COMPREPLY: artist_credit: artpath: artist: artist_sort:
@: artist: artpath:
COMPREPLY: test
@: test
COMPREPLY: -h --help -o --option
@: -o --option