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/04/01 17:25:39 UTC
[GitHub] [airflow] uranusjr opened a new pull request #15141: Share app instance between Kerberos tests
uranusjr opened a new pull request #15141:
URL: https://github.com/apache/airflow/pull/15141
Continuing #14878. I don’t have Kerberos set up to run the tests, but this should save a slight bit of time.
--
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] ashb commented on a change in pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#discussion_r612378895
##########
File path: tests/api/auth/backend/test_kerberos_auth.py
##########
@@ -19,42 +19,46 @@
import json
import os
import socket
-import unittest
from datetime import datetime
from unittest import mock
import pytest
from airflow.api.auth.backend.kerberos_auth import CLIENT_AUTH
from airflow.models import DagBag
-from airflow.www import app as application
+from airflow.www import app
from tests.test_utils.config import conf_vars
from tests.test_utils.db import clear_db_dags
KRB5_KTNAME = os.environ.get("KRB5_KTNAME")
-@pytest.mark.integration("kerberos")
-class TestApiKerberos(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
- dagbag = DagBag(include_examples=True)
- for dag in dagbag.dags.values():
- dag.sync_to_db()
-
- @conf_vars(
+@pytest.fixture(scope="module")
+def app_for_kerberos():
+ with conf_vars(
{
("api", "auth_backend"): "airflow.api.auth.backend.kerberos_auth",
("kerberos", "keytab"): KRB5_KTNAME,
('api', 'enable_experimental_api'): 'true',
}
- )
- def setUp(self):
- self.app = application.create_app(testing=True)
+ ):
+ yield app.create_app(testing=True)
+
+
+@pytest.fixture(scope="module", autouse=True)
+def dagbag_to_db():
+ dagbag = DagBag(include_examples=True)
+ for dag in dagbag.dags.values():
+ dag.sync_to_db()
+ yield
+ clear_db_dags()
- @classmethod
- def tearDownClass(cls) -> None:
- clear_db_dags()
+
+@pytest.mark.integration("kerberos")
+class TestApiKerberos:
+ @pytest.fixture(autouse=True)
+ def _set_attrs(self, app_for_kerberos):
Review comment:
Again here, because of `autouse=True` the name doesn't matter -- pytest will ensure that it is called before each test function.
##########
File path: tests/api/auth/backend/test_kerberos_auth.py
##########
@@ -19,42 +19,46 @@
import json
import os
import socket
-import unittest
from datetime import datetime
from unittest import mock
import pytest
from airflow.api.auth.backend.kerberos_auth import CLIENT_AUTH
from airflow.models import DagBag
-from airflow.www import app as application
+from airflow.www import app
from tests.test_utils.config import conf_vars
from tests.test_utils.db import clear_db_dags
KRB5_KTNAME = os.environ.get("KRB5_KTNAME")
-@pytest.mark.integration("kerberos")
-class TestApiKerberos(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
- dagbag = DagBag(include_examples=True)
- for dag in dagbag.dags.values():
- dag.sync_to_db()
-
- @conf_vars(
+@pytest.fixture(scope="module")
+def app_for_kerberos():
+ with conf_vars(
{
("api", "auth_backend"): "airflow.api.auth.backend.kerberos_auth",
("kerberos", "keytab"): KRB5_KTNAME,
('api', 'enable_experimental_api'): 'true',
}
- )
- def setUp(self):
- self.app = application.create_app(testing=True)
+ ):
+ yield app.create_app(testing=True)
+
+
+@pytest.fixture(scope="module", autouse=True)
+def dagbag_to_db():
Review comment:
`autouse=True` means PyTest will automatically make sure that this function is called once per module
--
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] ashb merged pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15141:
URL: https://github.com/apache/airflow/pull/15141
--
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 #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#issuecomment-821206131
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#issuecomment-821223370
`./breeze -i kerberos` should give you a docker env with kerberos set up I think.
From CI on this branch:
```
2021-04-01T20:33:12.0023083Z 18.85s setup tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
...
2021-04-01T20:33:12.0048850Z 0.09s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
...
2021-04-01T20:33:12.0055405Z 0.01s teardown tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_unauthorized
...
2021-04-01T20:33:12.0062038Z 0.01s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_unauthorized
2021-04-01T20:33:12.0099990Z =========== 37 passed, 8032 skipped, 8 warnings in 185.60s (0:03:05) ===========
```
Compared to on master:
```
31.00s setup tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
25.11s call tests/executors/test_celery_executor.py::TestCeleryExecutor::test_celery_integration_1_redis_redis_6379_0
20.30s call tests/executors/test_celery_executor.py::TestCeleryExecutor::test_celery_integration_0_amqp_guest_guest_rabbitmq_5672
10.97s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
7.16s call tests/providers/trino/hooks/test_trino.py::TestTrinoHookIntegration::test_should_record_records
6.03s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_unauthorized
=========== 37 passed, 8179 skipped, 8 warnings in 287.80s (0:04:47) ===========
```
So saves about a minute for the "integration" set of tests.
We probably need to optimize the "slow" path next -- which ever of the test groups on master is the slowest -- since they are run in parallel one test type being quicker might not help anymore if we are always waiting on a slow one.
--
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] uranusjr commented on pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#issuecomment-821251521
Thanks, I'll learn that. The gain is actually much more significant than I anticipated!
--
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] uranusjr commented on pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#issuecomment-821208750
No since I haven't figured out a way to set up Kerberos to run these tests locally 😟
--
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] uranusjr commented on pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#issuecomment-812288990
Both `TestApiKerberos._set_attrs` and `dagbag_to_db` are [autouse fixtures](https://docs.pytest.org/en/stable/fixture.html#autouse-fixtures-fixtures-you-don-t-have-to-request), which means they are loaded automatically for every test in `test_kerberos_auth.py`.
`_set_attrs` here uses the default (`function`) scope, which means it is wrapped around every test. The part before `yield` is like `setUp`, and the part after `tearDown` for `unittest.TestCase`. `dagbag_to_db` is `module`-scoped, so the code is automatically executed once for all tests in this module, in other words like `setUpClass` and `tearDownClass` but for the entire module.
The only performance improvements in this PR comes from `app_for_kerberos`, which is `module`-scoped and therefore creates the Flask app instance only once for this module, instead of once for every test. But these autouse fixtures need to be created from the TestCase setup/teardown equivalents since `unittest.TestCase` subclasses cannot load pytest fixtures.
--
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] jhtimmins commented on a change in pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#discussion_r606018061
##########
File path: tests/api/auth/backend/test_kerberos_auth.py
##########
@@ -19,42 +19,46 @@
import json
import os
import socket
-import unittest
from datetime import datetime
from unittest import mock
import pytest
from airflow.api.auth.backend.kerberos_auth import CLIENT_AUTH
from airflow.models import DagBag
-from airflow.www import app as application
+from airflow.www import app
from tests.test_utils.config import conf_vars
from tests.test_utils.db import clear_db_dags
KRB5_KTNAME = os.environ.get("KRB5_KTNAME")
-@pytest.mark.integration("kerberos")
-class TestApiKerberos(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
- dagbag = DagBag(include_examples=True)
- for dag in dagbag.dags.values():
- dag.sync_to_db()
-
- @conf_vars(
+@pytest.fixture(scope="module")
+def app_for_kerberos():
+ with conf_vars(
{
("api", "auth_backend"): "airflow.api.auth.backend.kerberos_auth",
("kerberos", "keytab"): KRB5_KTNAME,
('api', 'enable_experimental_api'): 'true',
}
- )
- def setUp(self):
- self.app = application.create_app(testing=True)
+ ):
+ yield app.create_app(testing=True)
+
+
+@pytest.fixture(scope="module", autouse=True)
+def dagbag_to_db():
Review comment:
It doesn't look like `dagbag_to_db` is getting used; does it have a purpose that's just not clear to me?
##########
File path: tests/api/auth/backend/test_kerberos_auth.py
##########
@@ -19,42 +19,46 @@
import json
import os
import socket
-import unittest
from datetime import datetime
from unittest import mock
import pytest
from airflow.api.auth.backend.kerberos_auth import CLIENT_AUTH
from airflow.models import DagBag
-from airflow.www import app as application
+from airflow.www import app
from tests.test_utils.config import conf_vars
from tests.test_utils.db import clear_db_dags
KRB5_KTNAME = os.environ.get("KRB5_KTNAME")
-@pytest.mark.integration("kerberos")
-class TestApiKerberos(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
- dagbag = DagBag(include_examples=True)
- for dag in dagbag.dags.values():
- dag.sync_to_db()
-
- @conf_vars(
+@pytest.fixture(scope="module")
+def app_for_kerberos():
+ with conf_vars(
{
("api", "auth_backend"): "airflow.api.auth.backend.kerberos_auth",
("kerberos", "keytab"): KRB5_KTNAME,
('api', 'enable_experimental_api'): 'true',
}
- )
- def setUp(self):
- self.app = application.create_app(testing=True)
+ ):
+ yield app.create_app(testing=True)
+
+
+@pytest.fixture(scope="module", autouse=True)
+def dagbag_to_db():
+ dagbag = DagBag(include_examples=True)
+ for dag in dagbag.dags.values():
+ dag.sync_to_db()
+ yield
+ clear_db_dags()
- @classmethod
- def tearDownClass(cls) -> None:
- clear_db_dags()
+
+@pytest.mark.integration("kerberos")
+class TestApiKerberos:
+ @pytest.fixture(autouse=True)
+ def _set_attrs(self, app_for_kerberos):
Review comment:
I'm not familiar with `_set_attrs`, where is this from?
--
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] ashb edited a comment on pull request #15141: Share app instance between Kerberos tests
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #15141:
URL: https://github.com/apache/airflow/pull/15141#issuecomment-821223370
`./breeze -i kerberos` should give you a docker env with kerberos set up I think.
From CI on this branch:
```
18.85s setup tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
...
0.09s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
...
0.01s teardown tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_unauthorized
...
0.01s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_unauthorized
=========== 37 passed, 8032 skipped, 8 warnings in 185.60s (0:03:05) ===========
```
Compared to on master:
```
31.00s setup tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
25.11s call tests/executors/test_celery_executor.py::TestCeleryExecutor::test_celery_integration_1_redis_redis_6379_0
20.30s call tests/executors/test_celery_executor.py::TestCeleryExecutor::test_celery_integration_0_amqp_guest_guest_rabbitmq_5672
10.97s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_trigger_dag
7.16s call tests/providers/trino/hooks/test_trino.py::TestTrinoHookIntegration::test_should_record_records
6.03s call tests/api/auth/backend/test_kerberos_auth.py::TestApiKerberos::test_unauthorized
=========== 37 passed, 8179 skipped, 8 warnings in 287.80s (0:04:47) ===========
```
So saves about a minute for the "integration" set of tests.
We probably need to optimize the "slow" path next -- which ever of the test groups on master is the slowest -- since they are run in parallel one test type being quicker might not help anymore if we are always waiting on a slow one.
--
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