kaxil commented on code in PR #73262: URL: https://github.com/apache/airflow/pull/73262#discussion_r4032658748
########## providers/common/compat/tests/unit/common/compat/standard/test_operators.py: ########## @@ -0,0 +1,95 @@ +# 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. +from __future__ import annotations + +import functools + +import pytest + +from airflow.providers.common.compat.standard import operators + +from tests_common.test_utils.version_compat import AIRFLOW_V_3_2_PLUS + +EXPECTED_EXPORTS = ( + "BaseAsyncOperator", + "BaseBranchOperator", + "BaseOperator", + "BranchMixIn", + "PythonOperator", + "ShortCircuitOperator", + "_SERIALIZERS", + "get_current_context", + "is_async_callable", +) + + +async def async_function(): + """Sample coroutine function.""" + + +def sync_function(): + """Sample plain function.""" + + +def test_public_exports(): + assert set(operators.__all__) == set(EXPECTED_EXPORTS) + + [email protected]("name", EXPECTED_EXPORTS) +def test_all_compat_imports_work(name): + assert getattr(operators, name) is not None + + +def test_invalid_import_raises_attribute_error(): Review Comment: This duplicates `test__compat_utils.py::TestCreateModuleGetattr::test_attribute_error_for_unknown_name`, which asserts the same `module has no attribute '...'` message against the same helper. The message is raised by `create_module_getattr`, not by either shim, so a copy per shim module adds a test without adding coverage. Please drop it here and in `test_triggers.py`. ########## providers/common/compat/tests/unit/common/compat/standard/test_operators.py: ########## @@ -0,0 +1,95 @@ +# 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. +from __future__ import annotations + +import functools + +import pytest + +from airflow.providers.common.compat.standard import operators + +from tests_common.test_utils.version_compat import AIRFLOW_V_3_2_PLUS + +EXPECTED_EXPORTS = ( + "BaseAsyncOperator", + "BaseBranchOperator", + "BaseOperator", + "BranchMixIn", + "PythonOperator", + "ShortCircuitOperator", + "_SERIALIZERS", + "get_current_context", + "is_async_callable", +) + + +async def async_function(): + """Sample coroutine function.""" + + +def sync_function(): + """Sample plain function.""" + + +def test_public_exports(): + assert set(operators.__all__) == set(EXPECTED_EXPORTS) + + [email protected]("name", EXPECTED_EXPORTS) +def test_all_compat_imports_work(name): Review Comment: `getattr(operators, name)` does not reach `__getattr__` for `BaseAsyncOperator` or `is_async_callable`. Both branches of the `AIRFLOW_V_3_2_PLUS` gate in `standard/operators.py` bind those two names at module level, so normal attribute lookup succeeds and the `_IMPORT_MAP` entry is never consulted. You flagged this in the description and offered a follow-up. I would rather it landed here: the two entries are dead on every Airflow version, and they are the only reason those names appear in `__all__`, which is `sorted(_IMPORT_MAP.keys())`. Confirmed on 3.4.0 by calling the module `__getattr__` directly: ``` BaseAsyncOperator module-level bound: True BaseAsyncOperator via __getattr__: ImportError Could not import 'BaseAsyncOperator' from any of: ('airflow.providers.common.compat.sdk',) is_async_callable via __getattr__: ImportError Could not import 'is_async_callable' from any of: ('airflow.providers.common.compat.sdk',) PythonOperator via __getattr__: <class 'airflow.providers.standard.operators.python.PythonOperator'> ``` `sdk.py` exports neither name. Suggested fix in `standard/operators.py`: drop both keys from `_IMPORT_MAP`, and declare `__all__` explicitly so the two names stay exported. That also makes this test earn its place. Once the map holds only names that route through the fallback chain, asserting each one resolves does exercise `create_module_getattr`, which is the coverage the compatibility jobs need and nothing has today. ########## providers/common/compat/tests/unit/common/compat/standard/test_operators.py: ########## @@ -0,0 +1,95 @@ +# 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. +from __future__ import annotations + +import functools + +import pytest + +from airflow.providers.common.compat.standard import operators + +from tests_common.test_utils.version_compat import AIRFLOW_V_3_2_PLUS + +EXPECTED_EXPORTS = ( + "BaseAsyncOperator", + "BaseBranchOperator", + "BaseOperator", + "BranchMixIn", + "PythonOperator", + "ShortCircuitOperator", + "_SERIALIZERS", + "get_current_context", + "is_async_callable", +) + + +async def async_function(): + """Sample coroutine function.""" + + +def sync_function(): + """Sample plain function.""" + + +def test_public_exports(): + assert set(operators.__all__) == set(EXPECTED_EXPORTS) + + [email protected]("name", EXPECTED_EXPORTS) +def test_all_compat_imports_work(name): + assert getattr(operators, name) is not None + + +def test_invalid_import_raises_attribute_error(): + with pytest.raises(AttributeError, match="module has no attribute 'NonExistentClass'"): + _ = operators.NonExistentClass + + [email protected]( + ("func", "expected"), + [ + pytest.param(async_function, True, id="coroutine-function"), + pytest.param(sync_function, False, id="plain-function"), + pytest.param(functools.partial(async_function), True, id="partial-of-coroutine-function"), + pytest.param(functools.partial(sync_function), False, id="partial-of-plain-function"), + pytest.param( + functools.partial(functools.partial(async_function)), + True, + id="nested-partial-of-coroutine-function", + ), + ], +) +def test_is_async_callable(func, expected): + """ + Coroutine functions are detected through any number of ``functools.partial`` wrappers. + + These cases hold on both sides of the Airflow 3.2 fork: the local stub unwraps partials in a + loop, and the real implementation does the same through ``unwrap_callable``. + """ + assert operators.is_async_callable(func) is expected + + [email protected](AIRFLOW_V_3_2_PLUS, reason="The BaseAsyncOperator stub only exists on Airflow < 3.2") +class TestBaseAsyncOperatorStub: + def test_is_async(self): Review Comment: Keep this as is if you trim elsewhere. The stub is defined only in `standard/operators.py`, only on the pre-3.2 branch of the version gate, so nothing else in the tree covers it and the compatibility jobs are the only place it runs. ########## providers/common/compat/tests/unit/common/compat/standard/test_triggers.py: ########## @@ -0,0 +1,37 @@ +# 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. +from __future__ import annotations + +import pytest + +from airflow.providers.common.compat.standard import triggers + +EXPECTED_EXPORTS = ("TimeDeltaTrigger",) + + +def test_public_exports(): + assert set(triggers.__all__) == set(EXPECTED_EXPORTS) + + [email protected]("name", EXPECTED_EXPORTS) +def test_all_compat_imports_work(name): + assert getattr(triggers, name) is not None + + +def test_invalid_import_raises_attribute_error(): Review Comment: Same as the note on `test_operators.py`: `test__compat_utils.py::TestCreateModuleGetattr::test_attribute_error_for_unknown_name` already covers this. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
