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/19 21:54:52 UTC
[GitHub] [airflow] shahar1 opened a new pull request, #27797: Add tests to PythonOperator
shahar1 opened a new pull request, #27797:
URL: https://github.com/apache/airflow/pull/27797
Add tests to PythonOperator, thus increasing its coverage to almost 100%.
--
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] shahar1 closed pull request #27797: Add tests to PythonOperator
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 closed pull request #27797: Add tests to PythonOperator
URL: https://github.com/apache/airflow/pull/27797
--
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] uranusjr commented on pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1321529553
Would it be a good idea to use `pytest.mark.parametrize` for this?
--
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 #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1336647899
Still fixes needed :(
--
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] shahar1 commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1036356325
##########
tests/operators/test_python.py:
##########
@@ -375,6 +375,20 @@ def func():
"INFO:airflow.task.operators:Done. Returned value not shown" in cm.output
), "Log message that the option is turned off should be shown"
+ def test_python_operator_templates_exts(self):
+ def func():
+ return "test_return_value"
+
+ python_operator = PythonOperator(
+ task_id="python_operator",
+ python_callable=func,
+ dag=self.dag,
+ show_return_value_in_logs=False,
+ templates_exts=['test_ext']
+ )
+
+ assert python_operator.template_ext == ['test_ext']
Review Comment:
Last time that I checked the following `if` in the constructor wasn't covered:
````python
if templates_exts:
self.template_ext = templates_exts
````
--
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] shahar1 commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1038201659
##########
tests/operators/test_python.py:
##########
@@ -1155,6 +1169,21 @@ def f():
)
copy.deepcopy(task)
+ def test_except_value_error(self):
+ def f():
+ return 1
+
+ task = PythonVirtualenvOperator(
+ python_callable=f,
+ task_id="task",
+ dag=self.dag,
+ )
+
+ task.log.error = unittest.mock.Mock()
+ task.pickling_library.loads = unittest.mock.Mock(side_effect=ValueError)
+ with pytest.raises(ValueError):
+ task._read_result(path=unittest.mock.Mock())
Review Comment:
Created a custom exception class to be raised, which includes the error from `log.error` in its `__str__` method). I might have understood your intentions wrong, let me know if it needs additional fixing.
##########
tests/operators/test_python.py:
##########
@@ -1155,6 +1169,21 @@ def f():
)
copy.deepcopy(task)
+ def test_except_value_error(self):
+ def f():
+ return 1
+
+ task = PythonVirtualenvOperator(
+ python_callable=f,
+ task_id="task",
+ dag=self.dag,
+ )
+
+ task.log.error = unittest.mock.Mock()
+ task.pickling_library.loads = unittest.mock.Mock(side_effect=ValueError)
+ with pytest.raises(ValueError):
+ task._read_result(path=unittest.mock.Mock())
Review Comment:
Created a custom exception class to be raised, which includes the error from `log.error` in its `__str__` method. I might have understood your intentions wrong, let me know if it needs additional fixing.
--
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] uranusjr commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1040774539
##########
airflow/exceptions.py:
##########
@@ -368,3 +368,11 @@ class AirflowProviderDeprecationWarning(DeprecationWarning):
deprecated_provider_since: str | None = None
"Indicates the provider version that started raising this deprecation warning"
+
+
+class DeserializingResultError(AirflowException):
+ """ Raised when an error is encountered while a pickling library deserializes a pickle file"""
+
+ def __str__(self):
+ return "Error deserializing result. Note that result deserialization \n" \
+ "is not supported across major Python versions."
Review Comment:
```suggestion
return (
"Error deserializing result. Note that result deserialization "
"is not supported across major Python versions."
)
```
##########
airflow/operators/python.py:
##########
@@ -372,11 +373,7 @@ def _read_result(self, path: Path):
try:
return self.pickling_library.loads(path.read_bytes())
except ValueError:
- self.log.error(
- "Error deserializing result. Note that result deserialization "
- "is not supported across major Python versions."
- )
- raise
+ raise DeserializingResultError
Review Comment:
```suggestion
raise DeserializingResultError from None
```
##########
airflow/exceptions.py:
##########
@@ -368,3 +368,11 @@ class AirflowProviderDeprecationWarning(DeprecationWarning):
deprecated_provider_since: str | None = None
"Indicates the provider version that started raising this deprecation warning"
+
+
+class DeserializingResultError(AirflowException):
Review Comment:
```suggestion
class DeserializingResultError(ValueError):
```
--
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 #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1327921195
Tests need fixing
--
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] uranusjr commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1034360950
##########
tests/operators/test_python.py:
##########
@@ -375,6 +375,20 @@ def func():
"INFO:airflow.task.operators:Done. Returned value not shown" in cm.output
), "Log message that the option is turned off should be shown"
+ def test_python_operator_templates_exts(self):
+ def func():
+ return "test_return_value"
+
+ python_operator = PythonOperator(
+ task_id="python_operator",
+ python_callable=func,
+ dag=self.dag,
+ show_return_value_in_logs=False,
+ templates_exts=['test_ext']
+ )
+
+ assert python_operator.template_ext == ['test_ext']
Review Comment:
Why is this test relevant? `tempalte_ext` is a property on BaseOperator, is it not?
--
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 #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1327921667
> Would it be a good idea to use `pytest.mark.parametrize` for this?
Using parametrize has its drawbacks - not really clear what's going on. I think if there is smal amount of DRY, DAMP is more important than DRY in tests. https://enterprisecraftsmanship.com/posts/dry-damp-unit-tests/
--
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] shahar1 commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1036356325
##########
tests/operators/test_python.py:
##########
@@ -375,6 +375,20 @@ def func():
"INFO:airflow.task.operators:Done. Returned value not shown" in cm.output
), "Log message that the option is turned off should be shown"
+ def test_python_operator_templates_exts(self):
+ def func():
+ return "test_return_value"
+
+ python_operator = PythonOperator(
+ task_id="python_operator",
+ python_callable=func,
+ dag=self.dag,
+ show_return_value_in_logs=False,
+ templates_exts=['test_ext']
+ )
+
+ assert python_operator.template_ext == ['test_ext']
Review Comment:
Last time that I checked the following `if` wasn't covered:
````python
if templates_exts:
self.template_ext = templates_exts
````
--
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] github-actions[bot] commented on pull request #27797: Add tests to PythonOperator
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1425005377
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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] shahar1 commented on pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1364649670
@uranusjr Hey, could you please help me finalize this PR? I'm having some trouble with the unit tests, probably due to the `raise ... from None` statement.
--
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 #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1328013440
> > Tests need fixing
>
> Thank you for the pushing, is there anything else required?
We will see when the tests are passing.
--
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] shahar1 commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1036356325
##########
tests/operators/test_python.py:
##########
@@ -375,6 +375,20 @@ def func():
"INFO:airflow.task.operators:Done. Returned value not shown" in cm.output
), "Log message that the option is turned off should be shown"
+ def test_python_operator_templates_exts(self):
+ def func():
+ return "test_return_value"
+
+ python_operator = PythonOperator(
+ task_id="python_operator",
+ python_callable=func,
+ dag=self.dag,
+ show_return_value_in_logs=False,
+ templates_exts=['test_ext']
+ )
+
+ assert python_operator.template_ext == ['test_ext']
Review Comment:
Last time that I checked the following `if` in the constructor wasn't covered (it might [have](https://app.codecov.io/gh/apache/airflow/tree/master/airflow/operators) since then):
````python
if templates_exts:
self.template_ext = templates_exts
````
--
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] shahar1 commented on pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1327987556
> Tests need fixing
Thank you for the pushing, is there anything else required?
--
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] shahar1 commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1038197777
##########
tests/operators/test_python.py:
##########
@@ -1155,6 +1169,21 @@ def f():
)
copy.deepcopy(task)
+ def test_except_value_error(self):
+ def f():
+ return 1
+
+ task = PythonVirtualenvOperator(
+ python_callable=f,
+ task_id="task",
+ dag=self.dag,
+ )
+
+ task.log.error = unittest.mock.Mock()
Review Comment:
I replaced `log.error` with a custom exception class, see below
--
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] shahar1 commented on pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1345175705
> Still fixes needed :(
I think the tests haven't passed until now because of other tests in the `main` branch.
In that case, do we have to wait until the other tests are ok?
@uranusjr Thank you for the fixes.
--
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] shahar1 commented on pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
shahar1 commented on PR #27797:
URL: https://github.com/apache/airflow/pull/27797#issuecomment-1345175665
> Still fixes needed :(
I think the tests haven't passed until now because of other tests in the `main` branch.
In that case, to merge this PR, do we have to wait until the other tests are ok along with this one?
@uranusjr Thank you for the kind help!
--
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] uranusjr commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1034362366
##########
tests/operators/test_python.py:
##########
@@ -1155,6 +1169,21 @@ def f():
)
copy.deepcopy(task)
+ def test_except_value_error(self):
+ def f():
+ return 1
+
+ task = PythonVirtualenvOperator(
+ python_callable=f,
+ task_id="task",
+ dag=self.dag,
+ )
+
+ task.log.error = unittest.mock.Mock()
+ task.pickling_library.loads = unittest.mock.Mock(side_effect=ValueError)
+ with pytest.raises(ValueError):
+ task._read_result(path=unittest.mock.Mock())
Review Comment:
Would be a good idea to use a custom exception class to ensure the exception is actually raised by the mocked pickling lib.
--
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] uranusjr commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1034361731
##########
tests/operators/test_python.py:
##########
@@ -1155,6 +1169,21 @@ def f():
)
copy.deepcopy(task)
+ def test_except_value_error(self):
+ def f():
+ return 1
+
+ task = PythonVirtualenvOperator(
+ python_callable=f,
+ task_id="task",
+ dag=self.dag,
+ )
+
+ task.log.error = unittest.mock.Mock()
Review Comment:
Maybe this should use `caplog` to actually capture the logs instead of mocking it.
--
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] uranusjr commented on a diff in pull request #27797: Add tests to PythonOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27797:
URL: https://github.com/apache/airflow/pull/27797#discussion_r1036761554
##########
tests/operators/test_python.py:
##########
@@ -375,6 +375,20 @@ def func():
"INFO:airflow.task.operators:Done. Returned value not shown" in cm.output
), "Log message that the option is turned off should be shown"
+ def test_python_operator_templates_exts(self):
+ def func():
+ return "test_return_value"
+
+ python_operator = PythonOperator(
+ task_id="python_operator",
+ python_callable=func,
+ dag=self.dag,
+ show_return_value_in_logs=False,
+ templates_exts=['test_ext']
+ )
+
+ assert python_operator.template_ext == ['test_ext']
Review Comment:
Oh it’s the `templates_exts` argument specific to PythonOperaotr. I wonder if it’s a good target for deprecation…
--
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