Enh/test remove ssh

[buhtz]

Abstract

Introducing a new unit (or better integration) test about removing a snapshot via SSH because there wasn't something like this before. I need this test because of the rsync incompatibility problem (#1247). It is intended to fail with a newer rsync version and to pass with an old one. I checked it with Debian testing (rsync 3.2.6) and Debian 11 stable "bullseye" (rsync 3.2.3).

Details

Target

The target of the test is the method snapshots.Snapsthots.remove(). As I explain there in the doc string the removing method seems a bit wired to me. But maybe there are good reasons for this so I didn't modified that behaviour.

But I added a check for the rsync return code. Before my PR it was ignored and had no effect when rsync failed.

Unittest

My testing class directly derive from unittest.TestCase and do not use one of the classes in generic.py. The latter isn't an elegant solution but this wasn't the reason why I avoided there use. I needed to introduce blanks in the path names to trigger the "rsync problem I need".

I collected all the code fragments from the setUp() etc methods in generic.py and build my own helper methods. I copied and refactored that code into private methods of my testing class. I'm sure we can improve that in the future and find a more generic solution to reuse that fragments for other unittests also. But it is IMHO a good start and also helped me to improve my understanding of BIT internals.

PEP8 and docstrings

Sorry I tried to resist but I did some code formating. I also added some docstrings while trying to understand some details.

[buhtz]

What is maybe important to be added to the CHANGELOG:

• snapshots.Snapshot.remove() now check the return value of rsync

I'm sure you can find a better way to phrase this. Maybe it will cause unexpected side effects in the future. So the terms "remove" and "rsync" are IMHO important here if someone search the CHANGELOG.

[buhtz]

Based on our discussion on the mailing list I modified the code. Now snapshots.Snapshot.remove() doesn't raise an exception anymore but just return a boolean to indicate success or error.

It is not a perfect solution of course but IMHO a good compromise to make my unittest testable and not breaking to much other things.

If and how the GUI should react on that return value of remove() should IMHO be the part of a separate discussion and PR. IMHO it isn't urgent yet because it wasn't checked in past also if remove() worked or not. And I am not aware of open Issue reports discussing problems in that use case.

IMHO we can merge that now so I can step further on my primary #1247 .

[aryoda]

Very helpful PR! I will do a small code "review" the next days but am not able to runtime-test it (I am not using SSH ATM) so I hope @emtiu could do this (uses SSH AFAIR)

[aryoda]

@buhtzz Could you give me please some more background (I am not into the rsync issue in detail):

I need this test because of the rsync incompatibility problem (#1247).
It is intended to fail with a newer rsync version and to pass with an old one.

If the test result depends on the installed rsync version we'd introduce a new problem (Arch Linux AUR packages for example
are executing the unit tests before make install and this PR could cause the installation to fail).

Just to be sure:

• Your plan is to implement a fix that will also make this unit test succeeding with also the new rsync version?
• Do you already have a working PoC code to test the support for the new rsync version against?
• When (circa) do you expect to send a PR for the "support new rsync version" fix?

Background of my questions:

If we merge this change than there is a certain risk to get new issues until the support for new rsync versions is
implemented and merged into master.

The shorter this time of period is the better - if it may take longer it could be better to send a single PR instead (containing this unit test + the changes to support the new rsync version).

I modified the code. Now snapshots.Snapshot.remove() doesn't raise an exception anymore
but just return a boolean to indicate success or error.

Changing the semantics of an existing method may have severe impact on existing functionality
(I have found more than 10 locations that call this method).

I can try to double-check this but I need to know:

• Which line of code did raise which exception under which condition?

If and how the GUI should react on that return value of remove() should IMHO be the part of a separate discussion and PR.
IMHO it isn't urgent yet because it wasn't checked in past also if remove() worked or not.

What does happen currently in the GUI if an exception is raised in remove?

I want to assure that the PR does not change the behavior of the GUI (eg. because the newly introduced return value is ignored for now but the thrown exception in the old code was handled and a message box shown eg. via a registered error listener)?

[buhtz]

Your questions are complex. I'm not sure I got every point.

To Arch:
Nearly every Distro does run unittests; I hope so. ;)
I suspect that Arch does install only stable versions (means "last release") of a software. So I see no problem here with Arch.

@buhtzz Could you give me please some more background (I am not into the rsync issue in detail):

Very complex. Some things are explained in the manpage or changelog. The whole picture is about how arguments are send to the remote machine via SSH. In the past you needed to escape blanks path\ with\ blank or enclose it with quotation marks "path with blank".

But the new (more modern and compared to other tools usual) behavior was introduce round about rsync 3.0.0 as optional but default with 3.2.4.

To fix this in short: We add the option -s to rsync to activate that behavior (in version before 3.2.4) and I remove quotations from SSH paths. Here is a quick & dirty example from my work in progress branch where I modified the quote= argument:
https://github.com/buhtzz/backintime/blob/3e91654f843caa1eb4c6a83126a53605bfe4e89c/common/snapshots.py#L1110-L1114

