Skip sshfs-related unit tests if sshfs is not installed

[aryoda]

There is an ARCH/AUR comment proposing this:

I was unable to get 1.4.0-5 to build. Checking the install logs, it
looks like it failed because I didn't have sshfs installed when it
got to running the test code to verify it built correctly. Installing
sshfs first allowed me to install 1.4.0-5 without issues. I don't
know if it'd be better to not require that set of tests

https://aur.archlinux.org/pkgbase/backintime/

[buhtz]

I'm not convinced. Skipping is IMHO never a good solution.
sshfs is just a dependency for testing. Thats it.

I checked https://github.com/bit-team/backintime/blob/dev/CONTRIBUTING.md#build--install

This could be modified because there is no section for "test dependencies". Not sure if "build dependencies" count for that?
We should add all "test dependencies" in the section "build dependencies" or create an extra "test dependencies" section.

Currently "sshfs" is just a recommendation in context of runtime dependencies.

In the following I reorganized the section in our CONTRIBUTION.md and added qttranslation to it.

Quick n dirty proposal

• Runtime dependencies for the CLI

  • python3 (>= 3.8)
  • rsync
  • cron-daemon
  • openssh-client
  • python3-keyring
  • python3-dbus
  • python3-packaging
  • Recommended
    • sshfs
    • encfs

• Runtime dependencies for the GUI

  • x11-utils
  • python3-pyqt5
  • python3-dbus.mainloop.pyqt5
  • libnotify-bin
  • policykit-1
  • qttranslations5-l10n
  • qtwayland5 (if Wayland is used as display server instead of X11)
  • Recommended
    • For SSH key storage one of these packages
      • python3-secretstorage
      • python3-keyring-kwallet
      • python3-gnomekeyring
    • For diff-like comparing files between backup snapshots one of these
      packages
      • kompare
      • or meld
    • Optional: Default icons
      • The oxygen icons should be offered as optional dependency
        since they are used as fallback in case of missing icons
        (mainly app and system-tray icons)

• Build and testing dependencies

  • All runtime dependencies for CLI and GUI including the recommended
  • build-essential
  • gzip
  • gettext
  • python3-pyfakefs

[aryoda]

I'm not convinced. Skipping is IMHO never a good solution.

Yes, I agree, that it is better to test everything.

Currently we are inconsistent here since the ssh-related tests are also skipped if no ssh server is available

sshfs is just a dependency for testing.

This does work for all "test-during-packaging" scenarios.
ARCH is different here "by design" (every installation does build from source + execute the unit tests before)
and installing test-only dependencies + removing them after testing (if the deps were not already installed by the user before) seems complicated.

We should add all "test dependencies" in the section "build dependencies" or create an extra "test dependencies" section.

Yes, this is a good starting point to make these deps visible.

[buhtz]

the ssh-related tests are also skipped if no ssh server is available

Yeah, I don't like that either. Why not (after the next release) remove the skipping from them. See no reason why not.

ARCH is different here "by design"

Then they should join the discussion and tell us how we can solve this meeting the needs of upstream (us) and arch.
@graysky2 Can you help us here?

[graysky2]

The Arch Way is to follow upstream's recommendations and defaults. If I am missing build deps or check deps let me know and I will fix that.

EDIT: I will say that building 1.4.0-5 in a clean buildroot (the recommended way of building) for me does not error out.

==> Starting prepare()...
patching file .travis.yml
patching file CHANGES
patching file VERSION
patching file common/config.py
patching file common/doc-dev/conf.py
patching file common/man/C/backintime-askpass.1
patching file common/man/C/backintime-config.1
patching file common/man/C/backintime.1
patching file debian/changelog
patching file qt/man/C/backintime-qt.1
patching file CHANGES
patching file qt/app.py
==> Starting build()...
Unknown Arguments:  --no-fuse-group
All OK. Now run:
    make
    sudo make install
