todotxt / todo.txt-cli

☑️ A simple and extensible shell script for managing your todo.txt file.
http://todotxt.org
GNU General Public License v3.0
5.56k stars 713 forks source link

Tests failing (on Arch) #347

Closed xxxserxxx closed 3 years ago

xxxserxxx commented 3 years ago

Do you want to request a feature or report a bug? Bug

What is the current behavior?

This was first identified when the Arch AUR package for todotxt failed because of the test failures. Another user reported the same failure; I pulled this repo and got the same results: make test fails tests.

Reference: https://aur.archlinux.org/packages/todotxt/#comment-817888

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

Ensure no environment variables are set that might interfere with tests:

10083 » printenv | grep TODO
10084 » 

Start with a clean clone:

10081» git clone https://github.com/todotxt/todo.txt-cli
Cloning into 'todo.txt-cli'...
remote: Enumerating objects: 2325, done.
remote: Total 2325 (delta 0), reused 0 (delta 0), pack-reused 2325
Receiving objects: 100% (2325/2325), 5.61 MiB | 1018.00 KiB/s, done.
Resolving deltas: 100% (1399/1399), done.

Make and make test the project:

10082» cd todo.txt-cli && make && make test
VERSION=2.12.0
make: 'VERSION-FILE' is up to date.
rm -rf tests/test-results "tests/trash directory"*
cd tests && ./t0000-config.sh
* FAIL 1: no config file

    todo.sh > output 2>&1 || test_cmp expect output

*   ok 2: config file (default location 1)
*   ok 3: config file (default location 2)
*   ok 4: config file (default location 3)
* FAIL 5: config file (global config file)

    cp test.cfg "$TODOTXT_GLOBAL_CFG_FILE"
    todo.sh > output;
    test_cmp expect output && test -f used_config &&
        rm -f "$TODOTXT_GLOBAL_CFG_FILE"

*   ok 6: config file (command line)
*   ok 7: config file (env variable)
*   ok 8: config file (minimal)
* failed 2 among 8 test(s)
make: *** [Makefile:96: tests/t0000-config.sh] Error 1

What is the expected behavior?

Unit tests pass.

Which versions todo.sh are you using? N/A

Which Operating System are you using? Linux Arch 5.12.12-arch1-1

Which version of bash are you using? GNU bash, version 5.1.8(1)-release (x86_64-pc-linux-gnu)

inkarkat commented 3 years ago

Do you have XDG_CONFIG_HOME set in Arch and is there also a $XDG_CONFIG_HOME/todo/config configuration? The test driver redirects $HOME but doesn't do the same for XDG_CONFIG_HOME, so that could explain the problem. On Ubuntu and Mac OS (where the CI runs), this isn't set, so it did not get detected.

If I'm right, the tests should pass if you unset XDG_CONFIG_HOME first.

xxxserxxx commented 3 years ago

Yes, that was it. I'll close this.

I don't remember setting up ~/.config/todo/config, I must have at one point. Is having one incorrect? If it's not an invalid set-up, should the test script unset that env var before running tests if having it set causes false failures?

I'll update the AUR -- even if it isn't changed here, the PKGBUILD should be able to unset it. AFAIK, it's a common setting.

willemw12 commented 3 years ago

This can easily be fixed in Arch Linux.

But if HOME is isolated during todotxt tests (in test-lib.sh), why not do the same for XDG_CONFIG_HOME?

inkarkat commented 3 years ago

UnsettingXDG_CONFIG_HOME should have been done in the test library; that was an oversight. Apparently no other developer is using that variable together with a config, so you were the first ones to stumble over it.

It's great that you can easily fix this in Arch, but as it's a general potential problem in our test suite, I'd (also) like to fix this here, via #349. Thanks for reporting this, and for confirming that the suggested fix works!

willemw12 commented 3 years ago

The original reporter has found that more environment variables influence the unit tests. He needed to do:

unset XDG_CONFIG_HOME
unset TODO_DIR
unset TODOTXT_FINAL_FILTER
unset TODO_ACTIONS_DIR
unset TODO_NOTES_DIR
unset TODO_NOTE_TAG
unset TODOTXT_SORT_COMMAND
unset TODO_CONF_DIR
unset TODO_FILE
unset TODO_NOTE_EXT

in order to have all the make test tests pass.

inkarkat commented 3 years ago

Ah, interesting; thanks for sharing that with us!

I've tried to make the tests fail by setting various variables to intentionally bad values:

$ export TODOTXT_FINAL_FILTER='tr a-z A-Z'
$ export TODO_DIR=/dev/null
$ export TODO_FILE=/dev/null
$ export TODOTXT_SORT_COMMAND=cat
$ export TODO_ACTIONS_DIR=/dev/null

But none of those affected the tests at all! In fact, I found this code in test-lib.sh that already unsets TODOTXT_CFG_FILE, any variable starting with TODOTXT_, and TODO_FILE, DONE_FILE, REPORT_FILE, TMP_FILE. This happens at the top of the test library, and the library itself is sourced at the very beginning of each test script. So I can't reproduce or explain the behavior that the original reported has seen.

Also, variables like TODO_NOTES_DIR and TODO_NOTE_TAG clearly belong to add-ons; however, a custom TODO_ACTIONS_DIR can indeed negatively affect the tests because the built-in defaulting (which in the tests point to within the test directory) then does not happen. I was able to break the tests via a custom action dir that overrides the built-in ls command and mangles the output. I've pushed https://github.com/todotxt/todo.txt-cli/pull/349/commits/8f8a0689749074188ce086e6a8b316a5b055ca62 to address this.

Hopefully, that's all that's required to make the tests succeed for you and the original reporter as well, and all the other variables were false positives.