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/01/14 16:52:02 UTC
[GitHub] [airflow] khalidmammadov opened a new pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
khalidmammadov opened a new pull request #20881:
URL: https://github.com/apache/airflow/pull/20881
This fixes `test_sync_perm_for_dag` test case inside `test_dagbag.py` that leaves a record for a test DAG permission in the DB.
It causes failure on another test case (tests/www/test_security.py::test_verify_public_role_has_no_permissions) in the other suite when run sequentially
To reproduce the issue within Breeze:
`pytest --disable-pytest-warnings tests/models/test_dagbag.py::TestDagBag::test_sync_perm_for_dag`
then
`pytest --disable-pytest-warnings tests/www/test_security.py::test_verify_public_role_has_no_permissions`
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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/main/UPDATING.md).
--
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] khalidmammadov commented on a change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r786619947
##########
File path: tests/models/test_dagbag.py
##########
@@ -44,19 +44,20 @@
from tests.test_utils import db
from tests.test_utils.asserts import assert_queries_count
from tests.test_utils.config import conf_vars
-from tests.test_utils.permissions import delete_dag_specific_permissions
class TestDagBag:
@classmethod
def setup_class(cls):
cls.empty_dir = mkdtemp()
+ db.clear_dag_specific_permissions()
@classmethod
def teardown_class(cls):
shutil.rmtree(cls.empty_dir)
+ db.clear_dag_specific_permissions()
- def setup_methods(self) -> None:
+ def setup_method(self) -> None:
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
Review comment:
Fixed, any better?
--
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] khalidmammadov commented on a change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r786750308
##########
File path: tests/models/test_dagbag.py
##########
@@ -44,19 +44,20 @@
from tests.test_utils import db
from tests.test_utils.asserts import assert_queries_count
from tests.test_utils.config import conf_vars
-from tests.test_utils.permissions import delete_dag_specific_permissions
class TestDagBag:
@classmethod
def setup_class(cls):
cls.empty_dir = mkdtemp()
+ db.clear_dag_specific_permissions()
@classmethod
def teardown_class(cls):
shutil.rmtree(cls.empty_dir)
+ db.clear_dag_specific_permissions()
- def setup_methods(self) -> None:
+ def setup_method(self) -> None:
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
Review comment:
I looked into it and looks like cleaning after each method is not required. I have added where it's necessary to do that before starting to test and final teardown_class will clean up after once all complete.
I have run the tests and all passes with this setup.
--
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] khalidmammadov commented on a change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r786619737
##########
File path: tests/models/test_dagbag.py
##########
@@ -56,15 +55,17 @@ def setup_class(cls):
def teardown_class(cls):
shutil.rmtree(cls.empty_dir)
- def setup_methods(self) -> None:
+ def setup_method(self) -> None:
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
+ db.clear_dag_specific_permissions()
Review comment:
Fixed
--
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] khalidmammadov commented on a change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r785303071
##########
File path: tests/models/test_dagbag.py
##########
@@ -837,6 +837,7 @@ def _sync_perms():
mock_sync_perm_for_dag.assert_called_once_with(
"test_example_bash_operator", {"Public": {"can_read"}}
)
+ delete_dag_specific_permissions()
Review comment:
This suite uses unittest and hence added to setup and clear down instead. Also. moved the function `as is` into `db.py` module under `test_utils` for consistency and removed `permissions.py` as it's not required anymore. Is that OK?
--
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] jedcunningham commented on a change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r785161712
##########
File path: tests/models/test_dagbag.py
##########
@@ -837,6 +837,7 @@ def _sync_perms():
mock_sync_perm_for_dag.assert_called_once_with(
"test_example_bash_operator", {"Public": {"can_read"}}
)
+ delete_dag_specific_permissions()
Review comment:
Can you look at creating a pytest fixture for this instead? Then it'll at least run if something goes wrong in this test.
--
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 #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#issuecomment-1015073007
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r786649388
##########
File path: tests/models/test_dagbag.py
##########
@@ -44,19 +44,20 @@
from tests.test_utils import db
from tests.test_utils.asserts import assert_queries_count
from tests.test_utils.config import conf_vars
-from tests.test_utils.permissions import delete_dag_specific_permissions
class TestDagBag:
@classmethod
def setup_class(cls):
cls.empty_dir = mkdtemp()
+ db.clear_dag_specific_permissions()
@classmethod
def teardown_class(cls):
shutil.rmtree(cls.empty_dir)
+ db.clear_dag_specific_permissions()
- def setup_methods(self) -> None:
+ def setup_method(self) -> None:
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
Review comment:
Oops, now it’s not cleaning up enough. Since the tests trigger DAGs, cleanup should still happen in `teardown_method`, not `teardown_class`. This way, cleanup happens once before any tests a run (`setup_class`), and once after _each_ test (`teardown_method`).
--
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 #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20881:
URL: https://github.com/apache/airflow/pull/20881
--
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 change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r786416813
##########
File path: tests/models/test_dagbag.py
##########
@@ -44,19 +44,20 @@
from tests.test_utils import db
from tests.test_utils.asserts import assert_queries_count
from tests.test_utils.config import conf_vars
-from tests.test_utils.permissions import delete_dag_specific_permissions
class TestDagBag:
@classmethod
def setup_class(cls):
cls.empty_dir = mkdtemp()
+ db.clear_dag_specific_permissions()
@classmethod
def teardown_class(cls):
shutil.rmtree(cls.empty_dir)
+ db.clear_dag_specific_permissions()
- def setup_methods(self) -> None:
+ def setup_method(self) -> None:
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
Review comment:
I actually meant this entire function can be merged into `setup_class`. Sorry for not being clear. The current change is OK if we’re to minimise the changed lines, although I’d still prefer we changing this to make the test suite ever so slightly faster.
--
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 change in pull request #20881: Fix a test case inside tests/models that leaves a trace in the DB
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20881:
URL: https://github.com/apache/airflow/pull/20881#discussion_r785638785
##########
File path: tests/models/test_dagbag.py
##########
@@ -56,15 +55,17 @@ def setup_class(cls):
def teardown_class(cls):
shutil.rmtree(cls.empty_dir)
- def setup_methods(self) -> None:
+ def setup_method(self) -> None:
db.clear_db_dags()
db.clear_db_runs()
db.clear_db_serialized_dags()
+ db.clear_dag_specific_permissions()
Review comment:
Clearing things before every test is too exccessive. It’s probably better to change this to `setup_class` instead.
--
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