You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2024/11/06 12:08:50 UTC
[PR] Note in pytest_plugin to not global import Airflow [airflow]
uranusjr opened a new pull request, #43738:
URL: https://github.com/apache/airflow/pull/43738
What it says on the tin… otherwise tests break.
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #43738:
URL: https://github.com/apache/airflow/pull/43738#discussion_r1831549371
##########
tests_common/pytest_plugin.py:
##########
@@ -50,6 +50,23 @@
Op = TypeVar("Op", bound=BaseOperator)
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
+
+assert "airflow" not in sys.modules, (
+ "Airflow SHOULD NOT have been imported at this point! "
+ "Read comments in pytest_plugin.py to understand more."
Review Comment:
I trust future contributors to at least lightly browse through the file when they move it… (practically this probably won’t be moved that much)
Putting an absolute path here seems a bit too excessive to me
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #43738:
URL: https://github.com/apache/airflow/pull/43738#discussion_r1831408906
##########
tests_common/pytest_plugin.py:
##########
@@ -50,6 +50,23 @@
Op = TypeVar("Op", bound=BaseOperator)
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
+
+assert "airflow" not in sys.modules, (
+ "Airflow SHOULD NOT have been imported at this point! "
+ "Read comments in pytest_plugin.py to understand more."
Review Comment:
nit: Is it worth changing to `Path(__file__).absolute()` in case this file gets moe around in future?
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #43738:
URL: https://github.com/apache/airflow/pull/43738#discussion_r1831070075
##########
tests_common/pytest_plugin.py:
##########
@@ -31,6 +31,18 @@
import pytest
import time_machine
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
Review Comment:
Probably. If our test setup gets more complicated people might need some additional pointers toward these comments. This assert showing up in the error log can help. I’ll add it with a more helpful error message.
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #43738:
URL: https://github.com/apache/airflow/pull/43738#discussion_r1830983646
##########
tests_common/pytest_plugin.py:
##########
@@ -31,6 +31,18 @@
import pytest
import time_machine
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
Review Comment:
Is it worth addming a check? Something like this?
```suggestion
# imported at runtime; those imports are only hints to the type checker.)
assert "airflow" not in sys.modules
```
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43738:
URL: https://github.com/apache/airflow/pull/43738#discussion_r1831068782
##########
tests_common/pytest_plugin.py:
##########
@@ -31,6 +31,18 @@
import pytest
import time_machine
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
Review Comment:
Absolutely - we already have this in conftest.py:
```
# We should set these before loading _any_ of the rest of airflow so that the
# unit test mode config is set as early as possible.
assert "airflow" not in sys.modules, "No airflow module can be imported before these lines"
```
##########
tests_common/pytest_plugin.py:
##########
@@ -31,6 +31,18 @@
import pytest
import time_machine
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
Review Comment:
Absolutely - we already have this in conftest.py:
```python
# We should set these before loading _any_ of the rest of airflow so that the
# unit test mode config is set as early as possible.
assert "airflow" not in sys.modules, "No airflow module can be imported before these lines"
```
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #43738:
URL: https://github.com/apache/airflow/pull/43738#discussion_r1831560451
##########
tests_common/pytest_plugin.py:
##########
@@ -50,6 +50,23 @@
Op = TypeVar("Op", bound=BaseOperator)
+# NOTE: DO NOT IMPORT AIRFLOW THINGS HERE!
+#
+# This plugin is responsible for configuring Airflow correctly to run tests.
+# Importing Airflow here loads Airflow too eagerly and break the configurations.
+# Instead, import what you want lazily inside a fixture function.
+#
+# Be aware that many things in tests_common also indirectly imports Airflow, so
+# those modules also should not be imported globally.
+#
+# (Things in the TYPE_CHECKING block are fine because they are not actually
+# imported at runtime; those imports are only hints to the type checker.)
+
+assert "airflow" not in sys.modules, (
+ "Airflow SHOULD NOT have been imported at this point! "
+ "Read comments in pytest_plugin.py to understand more."
Review Comment:
🤞
--
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
Re: [PR] Note in pytest_plugin to not global import Airflow [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr merged PR #43738:
URL: https://github.com/apache/airflow/pull/43738
--
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