msgfmt -o po/zh_TW.mo po/zh_TW.po
msgfmt -o po/zh_CN.mo po/zh_CN.po
msgfmt -o po/vi.mo po/vi.po
msgfmt -o po/uk.mo po/uk.po
msgfmt -o po/tr.mo po/tr.po
msgfmt -o po/th.mo po/th.po
msgfmt -o po/sv.mo po/sv.po
msgfmt -o po/sr.mo po/sr.po
msgfmt -o po/sl.mo po/sl.po
msgfmt -o po/sk.mo po/sk.po
msgfmt -o po/ru.mo po/ru.po
msgfmt -o po/ro.mo po/ro.po
msgfmt -o po/pt_BR.mo po/pt_BR.po
msgfmt -o po/pt.mo po/pt.po
msgfmt -o po/pl.mo po/pl.po
msgfmt -o po/nn.mo po/nn.po
msgfmt -o po/nl.mo po/nl.po
msgfmt -o po/nb.mo po/nb.po
msgfmt -o po/lt.mo po/lt.po
msgfmt -o po/ko.mo po/ko.po
msgfmt -o po/ja.mo po/ja.po
msgfmt -o po/it.mo po/it.po
msgfmt -o po/is.mo po/is.po
msgfmt -o po/id.mo po/id.po
msgfmt -o po/hu.mo po/hu.po
msgfmt -o po/hr.mo po/hr.po
msgfmt -o po/he.mo po/he.po
msgfmt -o po/gl.mo po/gl.po
msgfmt -o po/fr.mo po/fr.po
msgfmt -o po/fo.mo po/fo.po
msgfmt -o po/fi.mo po/fi.po
msgfmt -o po/fa.mo po/fa.po
msgfmt -o po/eu.mo po/eu.po
msgfmt -o po/et.mo po/et.po
msgfmt -o po/es.mo po/es.po
msgfmt -o po/eo.mo po/eo.po
msgfmt -o po/el.mo po/el.po
msgfmt -o po/de.mo po/de.po
msgfmt -o po/da.mo po/da.po
msgfmt -o po/cs.mo po/cs.po
msgfmt -o po/ca.mo po/ca.po
msgfmt -o po/bs.mo po/bs.po
msgfmt -o po/bg.mo po/bg.po
msgfmt -o po/ar.mo po/ar.po
#man pages
for i in $(ls -1 man/C/); do case $i in *.gz|*~) continue;; *) gzip -n --best -c man/C/$i > man/C/${i}.gz;; esac; done
#config-examples
gzip -n --best -c config-example-local > config-example-local.gz
gzip -n --best -c config-example-ssh > config-example-ssh.gz
All OK. Now run:
    make
    sudo make install
#man pages
for i in $(ls -1 man/C/); do case $i in *.gz|*~) continue;; *) gzip -n --best -c man/C/$i > man/C/${i}.gz;; esac; done
==> Starting check()...
/usr/bin/pytest 
============================= test session starts ==============================
platform linux -- Python 3.11.5, pytest-7.4.2, pluggy-1.3.0
rootdir: /build/backintime/src/backintime-1.4.0/common
plugins: pyfakefs-5.2.4
collected 359 items

test/test_applicationinstance.py ...................                     [  5%]
test/test_argparser.py .....................                             [ 11%]
test/test_backintime.py ..s                                              [ 11%]
test/test_backup.py ...............                                      [ 16%]
test/test_config.py ...........                                          [ 19%]
test/test_configfile.py ................................................ [ 32%]
......                                                                   [ 34%]
test/test_diagnostics.py .....                                           [ 35%]
test/test_encfstools.py .                                                [ 35%]
test/test_restore.py .........ss                                         [ 38%]
test/test_sid.py ...........................................             [ 50%]
test/test_snapshotlog.py ..........                                      [ 53%]
test/test_snapshots.py .......................................s......... [ 67%]
.ssss                                                                    [ 68%]
test/test_sshtools.py sssssssssssssssssssssss...ssssss                   [ 77%]
test/test_takeSnapshot.py .........sssssssss                             [ 82%]
test/test_tools.py ..........s.........................s.......s........ [ 97%]
.........                                                                [100%]Clearing the cache


======================= 310 passed, 49 skipped in 10.20s =======================

Is the issue that my build skipped 49 tests due to missing sshfs?

[buhtz]

Thanks for that output. Interesting.

Most of the 49 tests should be related to a missing SSH server run. They are not about missing sshfs.

If there is no SSH server running some tests are skipped. I would suggest to make an SSH server run. No further configuration is needed. The tests will generate and manage a key pair for that.

When you run the SSH tests I assume that then you will get some errors about missing "sshfs".

In the far away future I do plan (#1489) to separate the three tests types unit, integration and system tests. In that case distro maintainers can run them one after the other. Btw: Debian don't run tests on our package because our tests write to $HOME without using a fake-filesystem.

[graysky2]

I would suggest to make an SSH server run.

This is unprecedented as far as I know. A build root is created based on the depends, the package is then built in that build root. On Arch, the default state of openssh is not having the server's service enabled/started. So sshd is installed in the build root but not active.

As a test, I built the package on my native system (no build root). The native system has openssh installed and sshd running but not sshfs.

==> Starting check()...
/usr/bin/pytest 
===================================================== test session starts ======================================================
platform linux -- Python 3.11.5, pytest-7.4.2, pluggy-1.3.0
rootdir: /scratch/backintime/src/backintime-1.4.0/common
plugins: pyfakefs-5.2.4
collected 359 items                                                                                                            

test/test_applicationinstance.py ...................                                                                     [  5%]
test/test_argparser.py .....................                                                                             [ 11%]
test/test_backintime.py ..s                                                                                              [ 11%]
test/test_backup.py ...............                                                                                      [ 16%]
test/test_config.py ...........                                                                                          [ 19%]
test/test_configfile.py ......................................................                                           [ 34%]
test/test_diagnostics.py .....                                                                                           [ 35%]
test/test_encfstools.py .                                                                                                [ 35%]
test/test_restore.py .........ss                                                                                         [ 38%]
test/test_sid.py ...........................................                                                             [ 50%]
test/test_snapshotlog.py ..........                                                                                      [ 53%]
test/test_snapshots.py ..................................................ssss                                            [ 68%]
test/test_sshtools.py sssssssssssssssssssssss...ssssss                                                                   [ 77%]
test/test_takeSnapshot.py .........sssssssss                                                                             [ 82%]
test/test_tools.py ..............................................................                                        [100%]Clearing the cache


=============================================== 314 passed, 45 skipped in 11.42s ===============================================

So it skipped 4 fewer tests having detected sshd I guess? No build failure

Test 2 was putting sshfs on the system and building. Same result: 45 skipped and no errors.

[buhtz]

Interesting. On my own system it sometimes comes that I have to restart sshd, then it works. I don't know why it is that way. But it happens not often.

The tests are skipped based on a variable created here:

backintime/common/test/generic.py

Lines 71 to 74 in 524efd3

	LOCAL_SSH = all((tools.processExists('sshd'),
	os.path.isfile(PRIV_KEY_FILE),
	KEY_IN_AUTH,
	sshdPortAvailable))

• sshd to be an existing process
• A private key file should exist (which is usually configured by the tests them self)
• Some more key file magic (KEY_IN_AUTH)
• The port has to be avialble.

Sounds easy to me.

Could be more verbose (using -s option in pytest) with this code modification.

LOCAL_SSH = (tools.processExists('sshd'), 
                  os.path.isfile(PRIV_KEY_FILE), 
                  KEY_IN_AUTH, 
                  sshdPortAvailable)
print(f'{LOCAL_SSH=}')
LOCAL_SSH = all(LOCAL_SSH)

[aryoda]

TLDR ;-)

1. We should refactor our unit tests to separate at least real unit tests from integration tests (using eg. sshd):
   Test & Build: Distinguish between "unit", "system" and "integration" tests #1489
2. The AUR pkg should not need to enable and configure sshd because the related unit tests are in fact integration tests
3. I vote to close this issue (no need to skip more unit tests and building the AUR pkg version "1.4.0-5 in a clean buildroot does not error out").

Details

I would suggest to make an SSH server run.

This is unprecedented as far as I know. A build root is created based on the depends, the package is then built in that build root.

@graysky2

Requiring a running and configured sshd is an integration test but not an isolated unit test (which instead should mock sshd to better test only the "object under test" = our code - not the sshd).

So I think this shouldn't be done by the AUR package but instead we should refactor our unit tests as @buhtz proposed:

> In the far away future I do plan (https://github.com/bit-team/backintime/issues/1489) to separate the three tests types unit, integration and system tests.

So it skipped 4 fewer tests having detected sshd I guess?

Yes, 4 tests are not skipped because they do simple ssh tests eg. by calling ssh-keygen instead of logging-in into ssh:

backintime/common/test/test_sshtools.py

Lines 273 to 276 in 524efd3

	@unittest.skipIf(not tools.checkCommand('ssh-keygen'),
	"'ssh-keygen' not found.")
	def test_sshKeyFingerprint(self):
	self.assertIsNone(sshtools.sshKeyFingerprint(os.path.abspath(__file__)))

All other ssh tests - that require a successful ssh-login - were skipped because they require a local ssh server with installed public + private key file (and no passphrase) , eg. here:

backintime/common/test/test_sshtools.py

Lines 37 to 38 in 524efd3

	@unittest.skipIf(not generic.LOCAL_SSH, 'Skip as this test requires a local ssh server, public and private keys installed')
	class TestSSH(generic.SSHTestCase):

[buhtz]

Re-opening because of the dependency update in CONTRIBUTION.md file