You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/11/15 17:51:29 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #27696: Replace `unittests` in providers tests by pure `pytest` [Wave-3]
Taragolis opened a new pull request, #27696:
URL: https://github.com/apache/airflow/pull/27696
Follow-up: #26831, #26992
Attempt to replace `unittests.TestCase` by native `pytest` classes into providers tests except this:
- `google` (146 modules)
- `amazon` (88 modules)
- `apache` (33 modules)
- `microsoft` (27 modules)
All changes are straight forward:
- Get rid of `unittests.TestCase` class and `TestCase.assert*` methods
- Replace decorator `parameterized.expand` by `pytest.mark.parametrize`. I cant see any benefits if compare to builtin `pytest`.
- Rename ClassName**Test** to **Test**ClassName
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #27696: Replace `unittests` in providers tests by pure `pytest` [Wave-3]
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #27696:
URL: https://github.com/apache/airflow/pull/27696
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #27696: Replace `unittests` in providers tests by pure `pytest` [Wave-3]
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #27696:
URL: https://github.com/apache/airflow/pull/27696#discussion_r1023859753
##########
tests/test_utils/asserts.py:
##########
@@ -19,22 +19,38 @@
import logging
import re
import traceback
+import warnings
from collections import Counter
from contextlib import contextmanager
+from typing import TYPE_CHECKING
from sqlalchemy import event
# Long import to not create a copy of the reference, but to refer to one place.
import airflow.settings
+if TYPE_CHECKING:
+ from unittest import TestCase
+
log = logging.getLogger(__name__)
-def assert_equal_ignore_multiple_spaces(case, first, second, msg=None):
+def assert_equal_ignore_multiple_spaces(case: TestCase | None, first, second, msg=None):
def _trim(s):
return re.sub(r"\s+", " ", s.strip())
- return case.assertEqual(_trim(first), _trim(second), msg)
+ if case:
+ warnings.warn(
+ "Passing `case` has no effect and will be remove in the future. "
+ "Please set to `None` for avoid this warning.",
+ FutureWarning,
+ stacklevel=3,
+ )
+
+ if not msg:
+ assert _trim(first) == _trim(second)
+ else:
+ assert _trim(first) == _trim(second), msg
Review Comment:
Replace usage `unittests.TestCase.assertEqual` by assert.
Because some of tests use this helper (google, amazon, apache) I add warning as a reminder for further migration to `pytest`.
##########
tests/providers/sftp/hooks/test_sftp.py:
##########
@@ -180,11 +178,11 @@ def test_no_host_key_check_default(self, get_connection):
@mock.patch("airflow.providers.sftp.hooks.sftp.SFTPHook.get_connection")
def test_no_host_key_check_enabled(self, get_connection):
- connection = Connection(login="login", host="host", extra='{"no_host_key_check": false}')
+ connection = Connection(login="login", host="host", extra='{"no_host_key_check": true}')
get_connection.return_value = connection
hook = SFTPHook()
- assert hook.no_host_key_check is False
+ assert hook.no_host_key_check is True
Review Comment:
Fix this test.
It might be a typo because it had same implementations as `test_no_host_key_check_disabled` initially
##########
tests/providers/sftp/hooks/test_sftp.py:
##########
@@ -385,41 +385,45 @@ def test_invalid_ssh_hook(self, mock_get_connection):
with pytest.raises(AirflowException, match="ssh_hook must be an instance of SSHHook"):
connection = Connection(conn_id="sftp_default", login="root", host="localhost")
mock_get_connection.return_value = connection
- SFTPHook(ssh_hook="invalid_hook") # type: ignore
+ with pytest.warns(DeprecationWarning, match=r"Parameter `ssh_hook` is deprecated.*"):
+ SFTPHook(ssh_hook="invalid_hook")
@mock.patch("airflow.providers.ssh.hooks.ssh.SSHHook.get_connection")
def test_valid_ssh_hook(self, mock_get_connection):
connection = Connection(conn_id="sftp_test", login="root", host="localhost")
mock_get_connection.return_value = connection
- hook = SFTPHook(ssh_hook=SSHHook(ssh_conn_id="sftp_test"))
+ with pytest.warns(DeprecationWarning, match=r"Parameter `ssh_hook` is deprecated.*"):
+ hook = SFTPHook(ssh_hook=SSHHook(ssh_conn_id="sftp_test"))
assert hook.ssh_conn_id == "sftp_test"
assert isinstance(hook.get_conn(), paramiko.SFTPClient)
def test_get_suffix_pattern_match(self):
output = self.hook.get_file_by_pattern(TMP_PATH, "*.txt")
- self.assertTrue(output, TMP_FILE_FOR_TESTS)
+ # In CI files might have different name, so we check that file found rather than actual name
+ assert output, TMP_FILE_FOR_TESTS
Review Comment:
Just for now I keep same logic for this test: "Returns any file without check expected".
Seems like in CI and local breeze there are different set of files as result
```python
assert output == TMP_FILE_FOR_TESTS
```
Pass in the local environment and failed in the CI with error
```text
E AssertionError: assert 'remote_path.txt' == 'test_file.txt'
E - test_file.txt
E + remote_path.txt
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #27696: Replace `unittests` in providers tests by pure `pytest` [Wave-3]
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #27696:
URL: https://github.com/apache/airflow/pull/27696#discussion_r1024833332
##########
tests/providers/common/sql/sensors/test_sql.py:
##########
@@ -245,8 +242,8 @@ def test_sql_sensor_postgres_poke_invalid_success(self, mock_hook):
op.poke(None)
assert "self.success is present, but not callable -> [1]" == str(ctx.value)
- @unittest.skipIf(
- "AIRFLOW_RUNALL_TESTS" not in os.environ, "Skipped because AIRFLOW_RUNALL_TESTS is not set"
+ @pytest.mark.skipif(
+ "AIRFLOW_RUNALL_TESTS" not in os.environ, reason="Skipped because AIRFLOW_RUNALL_TESTS is not set"
Review Comment:
I have no idea is variable `AIRFLOW_RUNALL_TESTS` set anywhere because I couldn't find any reference outside of tests in `main`. This variable also check in some other tests
I found that initially this env added by this PR https://github.com/apache/airflow/pull/7468 and info about it should store into the https://issues.apache.org/jira/browse/AIRFLOW-6847 (unfortunately I do not have access).
When I tried set `export AIRFLOW_RUNALL_TESTS=x` in `breeze` and run tests I've always get same error, regardless of whether migrated to pytest or not
```
ERROR airflow.task:taskinstance.py:1749 Task failed with exception
Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/urllib3/connection.py", line 175, in _new_conn
(self._dns_host, self.port), self.timeout, **extra_kw
File "/usr/local/lib/python3.7/site-packages/urllib3/util/connection.py", line 95, in create_connection
raise err
File "/usr/local/lib/python3.7/site-packages/urllib3/util/connection.py", line 85, in create_connection
sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/urllib3/connectionpool.py", line 710, in urlopen
chunked=chunked,
File "/usr/local/lib/python3.7/site-packages/urllib3/connectionpool.py", line 398, in _make_request
conn.request(method, url, **httplib_request_kw)
File "/usr/local/lib/python3.7/site-packages/urllib3/connection.py", line 239, in request
super(HTTPConnection, self).request(method, url, body=body, headers=headers)
File "/usr/local/lib/python3.7/http/client.py", line 1281, in request
self._send_request(method, url, body, headers, encode_chunked)
File "/usr/local/lib/python3.7/http/client.py", line 1327, in _send_request
self.endheaders(body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.7/http/client.py", line 1276, in endheaders
self._send_output(message_body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.7/http/client.py", line 1036, in _send_output
self.send(msg)
File "/usr/local/lib/python3.7/http/client.py", line 976, in send
self.connect()
File "/usr/local/lib/python3.7/site-packages/urllib3/connection.py", line 205, in connect
conn = self._new_conn()
File "/usr/local/lib/python3.7/site-packages/urllib3/connection.py", line 187, in _new_conn
self, "Failed to establish a new connection: %s" % e
urllib3.exceptions.NewConnectionError: <urllib3.connection.HTTPConnection object at 0x7f4a91d33e10>: Failed to establish a new connection: [Errno 111] Connection refused
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/requests/adapters.py", line 499, in send
timeout=timeout,
File "/usr/local/lib/python3.7/site-packages/urllib3/connectionpool.py", line 788, in urlopen
method, url, error=e, _pool=self, _stacktrace=sys.exc_info()[2]
File "/usr/local/lib/python3.7/site-packages/urllib3/util/retry.py", line 592, in increment
raise MaxRetryError(_pool, url, error or ResponseError(cause))
urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=3400): Max retries exceeded with url: /v1/statement (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f4a91d33e10>: Failed to establish a new connection: [Errno 111] Connection refused'))
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/opt/airflow/airflow/sensors/base.py", line 200, in execute
poke_return = self.poke(context)
File "/opt/airflow/airflow/providers/common/sql/sensors/sql.py", line 92, in poke
records = hook.get_records(self.sql, self.parameters)
File "/opt/airflow/airflow/providers/presto/hooks/presto.py", line 152, in get_records
return super().get_records(self.strip_sql_string(sql), parameters)
File "/opt/airflow/airflow/providers/common/sql/hooks/sql.py", line 196, in get_records
return self.run(sql=sql, parameters=parameters, handler=fetch_all_handler)
File "/opt/airflow/airflow/providers/presto/hooks/presto.py", line 196, in run
return_last=return_last,
File "/opt/airflow/airflow/providers/common/sql/hooks/sql.py", line 266, in run
self._run_command(cur, sql_statement, parameters)
File "/opt/airflow/airflow/providers/common/sql/hooks/sql.py", line 291, in _run_command
cur.execute(sql_statement)
File "/usr/local/lib/python3.7/site-packages/prestodb/dbapi.py", line 236, in execute
result = self._query.execute()
File "/usr/local/lib/python3.7/site-packages/prestodb/client.py", line 542, in execute
response = self._request.post(self._sql)
File "/usr/local/lib/python3.7/site-packages/prestodb/client.py", line 356, in post
proxies=PROXIES,
File "/usr/local/lib/python3.7/site-packages/prestodb/exceptions.py", line 130, in decorated
raise error
File "/usr/local/lib/python3.7/site-packages/prestodb/exceptions.py", line 117, in decorated
result = func(*args, **kwargs)
File "/usr/local/lib/python3.7/site-packages/requests/sessions.py", line 635, in post
return self.request("POST", url, data=data, json=json, **kwargs)
File "/usr/local/lib/python3.7/site-packages/requests/sessions.py", line 587, in request
resp = self.send(prep, **send_kwargs)
File "/usr/local/lib/python3.7/site-packages/requests/sessions.py", line 701, in send
r = adapter.send(request, **kwargs)
File "/usr/local/lib/python3.7/site-packages/requests/adapters.py", line 565, in send
raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPConnectionPool(host='localhost', port=3400): Max retries exceeded with url: /v1/statement (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f4a91d33e10>: Failed to establish a new connection: [Errno 111] Connection refused'))
```
Is it possible that this kind of tests are actually broken and just not run in CI?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27696: Replace `unittests` in providers tests by pure `pytest` [Wave-3]
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27696:
URL: https://github.com/apache/airflow/pull/27696#discussion_r1025125302
##########
tests/providers/common/sql/sensors/test_sql.py:
##########
@@ -245,8 +242,8 @@ def test_sql_sensor_postgres_poke_invalid_success(self, mock_hook):
op.poke(None)
assert "self.success is present, but not callable -> [1]" == str(ctx.value)
- @unittest.skipIf(
- "AIRFLOW_RUNALL_TESTS" not in os.environ, "Skipped because AIRFLOW_RUNALL_TESTS is not set"
+ @pytest.mark.skipif(
+ "AIRFLOW_RUNALL_TESTS" not in os.environ, reason="Skipped because AIRFLOW_RUNALL_TESTS is not set"
Review Comment:
It's very likely
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org