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 2021/03/08 14:16:14 UTC
[GitHub] [airflow] cccs-seb opened a new pull request #14663: Add tests for python & pythonvenv operators execute method.
cccs-seb opened a new pull request #14663:
URL: https://github.com/apache/airflow/pull/14663
See original PR (https://github.com/apache/airflow/pull/14061)
Add some tests to make sure the PythonOperator & PythonVirtualenvOperator return their execute method results. Had to make some slight modifications to be able to unit test the execute method in the PythonVirtualenvOperator.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cccs-seb commented on a change in pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
cccs-seb commented on a change in pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#discussion_r596870934
##########
File path: airflow/operators/python.py
##########
@@ -489,7 +489,9 @@ def __init__( # pylint: disable=too-many-arguments
self.pickling_library = dill if self.use_dill else pickle
def execute(self, context: Dict):
- serializable_context = {key: context[key] for key in self._get_serializable_context_keys()}
+ serializable_context = {
+ key: context[key] for key in self._get_serializable_context_keys() if context.get(key) is not None
+ }
Review comment:
Great idea - I'll make the change as soon as I can. Thanks for the suggestion!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #14663:
URL: https://github.com/apache/airflow/pull/14663
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cccs-seb commented on a change in pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
cccs-seb commented on a change in pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#discussion_r590796257
##########
File path: airflow/operators/python.py
##########
@@ -489,7 +489,9 @@ def __init__( # pylint: disable=too-many-arguments
self.pickling_library = dill if self.use_dill else pickle
def execute(self, context: Dict):
- serializable_context = {key: context[key] for key in self._get_serializable_context_keys()}
+ serializable_context = {
+ key: context[key] for key in self._get_serializable_context_keys() if context.get(key) is not None
+ }
Review comment:
With the previous code, if I passed an empty context object to test the execute method, it would throw a KeyError when it tries to do `key: context[key]`. Essentially, if I can't provide a context object with all the keys from _get_serializable_context_keys, an KeyError will be thrown.
Most operator execute method tests I have seen use an empty context object as param to test. I tried to do the same here which required the change above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cccs-seb commented on pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
cccs-seb commented on pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#issuecomment-792785954
@turbaszek Pinging you as this PR is a follow-up to your request [here](https://github.com/apache/airflow/pull/14061#issuecomment-773099544). Let me know if I can improve anything!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#issuecomment-808832547
[The Workflow run](https://github.com/apache/airflow/actions/runs/694064919) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#discussion_r593031989
##########
File path: airflow/operators/python.py
##########
@@ -489,7 +489,9 @@ def __init__( # pylint: disable=too-many-arguments
self.pickling_library = dill if self.use_dill else pickle
def execute(self, context: Dict):
- serializable_context = {key: context[key] for key in self._get_serializable_context_keys()}
+ serializable_context = {
+ key: context[key] for key in self._get_serializable_context_keys() if context.get(key) is not None
+ }
Review comment:
@cccs-seb have you tried using `mock.MagicMock()` as context? In this way we probably won't need this change and we can do even more - for example assert that some values from the context were accessed. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#issuecomment-840905285
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #14663: Add tests for python & pythonvenv operators execute method.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #14663:
URL: https://github.com/apache/airflow/pull/14663#discussion_r590707502
##########
File path: airflow/operators/python.py
##########
@@ -489,7 +489,9 @@ def __init__( # pylint: disable=too-many-arguments
self.pickling_library = dill if self.use_dill else pickle
def execute(self, context: Dict):
- serializable_context = {key: context[key] for key in self._get_serializable_context_keys()}
+ serializable_context = {
+ key: context[key] for key in self._get_serializable_context_keys() if context.get(key) is not None
+ }
Review comment:
Can you explain this change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org