Taskwarrior: Tests currently failing on active development branches (2.5.2, 2.6.0)

[tbabej]

Running make test shows that there are currently couple of tests broken on the 2.6.0 branch:

Failed:                        
filter.t                            5
project.t                           1
search.t                            1
undo.t                              2

To maximize utility of the CI, we should strive to get these fixed.

[mrossinek]

I started to look into the other failing tests again. I also remembered the discussion which we had a while back over at #2097.

• [x] filter.t: will update later
• [x] project.t: this we had already discussed. We wanted to get @pbeckingham's input on what the expected syntax is for the user. We determined that task project:\"three\ four\" list works as expected in bash and zsh as it escapes the argument correctly. In order to reflect this in the test case we need to change it to be r'project:\"three\ four\" list' where we need to make it a raw string in order to ensure that Python does not interpret the backslashes itself. (Alternatively we could write \\\" but I find the raw string much more legible and clear). We should probably also document the requirements for this syntax.
• [x] search.t: this essentially boils down to the exact same problem as in the project filter above: no clarity on the syntax when dealing with spaces. If we clarify it above, we can apply the same fix here.
• [x] undo.t: After bisecting I found this commit to be the one which introduced the problem: https://github.com/GothenburgBitFactory/taskwarrior/commit/48dff115c2d237e0c94b97a868d81360358c460e. I will try to come up with a fix which doesn't revert the above.

[mrossinek]

I don't think it makes much sense to open another issue for the discussion of the tests which fail on 2.5.2 branch. So I will add them here:

% ./problems
Failed:
diag.t                              1
filter.t                            5
project.t                           3
search.t                            1
version.t                           1
wait.t                              1

• [x] diag.t: on my system it complains about not finding C++14 (instead I have C++17)
• [x] filter.t: the same issues as on 2.6.0 branch. Detailed discussion in next comment.
• [x] project.t: one overlap with 2.6.0 branch. (see comment below)
• [x] search.t: same issue as 2.6.0 branch (see previous comment)
• [x] version.t can probably be addressed by cherry-picking #2343 on to the 2.5.2 branch
• [x] wait.t: requires cherry-picking 447830d

[tbabej]

Yeah, my bad for the title being too specific. Fixed now :slightly_smiling_face:

[mrossinek]

Discussion of the filter.t failures:

% ./filter.t | grep 'not ok'
not ok 21 - filter.t: filter - description contains +
not ok 24 - filter.t: 1630: Verify that 'due.before:9d' is correctly interpreted
not ok 25 - filter.t: 1630: Verify that 'due:7d' is correctly interpreted
not ok 29 - filter.t: 1915: Make sure parser handles complex and-or queries correctly (4)
not ok 33 - filter.t: 1915: Make sure parser handles complex and-or queries correctly (3)

• [x] 21: this is a similar issue as the project.t and search.t issues discussed above: can be addressed with clarifying syntax or needs more work. The Python test can be fixed with the following syntax: self.t(r"all description.contains:\"foobar\\+\"")
• [x] 24 & 25: these refer to #1654 which I am unsure why it is closed. We may want to re-open that one and dig into it as I don't think it had been fixed before (please correct me if I'm wrong)
• [x] 29 & 33: these refer to the currently open issue #1928

[mrossinek]

Discussion of the project.t failures:

% ./project.t | grep 'not ok'
not ok 5 - project.t: 1455: Test project:school)
not ok 6 - project.t: 1511: Test project:one-two can be added and queried
not ok 7 - project.t: 1617: Verify search for multi-word project

• [x] 5: cherry-picked 517ea11
• [x] 6: cherry-picked bf2cd0f
• [x] 7: same syntax discussion as search.t and filter.t (21)

[mrossinek]

We should really discuss what solution we want to go for for the remaining failing tests. I am happy to open a new PR since #2350 has already been merged...

Also: we should address the fact that the CI keeps failing due to Python version issues. Our test suite depends on Python 3 but currently most (if not all) the docker images are configured to work with Python2. I think required changes will be:

• [x] update the docker install command to explicitly install python3 packages
• [x] search and replace /usr/bin/env python with /usr/bin/env python3 inside of the test suite
• [x] possibly further minor adaptions to fix other inconsistencies

[mrossinek]

I have tested the above on the debianstable docker image locally and I think I got it working. I also noticed that (at least for Debian) the libfaketime package needs to be replaced by the faketime package.

