deepyaman commented on code in PR #29061: URL: https://github.com/apache/flink/pull/29061#discussion_r3928289698
########## flink-python/pyflink/util/tests/test_api_stability_decorators.py: ########## @@ -0,0 +1,443 @@ +################################################################################ +# 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 abc +import enum +import inspect +import os +import subprocess +import sys +import textwrap +import unittest +import warnings + +from pyflink.util.api_stability_decorators import ( + Deprecated, + Experimental, + Internal, + Public, + PublicEvolving, +) + + +class DeprecatedTests(unittest.TestCase): + """ + Tests for the :class:`Deprecated` decorator, which must warn when a deprecated API is + used, and not when it is defined. + + Blocks that must not warn turn warnings into errors, so that one fails where it is + raised rather than in a comparison afterwards. + """ + + def test_decoration_does_not_warn(self): + with warnings.catch_warnings(): + warnings.simplefilter("error") + + @Deprecated(since="1.0.0", detail="Use :func:`new_func` instead.") + def func(): + pass + + @Deprecated(since="1.0.0") + class Cls: + def __init__(self): + pass + + def test_importing_pyflink_table_does_not_warn(self): + # A fresh interpreter is the only way to observe an import: pyflink.table is + # already in sys.modules here, so importing it again is a no-op. Only this + # decorator's own warnings are inspected, so third-party noise cannot fail it. Review Comment: I tried all three before answering, and the two lightweight ones are unfortunately vacuous — they pass with the bug present. Against the original, unfixed decorator (which warned four times on `import pyflink.table`): ``` A. del sys.modules['pyflink.table'] + import : no warnings seen B. importlib.reload(pyflink.table) : no warnings seen C. purge every pyflink.* + import : ['TableSchema has been deprecated...', 'TableResult.get_table_schema...', 'Table.get_schema...', 'TableEnvironment.register_catalog...'] ``` A and B miss it because the decorators do not live in `pyflink/table/__init__.py`. They run in `table_schema.py`, `table.py`, `table_result.py` and friends, and those stay in `sys.modules`, so re-importing (or reloading) the package re-runs only the `__init__` and re-binds names that are already built. Nothing is re-decorated, so there is nothing to observe. A test written that way would go green whether or not the bug is there. C does work, but it costs the rest of the process: ``` Deprecated class identity preserved : False TableSchema identity preserved : False {Deprecated} == {reimported Deprecated} ? False ``` Everything imported afterwards is a *different class object* from what the already-imported test modules hold. That matters concretely here: `PythonAPICompletenessTestCase.check_stability_decorators` compares `__stability_decorators` against imported decorator classes by identity, so a purge in a shared interpreter is exactly the kind of thing that makes an unrelated test fail later, in a way that is miserable to debug. Within this module it would also leave the other 23 tests holding the pre-purge classes while `sys.modules` holds new ones. The subprocess has none of that: a clean interpreter, no shared state to corrupt, and it fails on the unfixed decorator as it should. It is also what CPython's own suite does for import-time behaviour (`test.support.script_helper`). Happy to switch if you would still rather avoid the subprocess — but I would want it to be option C plus something like `pytest-forked`, not A or B, since those cannot see the regression at all. -- 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]
