Taragolis commented on code in PR #27948:
URL: https://github.com/apache/airflow/pull/27948#discussion_r1032884418
##########
tests/providers/apache/hdfs/hooks/test_webhdfs.py:
##########
@@ -85,61 +84,39 @@ def test_get_conn_with_schema(self, socket_mock,
mock_get_connection, mock_insec
mock_insecure_client.return_value.status.assert_called_once_with("/")
assert conn == mock_insecure_client.return_value
- @patch("airflow.providers.apache.hdfs.hooks.webhdfs.requests.Session",
return_value="session")
- @patch("airflow.providers.apache.hdfs.hooks.webhdfs.InsecureClient")
- @patch(
-
"airflow.providers.apache.hdfs.hooks.webhdfs.WebHDFSHook.get_connection",
- return_value=Connection(host="host_1.com,host_2.com", login="user"),
+ @pytest.mark.parametrize(
+ "test_connection",
+ [
+ pytest.param(Connection(host="host_1.com,host_2.com",
login="user"), id="without-password"),
+ pytest.param(
+ Connection(host="host_1.com,host_2.com", login="user",
password="password"),
+ id="with-password",
+ ),
+ ],
Review Comment:
Merge two identical test implementation (`test_get_conn_without_port_schema`
and `test_get_conn_with_password_without_port_schema`) into single one
##########
tests/providers/apache/livy/hooks/test_livy.py:
##########
@@ -17,34 +17,55 @@
from __future__ import annotations
Review Comment:
A lot of changes in this module:
1. Make sure that changes in tests-connections reflected in tests
2. Replace all SubTests
3. replace `requests_mock.mock` decorator by `requests_mock` fixture
4. Add additional test parametrisation
##########
tests/providers/apache/hive/hooks/test_hive.py:
##########
@@ -55,23 +53,7 @@ def __init__(self):
self.iterable = []
-class TestHiveEnvironment(unittest.TestCase):
- def setUp(self):
- self.next_day = (DEFAULT_DATE +
datetime.timedelta(days=1)).isoformat()[:10]
- self.database = "airflow"
- self.partition_by = "ds"
- self.table = "static_babynames_partitioned"
- with mock.patch(
-
"airflow.providers.apache.hive.hooks.hive.HiveMetastoreHook.get_metastore_client"
- ) as get_metastore_mock, mock.patch(
-
"airflow.providers.apache.hive.hooks.hive.HiveMetastoreHook.get_connection"
- ):
- get_metastore_mock.return_value = mock.MagicMock()
-
- self.hook = HiveMetastoreHook()
Review Comment:
This class only use once in the same module and also have the same name as
reusable test class from `tests/providers/apache/hive/__init__.py` which may
confuse
##########
tests/providers/apache/hdfs/sensors/test_hdfs.py:
##########
@@ -97,21 +96,22 @@ def test_legacy_file_does_not_exists(self):
task.execute(None)
-class TestHdfsSensorFolder(unittest.TestCase):
- def setUp(self):
+class TestHdfsSensorFolder:
+ def setup_method(self, method):
self.hook = FakeHDFSHook
- self.log = logging.getLogger()
- self.log.setLevel(logging.DEBUG)
+
+ logger = logging.getLogger(__name__)
+ logger.setLevel(logging.DEBUG)
+ logger.debug("#" * 10)
+ logger.debug("Running test case: %s.%s", self.__class__.__name__,
method.__name__)
+ logger.debug("#" * 10)
Review Comment:
This part previously exists in tests case methods.
Is a good idea get rid of this logger entirely?
##########
tests/providers/apache/hdfs/sensors/test_hdfs.py:
##########
@@ -200,21 +191,22 @@ def test_should_be_non_empty_directory_fail(self):
task.execute(None)
-class TestHdfsSensorRegex(unittest.TestCase):
- def setUp(self):
+class TestHdfsSensorRegex:
+ def setup_method(self, method):
self.hook = FakeHDFSHook
- self.log = logging.getLogger()
- self.log.setLevel(logging.DEBUG)
+
+ logger = logging.getLogger(__name__)
+ logger.setLevel(logging.DEBUG)
+ logger.debug("#" * 10)
+ logger.debug("Running test case: %s.%s", self.__class__.__name__,
method.__name__)
+ logger.debug("#" * 10)
Review Comment:
Same as mention for `TestHdfsSensorFolder`
##########
tests/providers/apache/drill/operators/test_drill.py:
##########
@@ -32,13 +30,13 @@
@pytest.mark.backend("drill")
-class TestDrillOperator(unittest.TestCase):
- def setUp(self):
Review Comment:
`TestDrillOperator` has a bit strange decorator might be some typo or
outdated stuff:
- Backend `drill` not exists
- Integration `drill` not exists but might be exists in the past
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