Before I open the PR I wanted to clarify whether to PR this on 2.5.2 or 2.6.0. This has been confusing me lately..

[tbabej]

@mrossinek thanks for looking into this!

Open the PR to update the test image Python version on 2.6.0. We need to, of course, fix the CI for the 2.5.2 branch as well, and hence either:
a) you also open a PR on 2.5.2
b) or during the time of merge, the commits will be cherry-picked on 2.5.2 manually by the person merging the PR.

[tbabej]

Down to 1 failing test on 2.5.3. Tests passing on 2.6.0.

[mrossinek]

Oddly enough this one failure is a new one...

not ok 28 - columns.t: Verify formatting of assorted long recurrence columns
# ERROR: CommandError on file /usr/lib/python3.9/unittest/case.py line 59 in testPartExecutor: 'yield':
#        Command '['/home/max/Git/taskwarrior/src/task', 'xxx', 'rc.report.xxx.columns:id,status,due,recur.duration,mask,imask,parent.long']' was SIGABRT'ed. SIGABRT usually means task timed out.
#
#        *** Start STDOUT ***
#
#        *** End STDOUT ***
#
#        *** Start STDERR ***
#
#        *** End STDERR ***

I might have time to look into it later today :+1:

[tbabej]

To hand out some pointers, it might have something to do with master retaining these extra commits that @liskin mentions here: https://github.com/GothenburgBitFactory/taskwarrior/pull/2354#issuecomment-739431607.

The 2.5.3 was branched off from master so it also contains them. Maybe just reverting them would fix the problem. 2.5.2 nor 2.6.0 contain them, which would be in line with what we observed.

[mrossinek]

@tbabej You're absolutely right! A quick bisect reveals commit 64243e6 to be the commit which caused this test to fail.
Interestingly enough, it results in a different error message:

not ok 28 - columns.t: Verify formatting of assorted long recurrence columns
# FAIL: AssertionError on file ./test/columns.t line 252 in test_recurrence_formats_long: 'self.assertRegexpMatches(out, "1\sRecurring\s+\d{4}-\d{2}-\d{2}\s+P30D\s+-")':
#       Regexp didn't match: '1\\sRecurring\\s+\\d{4}-\\d{2}-\\d{2}\\s+P30D\\s+-' not found in '1 Recurri 2020-12- P30 -
# 2 Pending 2020-12- P30             0 6b2b02ed-40b0-4b55-aa83-c61b4548f3
# '

@liskin In your description of #2354 you explain that you skipped cherry-picking this commit because:

1. most commands now use Table instead of ViewTask, so the benefit of the change is quite limited

I assume that this change was made somewhere in 2.5.2 and, therefore, is now released and part of master. Thus, I just opened a new PR #2362 which reverts #2223 and thereby fixes the last failing unittest :tada:

[liskin]

I assume that this change was made somewhere in 2.5.2 and, therefore, is now released and part of master. Thus, I just opened a new PR #2362 which reverts #2223 and thereby fixes the last failing unittest

No, that change was merged only to master, not to 2.5.2 nor 2.6.0. I don't think it's released although I haven't checked. I said multiple times that the change should either be reverted or the master branch should be nuked. Neither happened, hence this mess.

[mrossinek]

What I meant was that I assume the change from ViewTask to Table was made somewhere on 2.5.2
v2.5.2 was released yesterday and, thus, if the above is true, the use of Table is now released.

This means, we can now safely revert the commit on the 2.5.3 branch (as done in #2362). It is not necessary to do so on 2.6.0 because it has never been merged into 2.6.0.

EDIT: master now includes everything which it did previously PLUS the commits which were part of the 2.5.2 branch. The new 2.5.3 branch was then created based off the new master and is thus the minor-release development branch.

I must admit though, that I also find the branching model a little bit confusing. It would make more sense to me if 2.6.0 were actually built on top of master and bug fixes were backported from this development branch to 2.5.3 (which would become the next master at a minor version bump).

[liskin]

Oh I see, sorry! You're correct then. :-)

[tbabej]

Tests are now passing both on 2.5.3 and 2.6.0, on all tested platforms. Thanks for the great work!

[mrossinek]

Awesome! :rocket:

@tbabej What do you think about enabling required status checks to ensure that it stays this way? This will require that PRs pass the CI checks before being merged. You can read more about that here: https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/enabling-required-status-checks

[tbabej]

@mrossinek Good idea, done.