Merge pull request #3128 from skshetry/refactor-hdfs-gcp

test: move hdfs and gcp functions into helper classes

tests/func/test_data_cloud.py

@@ -31,10 +31,10 @@
 from tests.utils import spy
 
 from tests.remotes import (
-    _should_test_gcp,
-    _should_test_hdfs,
     Azure,
+    GCP,
     GDrive,
+    HDFS,
     S3,
     SSHMocked,
     OSS,
@@ -44,8 +44,6 @@
     TEST_GDRIVE_CLIENT_ID,
     TEST_GDRIVE_CLIENT_SECRET,
     TEST_REMOTE,
-    get_gcp_url,
-    get_hdfs_url,
     get_local_url,
 )
 
@@ -232,7 +230,7 @@ def _get_cloud_class(self):
 
 class TestRemoteGS(TestDataCloudBase):
     def _should_test(self):
-        return _should_test_gcp()
+        return GCP.should_test()
 
     def _setup_cloud(self):
         self._ensure_should_run()
@@ -250,7 +248,7 @@ def _setup_cloud(self):
         self.assertIsInstance(self.cloud.get_remote(), self._get_cloud_class())
 
     def _get_url(self):
-        return get_gcp_url()
+        return GCP.get_url()
 
     def _get_cloud_class(self):
         return RemoteGS
@@ -332,10 +330,10 @@ def _get_cloud_class(self):
 
 class TestRemoteHDFS(TestDataCloudBase):
     def _should_test(self):
-        return _should_test_hdfs()
+        return HDFS.should_test()
 
     def _get_url(self):
-        return get_hdfs_url()
+        return HDFS.get_url()
 
     def _get_cloud_class(self):
         return RemoteHDFS
@@ -431,10 +429,10 @@ def _test(self):
 
 class TestRemoteHDFSCLI(TestDataCloudCLIBase):
     def _should_test(self):
-        return _should_test_hdfs()
+        return HDFS.should_test()
 
     def _test(self):
-        url = get_hdfs_url()
+        url = HDFS.get_url()
 
         self.main(["remote", "add", TEST_REMOTE, url])
 
@@ -485,10 +483,10 @@ def _test(self):
 
 class TestRemoteGSCLI(TestDataCloudCLIBase):
     def _should_test(self):
-        return _should_test_gcp()
+        return GCP.should_test()
 
     def _test(self):
