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

tests: remove redundant native ssh tests #3104

Merged
merged 6 commits into from
Jan 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions dvc/remote/ssh/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,12 @@ def _remove_file(self, path):
def _remove_dir(self, path):
for root, dirs, files in self.walk(path, topdown=False):
for fname in files:
path = posixpath.join(root, fname)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a pretty nasty bug 🙁 Added a test

with suppress(FileNotFoundError):
self._remove_file(path)
self._remove_file(posixpath.join(root, fname))

for dname in dirs:
path = posixpath.join(root, dname)
with suppress(FileNotFoundError):
self.sftp.rmdir(dname)
self.sftp.rmdir(posixpath.join(root, dname))

with suppress(FileNotFoundError):
self.sftp.rmdir(path)
Expand Down
2 changes: 2 additions & 0 deletions scripts/build-requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
PyInstaller==3.5
psutil
6 changes: 2 additions & 4 deletions scripts/build_posix.sh
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,8 @@ install_dependencies()
pip install --upgrade pip

print_info "Installing requirements..."
pip install .[all] psutil

print_info "Installing pyinstaller..."
pip install pyinstaller
pip install .[all]
pip install -r scripts/build-requirements.txt
}

build_dvc()
Expand Down
3 changes: 1 addition & 2 deletions scripts/build_windows.cmd
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ if %errorlevel% neq 0 (echo Error: Couldn't find Inno Setup compiler. && goto :e
echo ====== Installing requirements... ======
echo PKG = "exe" > dvc\utils\build.py
call pip install .[all] || goto :error
call pip install psutil || goto :error
call pip install -r scripts\build-requirements.txt || goto :error
call dvc pull || goto :error
call pip install pyinstaller || goto :error

echo ====== Building dvc binary... ======
call pyinstaller --additional-hooks-dir scripts\hooks dvc/__main__.py --name dvc --specpath build
Expand Down
58 changes: 0 additions & 58 deletions scripts/hooks/hook-Crypto.py

This file was deleted.

3 changes: 0 additions & 3 deletions scripts/hooks/hook-aliyunsdkcore.py

This file was deleted.

1 change: 0 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ def run(self):

# Extra dependecies to run tests
tests_requirements = [
"PyInstaller==3.5",
"wheel>=0.31.1",
"pydot>=1.2.4",
# Test requirements:
Expand Down
25 changes: 0 additions & 25 deletions tests/func/test_data_cloud.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
from tests.remotes import (
_should_test_gcp,
_should_test_hdfs,
_should_test_ssh,
Azure,
GDrive,
S3,
Expand All @@ -47,7 +46,6 @@
get_gcp_url,
get_hdfs_url,
get_local_url,
get_ssh_url,
get_ssh_url_mocked,
)

Expand Down Expand Up @@ -296,17 +294,6 @@ def test(self):
self.assertTrue(os.path.isdir(self.dname))


class TestRemoteSSH(TestDataCloudBase):
def _should_test(self):
return _should_test_ssh()

def _get_url(self):
return get_ssh_url()

def _get_cloud_class(self):
return RemoteSSH


@pytest.mark.usefixtures("ssh_server")
class TestRemoteSSHMocked(TestDataCloudBase):
@pytest.fixture(autouse=True)
Expand Down Expand Up @@ -442,18 +429,6 @@ def _test(self):
self._test_cloud(TEST_REMOTE)


class TestRemoteSSHCLI(TestDataCloudCLIBase):
def _should_test(self):
return _should_test_ssh()

def _test(self):
url = get_ssh_url()

self.main(["remote", "add", TEST_REMOTE, url])

self._test_cloud(TEST_REMOTE)


class TestRemoteHDFSCLI(TestDataCloudCLIBase):
def _should_test(self):
return _should_test_hdfs()
Expand Down
40 changes: 28 additions & 12 deletions tests/func/test_repro.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
_should_test_gcp,
_should_test_hdfs,
_should_test_ssh,
get_ssh_url,
get_ssh_url_mocked,
S3,
TEST_AWS_REPO_BUCKET,
TEST_GCP_REPO_BUCKET,
Expand Down Expand Up @@ -1611,30 +1611,46 @@ def test(self):
assert evaluation[2].relpath == "E.dvc"


def test_ssh_dir_out(tmp_dir, dvc):
if not _should_test_ssh():
pytest.skip()

@pytest.mark.skipif(
os.name == "nt",
reason="external output scenario is not supported on Windows",
)
def test_ssh_dir_out(tmp_dir, dvc, ssh_server):
tmp_dir.gen({"foo": "foo content"})

# Set up remote and cache
remote_url = get_ssh_url()
user = ssh_server.test_creds["username"]
port = ssh_server.port
keyfile = ssh_server.test_creds["key_filename"]

remote_url = get_ssh_url_mocked(user, port)
assert main(["remote", "add", "upstream", remote_url]) == 0
assert main(["remote", "modify", "upstream", "keyfile", keyfile]) == 0

cache_url = get_ssh_url()
cache_url = get_ssh_url_mocked(user, port)
assert main(["remote", "add", "sshcache", cache_url]) == 0
assert main(["config", "cache.ssh", "sshcache"]) == 0
assert main(["remote", "modify", "sshcache", "keyfile", keyfile]) == 0

# Recreating to reread configs
repo = DvcRepo(dvc.root_dir)

# To avoid "WARNING: UNPROTECTED PRIVATE KEY FILE" from ssh
os.chmod(keyfile, 0o600)

(tmp_dir / "script.py").write_text(
"import sys, pathlib\n"
"path = pathlib.Path(sys.argv[1])\n"
"dir_out = path / 'dir-out'\n"
"dir_out.mkdir()\n"
"(dir_out / '1.txt').write_text('1')\n"
"(dir_out / '2.txt').write_text('2')\n"
)

url_info = URLInfo(remote_url)
mkdir_cmd = "mkdir dir-out;cd dir-out;echo 1 > 1.txt; echo 2 > 2.txt"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually didn't work because of the bug in remove. That is why you need to use && to ensure that no error occurs. Same complaint as when people don't use set -e \n set -x in their bash scripts 🙂

repo.run(
cmd="ssh {netloc} 'cd {path};{cmd}'".format(
netloc=url_info.netloc, path=url_info.path, cmd=mkdir_cmd
),
outs=[(url_info / "dir-out").url],
cmd="python {} {}".format(tmp_dir / "script.py", url_info.path),
outs=["remote://upstream/dir-out"],
deps=["foo"], # add a fake dep to not consider this a callback
)

Expand Down
10 changes: 10 additions & 0 deletions tests/unit/remote/ssh/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ def test_makedirs(tmp_path, ssh):
assert os.path.isdir(path)


def test_remove_dir(tmp_path, ssh):
dpath = tmp_path / "dir"
dpath.mkdir()
(dpath / "file").write_text("file")
(dpath / "subdir").mkdir()
(dpath / "subdir" / "subfile").write_text("subfile")
ssh.remove(dpath.absolute().as_posix())
assert not dpath.exists()


def test_walk(tmp_path, ssh):
root_path = tmp_path
dir_path = root_path / "dir"
Expand Down