Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port test_initremote_basic_sshurl over to use ria_sshserver fixture #58

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

adswa
Copy link
Member

@adswa adswa commented Sep 13, 2023

This change copies the entire function _test_initremote_basic over from datalad-core without any change other then removing two with_tempfile decorators.
I further adjusts test_initremote_basic_sshurl. This test parametrizes the _test_initremote_basic helper with things like the ssh url. My changes are supposed to make the helper use things from the ria_sshserver fixture, and it would be cool to know if the way I did this looks correct.

This is the diff to what's in datalad-core for these two functions:

diff --git a/datalad/distributed/tests/test_ria_basics.py b/datalad/distributed/tests/test_ria_basics.py
index 932711d0c..6b8926daf 100644
--- a/datalad/distributed/tests/test_ria_basics.py
+++ b/datalad/distributed/tests/test_ria_basics.py
@@ -64,8 +64,7 @@ from datalad.utils import Path
 # talking to the special remote via annex.
 
 
-@with_tempfile
-@with_tempfile
+
 def _test_initremote_basic(url, io, store, ds_path, link):
 
     ds_path = Path(ds_path)
@@ -163,16 +162,18 @@ def _test_initremote_basic(url, io, store, ds_path, link):
     #       - might require to run initremote directly to get the output
 
 
-# TODO: Skipped due to gh-4436
-@known_failure_windows
-@skip_ssh
-@with_tempfile
-def test_initremote_basic_sshurl(storepath=None):
+def test_initremote_basic_sshurl(ria_sshserver, tmp_path):
+    """Test via SSH"""
+    # retrieve all values from the ssh-server fixture
+    host, port, login, key, path, localpath = ria_sshserver.values()
+    # create all parameters _test_initremote_basic() requires
+    storepath = tmp_path / "store"
+    url = f'ria+ssh://{login}@{host}:{storepath}'
+    io = SSHRemoteIO(host)
+    ds_path = Path(tmp_path / 'my-ds')
+    link = tmp_path / "link"
     _test_initremote_basic(
-        'ria+ssh://datalad-test{}'.format(Path(storepath).as_posix()), \
-        SSHRemoteIO('datalad-test'), \
-        storepath,
-    )
+        url, io, storepath, ds_path, link)
 

@mih
Copy link
Member

mih commented Sep 20, 2023

I will give this a pass now.

Rebased, and force-pushed. It passes for me locally. Let's see how windows does.

Update: It does nothing https://ci.appveyor.com/project/mih/datalad-ria/builds/48082357

Apparently, we need to focus on going up the code stack, and first verify that SSHRemoteIO is fully functional. I am not aware of tests that could be ported for that purpose. They need to be written from scratch.

mih and others added 5 commits September 20, 2023 13:50
This should be easier to use. The old implementation is now available in
`ria_sshserver_setup` and should be considered internal.

The new fixture returns the base RIA URL, and a local path to that same
root directory (if one exists).

It also sets `DATALAD_SSH_IDENTITYFILE`, which is needed by datalad-core's
`SSHManager`. The setup may still need to use the `datalad_cfg` fixture,
and possibly a dedicated reload to be effective.

Closes #60
Put all the "should not work" in a dedicated test. Needs no dataset
population, needs no store, needs no remote IO.
…lace with corresponding pytest/python functionality
Comment on lines 13 to 14
common_init_opts,
populate_dataset,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be replaced with fixtures introduced in #67 and #65

@adswa
Copy link
Member Author

adswa commented Sep 20, 2023

EDIT: My thoughts below are wrong, the differences just resulted from different environment variables set on the two computers.

I think something odd is happening on Windows with regard to the path on the ssh server for the _defunc_test_initremote_basic_sshurl test. Compare this debug message for the internal initremote calll on Linux:

[DEBUG  ] Run ['git', '-c', 'diff.ignoreSubmodules=none', '-c', 'core.quotepath=false', 'annex', 'initremote', 'ria-remote', 'encryption=none', 'type=external', 'externaltype=ora', 'autoenable=true', 'url=ria+ssh://adina@localhost/tmp/pytest-of-adina/pytest-58/sshriaroot0', '-c', 'annex.dotfiles=true'] (protocol_class=StdOutErrCapture) (cwd=/tmp/pytest-of-adina/pytest-58/test_defunc_test_initremote_ba0/my-ds) 

with this one from Windows:

[DEBUG] Run ['git', '-c', 'diff.ignoreSubmodules=none', '-c', 'core.quotepath=false', 'annex', 'initremote', 'ria-remote', 'encryption=none', 'type=external', 'externaltype=ora', 'autoenable=true', 'url=ria+ssh://sshuser@localhost/usr/local/apache2/htdocs', '-c', 'annex.dotfiles=true'] (protocol_class=StdOutErrCapture) (cwd=C:\Users\adina\AppData\Local\Temp\pytest-of-adina\pytest-68\dataset0)

This path (sshuser@localhost/usr/local/apache2/htdoc) in the Docker container is owned by root, and would not be writable by sshuser.

datalad_ria/tests/fixtures.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants