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/12/06 09:59:03 UTC
[GitHub] [airflow] IAL32 opened a new pull request, #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
IAL32 opened a new pull request, #28145:
URL: https://github.com/apache/airflow/pull/28145
Related: https://github.com/apache/airflow/pull/28039 https://github.com/apache/airflow/pull/28139 @Taragolis
Migrate Amazon provider's transfer tests to `pytest`.
All changes are more or less straightforward:
- Get rid of unittests.TestCase class and TestCase.assert* methods
- Convert setUp* and tearDown* methods to [appropriate pytest alternative](https://docs.pytest.org/en/6.2.x/xunit_setup.html#classic-xunit-style-setup)
- Replace decorator `parameterized.expand` by `pytest.mark.parametrize`.
--
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 #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28145:
URL: https://github.com/apache/airflow/pull/28145#discussion_r1041470239
##########
tests/providers/amazon/aws/transfers/test_dynamodb_to_s3.py:
##########
@@ -18,13 +18,12 @@
from __future__ import annotations
import json
-import unittest
from unittest.mock import MagicMock, patch
from airflow.providers.amazon.aws.transfers.dynamodb_to_s3 import DynamoDBToS3Operator
-class DynamodbToS3Test(unittest.TestCase):
+class DynamodbToS3Test:
def setUp(self):
Review Comment:
For some reason keeping `setUp` not affect test execution `¯\_(ツ)_/¯`
```suggestion
class DynamodbToS3Test:
def setup_method(self):
```
--
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] IAL32 commented on a diff in pull request #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
IAL32 commented on code in PR #28145:
URL: https://github.com/apache/airflow/pull/28145#discussion_r1041490849
##########
tests/providers/amazon/aws/sensors/test_batch.py:
##########
@@ -42,7 +42,7 @@ def setup_method(self):
@mock.patch.object(BatchClientHook, "get_job_description")
def test_poke_on_success_state(self, mock_get_job_description):
mock_get_job_description.return_value = {"status": "SUCCEEDED"}
- assert self.batch_sensor.poke({})
+ assert self.batch_sensor.poke({}) is True
Review Comment:
Small nitpick from @ferruzzi :
https://github.com/apache/airflow/pull/28139#pullrequestreview-1207017465
--
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] ferruzzi commented on a diff in pull request #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28145:
URL: https://github.com/apache/airflow/pull/28145#discussion_r1041609034
##########
tests/providers/amazon/aws/sensors/test_batch.py:
##########
@@ -42,7 +42,7 @@ def setup_method(self):
@mock.patch.object(BatchClientHook, "get_job_description")
def test_poke_on_success_state(self, mock_get_job_description):
mock_get_job_description.return_value = {"status": "SUCCEEDED"}
- assert self.batch_sensor.poke({})
+ assert self.batch_sensor.poke({}) is True
Review Comment:
Thanks for including it here. :+1:
--
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 #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28145:
URL: https://github.com/apache/airflow/pull/28145#discussion_r1040867411
##########
tests/providers/amazon/aws/transfers/test_sql_to_s3.py:
##########
@@ -146,11 +145,12 @@ def test_execute_json(self, mock_s3_hook, temp_mock):
replace=True,
)
- @parameterized.expand(
+ @pytest.mark.parametrize(
+ "_, params",
[
("with-csv", {"file_format": "csv", "null_string_result": None}),
("with-parquet", {"file_format": "parquet", "null_string_result": "None"}),
- ]
+ ],
)
def test_fix_dtypes(self, _, params):
Review Comment:
With `pytest.mark.parametrize` you do not need to use unnamed variable `_` to show actual tests case.
You might use `pytest.param(..., id="some value")`. Actually this id could be use as part of run individual test
```python
@pytest.mark.parametrize(
"params",
[
pytest.param({"file_format": "csv", "null_string_result": None}, id="with-csv"),
pytest.param({"file_format": "parquet", "null_string_result": "None"}, id="with-parquet"),
],
)
def test_fix_dtypes(self, params):
```
##########
tests/providers/amazon/aws/transfers/test_sftp_to_s3.py:
##########
@@ -95,9 +85,10 @@ def test_sftp_to_s3_operation(self, use_temp_file=True):
create_file_task.execute(None)
# Test for creation of s3 bucket
+ s3_hook = S3Hook("aws_default")
Review Comment:
We do not need any connection here we have mock s3 anyway and better use as keyword
```suggestion
s3_hook = S3Hook(aws_conn_id=None)
```
##########
tests/providers/amazon/aws/transfers/test_s3_to_sftp.py:
##########
@@ -84,7 +79,8 @@ def test_s3_to_sftp_operation(self):
with open(LOCAL_FILE_PATH, "w") as file:
file.write(test_remote_file_content)
- self.s3_hook.load_file(LOCAL_FILE_PATH, self.s3_key, bucket_name=BUCKET)
+ s3_hook = S3Hook("aws_default")
Review Comment:
```suggestion
s3_hook = S3Hook(aws_conn_id=None)
```
--
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] IAL32 commented on a diff in pull request #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
IAL32 commented on code in PR #28145:
URL: https://github.com/apache/airflow/pull/28145#discussion_r1041487038
##########
tests/providers/amazon/aws/transfers/test_dynamodb_to_s3.py:
##########
@@ -18,13 +18,12 @@
from __future__ import annotations
import json
-import unittest
from unittest.mock import MagicMock, patch
from airflow.providers.amazon.aws.transfers.dynamodb_to_s3 import DynamoDBToS3Operator
-class DynamodbToS3Test(unittest.TestCase):
+class DynamodbToS3Test:
def setUp(self):
Review Comment:
I made the change anyways, wouldn't want that to bite us in the back later on ahah
--
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 pull request #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28145:
URL: https://github.com/apache/airflow/pull/28145#issuecomment-1339192658
some tests failing :(
--
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 #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28145:
URL: https://github.com/apache/airflow/pull/28145#discussion_r1041470239
##########
tests/providers/amazon/aws/transfers/test_dynamodb_to_s3.py:
##########
@@ -18,13 +18,12 @@
from __future__ import annotations
import json
-import unittest
from unittest.mock import MagicMock, patch
from airflow.providers.amazon.aws.transfers.dynamodb_to_s3 import DynamoDBToS3Operator
-class DynamodbToS3Test(unittest.TestCase):
+class DynamodbToS3Test:
def setUp(self):
Review Comment:
For some reason keep `setUp` not affect test execution `¯\_(ツ)_/¯`
```suggestion
class DynamodbToS3Test:
def setup_method(self):
```
--
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 merged pull request #28145: Migrate amazon provider transfer tests from `unittests` to `pytest`
Posted by GitBox <gi...@apache.org>.
Taragolis merged PR #28145:
URL: https://github.com/apache/airflow/pull/28145
--
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