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 2020/02/14 08:05:17 UTC

[GitHub] [airflow] mik-laj opened a new pull request #7419: [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs

mik-laj opened a new pull request #7419:  [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419
 
 
   When DAG is loaded, simple checks are performed, e.g. whether python code syntactically correct, whether valid arguments have been passed. 
   
   This allows us to check the initial correctness of DAGs at a low cost.
   
   ---
   Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [ ] Description above provides context of the change
   - [ ] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
   - [ ] Unit tests coverage for changes (not needed for documentation changes)
   - [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [ ] Relevant documentation is updated including usage instructions.
   - [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   <sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7419: [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7419:  [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419#discussion_r380344608
 
 

 ##########
 File path: tests/test_example_dags.py
 ##########
 @@ -0,0 +1,41 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import os
+import unittest
+from glob import glob
+
+from airflow.models import DagBag
+
 
 Review comment:
   We had a long discussion about pytest and parameterized. https://lists.apache.org/thread.html/1e4df7d4b0cd9b2d2bb76a3336471aa85e852545dd41ada6d4e461b8%40%3Cdev.airflow.apache.org%3E
   
   The use of subtest is more nicely displayed in the IDE.
   ![Screenshot 2020-02-17 at 20 38 16](https://user-images.githubusercontent.com/12058428/74682183-a06b1180-51c5-11ea-98e1-7582353450de.png)
   Using parameterized will fake us the number of tests, because suddenly the number of tests will increase by 80. If it's really not a few tests, but only one. I also don't like parameterized when generating a list is expensive. Here it is necessary to walk through a large number of directories to create these tests. 
   
   I don't see the benefit of changing the place where this message is to be displayed. On the other hand, the lack of mock makes this test easier to understand.
   
   
   
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on a change in pull request #7419: [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs

Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7419:  [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419#discussion_r379340229
 
 

 ##########
 File path: tests/test_project_structure.py
 ##########
 @@ -251,3 +250,20 @@ def find_resource_files(department="*", resource_type="*", service="*"):
         # Exclude __init__.py and pycache
         resource_files = (f for f in resource_files if not f.endswith("__init__.py"))
         return resource_files
+
+
+class TestOperatorsHooks(unittest.TestCase):
+    def test_some_suffix_is_illegal(self):
 
 Review comment:
   ```suggestion
       def test_no_illegal_suffixes(self):
             illegal_suffixes = ["_operator.py", "_hook.py", "_sensor.py"]
   ```
   WDYT? Will it make the test easier to understand?

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj merged pull request #7419: [AIRFLOW-6804] Add the basic test for all example DAGs

Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #7419: [AIRFLOW-6804] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419
 
 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] yuqian90 commented on a change in pull request #7419: [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs

Posted by GitBox <gi...@apache.org>.
yuqian90 commented on a change in pull request #7419:  [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419#discussion_r379827389
 
 

 ##########
 File path: airflow/providers/qubole/example_dags/example_qubole.py
 ##########
 @@ -51,6 +42,18 @@
     schedule_interval=None,
     tags=['example'],
 ) as dag:
+    dag.doc_md = textwrap.dedent(
 
 Review comment:
   Why not keep the original docstr, but make this line ``dag.doc_md = __doc__``. Same for other dags changes.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7419: [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7419:  [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419#discussion_r380403509
 
 

 ##########
 File path: airflow/providers/qubole/example_dags/example_qubole.py
 ##########
 @@ -51,6 +42,18 @@
     schedule_interval=None,
     tags=['example'],
 ) as dag:
+    dag.doc_md = textwrap.dedent(
 
 Review comment:
   Because now this file contains two DAGs, so they contain two different documentation. 

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [airflow] yuqian90 commented on a change in pull request #7419: [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs

Posted by GitBox <gi...@apache.org>.
yuqian90 commented on a change in pull request #7419:  [AIRFLOW-6804][depends on AIRFLOW-6792] Add the basic test for all example DAGs
URL: https://github.com/apache/airflow/pull/7419#discussion_r379827780
 
 

 ##########
 File path: tests/test_example_dags.py
 ##########
 @@ -0,0 +1,41 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import os
+import unittest
+from glob import glob
+
+from airflow.models import DagBag
+
 
 Review comment:
   For new tests, did you consider using `pytest` instead of `unittest`? This test can be a simple function with `pytest`. With `pytest.mark.paraemtrize`, individual modules can be created as sub tests automatically. Something like this will do. Having the relative_filepath in the test name (instead of full path) is just to make sure test names don't change.
   
   One other suggestion you may consider is to mock `log.exception()` to raise error (instead of counting `import_errors`). That way, when a DAG fails to import and `log.exception()` is called inside dagbag.py, the stack trace is printed immediately by pytest instead of printing the exception to the terminal output together with other output.
   
   ```python
   import os
   import pytest
   
   @pytest.mark.parametrize("relative_filepath", [os.realpath(p, ROOT_FOLDER) 
                                                  for p in glob(f"{ROOT_FOLDER}/airflow/**/example_dags/example_*.py", 
                                                                recursive=True))])
   def test_should_be_importable(relative_filepath):
       dagbag = DagBag(
           dag_folder=os.path.join(ROOT_FOLDER, relative_filepath),
           executor=None,
           include_examples=False,
       )
       assert len(dagbag.import_errors) == 0, f"import_errors={str(dagbag.import_errors)}" 
       assert len(dagbag.dag_ids) >= 1, f"{relative_filepath} should contain dags"
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services