-        url = get_gcp_url()
+        url = GCP.get_url()
 
         self.main(["remote", "add", TEST_REMOTE, url])
         self.main(

tests/func/test_repro.py

@@ -9,6 +9,7 @@
 from subprocess import PIPE
 from subprocess import Popen
 from urllib.parse import urljoin
+from unittest import SkipTest
 
 import boto3
 import paramiko
@@ -35,8 +36,8 @@
 from dvc.utils.stage import load_stage_file
 from tests.basic_env import TestDvc
 from tests.remotes import (
-    _should_test_gcp,
-    _should_test_hdfs,
+    GCP,
+    HDFS,
     S3,
     SSH,
     SSHMocked,
@@ -857,7 +858,9 @@ def check_already_cached(self, stage):
     @patch("dvc.prompt.confirm", return_value=True)
     def test(self, mock_prompt):
         if not self.should_test():
-            return
+            raise SkipTest(
+                "Test {} is disabled".format(self.__class__.__name__)
+            )
 
         cache = (
             self.scheme
@@ -975,7 +978,7 @@ def write(self, bucket, key, body):
 
 class TestReproExternalGS(TestReproExternalBase):
     def should_test(self):
-        return _should_test_gcp()
+        return GCP.should_test()
 
     @property
     def scheme(self):
@@ -996,7 +999,7 @@ def write(self, bucket, key, body):
 
 class TestReproExternalHDFS(TestReproExternalBase):
     def should_test(self):
-        return _should_test_hdfs()
+        return HDFS.should_test()
 
     @property
     def scheme(self):

tests/remotes.py

@@ -43,52 +43,6 @@
 TEST_GDRIVE_CLIENT_SECRET = "2fy_HyzSwkxkGzEken7hThXb"
 
 
-def _should_test_gcp():
-    do_test = env2bool("DVC_TEST_GCP", undefined=None)
-    if do_test is not None:
-        return do_test
-
-    if not os.path.exists(TEST_GCP_CREDS_FILE):
-        return False
-
-    try:
-        check_output(
-            [
-                "gcloud",
-                "auth",
-                "activate-service-account",
-                "--key-file",
-                TEST_GCP_CREDS_FILE,
-            ]
-        )
-    except (CalledProcessError, OSError):
-        return False
-    return True
-
-
-def _should_test_hdfs():
-    if platform.system() != "Linux":
-        return False
-
-    try:
-        check_output(
-            ["hadoop", "version"], shell=True, executable=os.getenv("SHELL")
-        )
-    except (CalledProcessError, IOError):
-        return False
-
-    p = Popen(
-        "hadoop fs -ls hdfs://127.0.0.1/",
-        shell=True,
-        executable=os.getenv("SHELL"),
-    )
-    p.communicate()
-    if p.returncode != 0:
-        return False
-
-    return True
-
-
 def get_local_storagepath():
     return TestDvc.mkdtemp()
 
@@ -97,20 +51,6 @@ def get_local_url():
     return get_local_storagepath()
 
 
-def get_hdfs_url():
-    return "hdfs://{}@127.0.0.1{}".format(
-        getpass.getuser(), get_local_storagepath()
-    )
-
-
-def get_gcp_storagepath():
-    return TEST_GCP_REPO_BUCKET + "/" + str(uuid.uuid4())
-
-
-def get_gcp_url():
-    return "gs://" + get_gcp_storagepath()
-
-
 class Local:
     should_test = lambda: True  # noqa: E731
     get_url = get_local_url
@@ -160,8 +100,36 @@ def put_objects(remote, objects):
 
 
 class GCP:
-    should_test = _should_test_gcp
-    get_url = get_gcp_url
+    @staticmethod
+    def should_test():
+        do_test = env2bool("DVC_TEST_GCP", undefined=None)
+        if do_test is not None:
+            return do_test
+
+        if not os.path.exists(TEST_GCP_CREDS_FILE):
+            return False
+
+        try:
+            check_output(
+                [
+                    "gcloud",
+                    "auth",
+                    "activate-service-account",
+                    "--key-file",
+                    TEST_GCP_CREDS_FILE,
+                ]
+            )
+        except (CalledProcessError, OSError):
+            return False
+        return True
+
+    @staticmethod
+    def get_storagepath():
+        return TEST_GCP_REPO_BUCKET + "/" + str(uuid.uuid4())
+
+    @staticmethod
+    def get_url():
+        return "gs://" + GCP.get_storagepath()
 
     @classmethod
     @contextmanager
@@ -277,5 +245,33 @@ def get_url(user, port):
 
 
 class HDFS:
-    should_test = _should_test_hdfs
-    get_url = get_hdfs_url
+    @staticmethod
+    def should_test():
+        if platform.system() != "Linux":
+            return False
+
+        try:
+            check_output(
+                ["hadoop", "version"],
+                shell=True,
+                executable=os.getenv("SHELL"),
+            )
+        except (CalledProcessError, IOError):
+            return False
+
+        p = Popen(
+            "hadoop fs -ls hdfs://127.0.0.1/",
+            shell=True,
+            executable=os.getenv("SHELL"),
+        )
+        p.communicate()
+        if p.returncode != 0:
+            return False
+
+        return True
+
+    @staticmethod
+    def get_url():
+        return "hdfs://{}@127.0.0.1{}".format(
+            getpass.getuser(), get_local_storagepath()
+        )