(Arch Linux AUR packages for example are executing the unit tests before make install

I hope so. But I suspect Arch does it only with stable versions of software.

Your plan is to implement a fix that will also make this unit test succeeding with also the new rsync version?

Correct.

Do you already have a working PoC code to test the support for the new rsync version against?

The unittests are the PoC. That is why I implement them.
Fixing the rsync problem is very easy and doesn't need much code changes. But I won't do this without unittests.
Beside the unittest I have my own manual-testing-protocol I go through step by step do identify situations where it make problems that are not covered by a unit or integration test. I follow that protocol on different distros.

When (circa) do you expect to send a PR for the "support new rsync version" fix?

Hard to say because it seems to me that most of the SSH situations are not covered by unittests. Currently I try to understand how backupPermissions() work to create a SSH unittest for it.

If we merge this change than there is a certain risk to get new issues until the support for new rsync versions is implemented and merged into master.

Because we only release a new stable version after the rsync thing is totally fixed I don't see that problem.
It will appear only for people using our upstream code.

It is an advantage if the tests fail in such situations and not a problem. It is a bigger problem if people do use that rsync-incompatible code without being informed about problems it cause.

The shorter this time of period is the better

Absolutely. 🗡️

• if it may take longer it could be better to send a single PR instead (containing this unit test + the changes to support the new rsync version).

I also thought about it. But because of a lot of missing unittests it will be impossible to review.

Again: The new introduced unittests (there come more) don't hurt because they exist only in upstream/unstable/pre-release version of BIT. If there really is a distro out there shipping BIT from master then they will be informed that something isn't OK.

I modified the code. Now snapshots.Snapshot.remove() doesn't raise an exception anymore
but just return a boolean to indicate success or error.

Changing the semantics of an existing method may have severe impact on existing functionality (I have found more than 10 locations that call this method).

I haven't change the semantic but introduced one. The method was silent before.

What does happen currently in the GUI if an exception is raised in remove?

The method didn't raised something or returned something before. It was silent. It only produced a WARNING in the logging output when the rsync call failed. And the latter was ignored and never checked by the whole application.

I want to assure that the PR does not change the behavior of the GUI

It does not.

because the newly introduced return value is ignored for now but the thrown exception in the old code was handled

There was no exception in the "old code". There was nothing in the old code. See the current master branch

    def remove(self, sid):
        """
        Remove snapshot ``sid``.

        Args:
            sid (SID):              snapshot to remove
        """
        if isinstance(sid, RootSnapshot):
            return
        rsync = tools.rsyncRemove(self.config)
        with TemporaryDirectory() as d:
            rsync.append(d + os.sep)
            rsync.append(self.rsyncRemotePath(sid.path(use_mode = ['ssh', 'ssh_encfs'])))
            tools.Execute(rsync).run()
            shutil.rmtree(sid.path())

I only found that problem because of my manual-testing-protocol and checking the terminal output.

[aryoda]

@buhtzz THX for clarifying my questions, I think I do now understand intentions.

But I suspect Arch does it only with stable versions of software.

not necessarily, they are quite eager to use new versions from Git (even not released) and they are already using
rsync v3.2.6 but we can ignore this issue for now I think since I have documented a work-around for the installation issue in my other PR #1335) and the work-around for rsync is also documented in the README here.

Because we only release a new stable version after the rsync thing is totally fixed I don't see that problem.

Agreed, master must compile and tests must succeed in standard DEV distros and is treated as "unstable" until the
next release in all other distros.

Fixing the rsync problem is very easy and doesn't need much code changes. But I won't do this without unittests.

OK, so the fix is clear but the the main efforts are implementing missing unittests to avoid introducing another bug.
Sounds like a bottleneck.

I haven't change the semantic but introduced one. The method was silent before.
Yes, it was absolutely silent before.

---

If have done a little review of the code and merged it into a local branch (result: builds and unit tests succeed).

I just have added review questions to four code locations that were unclear to me, could you please check this (esp. if the print statements are really required).

THX for improving the documentation and code coverage!

[buhtz]

Dear Jürgen,
thank you very much for review. In my perception, we're a pretty good team especially with reviews. It contributes to my peaceful sleep if one of you reviewing my code.

Sorry for the debug-prints. I need to be more diligent here! No 🐝 -stamp for this.

My intention was hard to explain in the first place because test driven development is so natural to me as breathing. In the consequence I often miss the correct words to explain it. The BIT project is a bit different but in other (more fresh) projects I often write the test first and implement the feature after that.

And all this will become more easier to maintain with full access to the repo and deciding about a branching model.

EDIT

I just have added review questions to four code locations that were unclear to me

I couldn't find review comments in the files. Mayb I did something wrong with the GitHub frontend?

[buhtz]

No pushing. Just a sidenote. 😄

I have a follow-up PR in the back. Beside what it adds (a new test case) it reorganize and clears up a bit the private methods (starting with _) in TestSshRemoveSnapshots which you can see in this PR here. So don't be too hard with me if you find that methods a bit chaotic now. 🌪️ They are improved in the next PR.

[aryoda]

I couldn't find review comments in the files. Maybe I did something wrong with the GitHub frontend?

I can see my review comments if I scroll up here in the PR, if they don't appear in your front-end this would be a bug of Github.

I have a follow-up PR in the back. Beside what it adds (a new test case) it reorganize and clears up a bit the private methods (starting with _) in TestSshRemoveSnapshots which you can see in this PR here.
So don't be too hard with me if you find that methods a bit chaotic now. tornado They are improved in the next PR.

My review comments are not blocking this PR, I think it helps me to understand non-obvious parts and gives us the chance to reconsider implementations.

BTW: If you want you could add the commits of the "follow-up PR" to this PR (if they belong together)

[buhtz]

I can see my review comments if I scroll up here in the PR, if they don't appear in your front-end this would be a bug of Github.

Mhm... Did you mean in the "Files canged" tab? I suspect the problem sits in front of my own monitor but I can't see any comment there. 😃 Can you name file and a line number as an example please?

BTW: If you want you could add the commits of the "follow-up PR" to this PR (if they belong together)

A bit to much at the current point. There are also other PRs open.