gemini-code-assist[bot] commented on code in PR #38566: URL: https://github.com/apache/beam/pull/38566#discussion_r3274482435
########## sdks/python/apache_beam/utils/subprocess_server_test.py: ########## @@ -37,6 +37,23 @@ class JavaJarServerTest(unittest.TestCase): + + # TODO(https://github.com/grpc/grpc/issues/37710): Remove once fixed. + @classmethod + def setUpClass(cls): + cls._old_fork_support = os.environ.get('GRPC_ENABLE_FORK_SUPPORT') Review Comment:  The `os` module is used here but it doesn't appear to be imported in this file. While it was added to `portable_runner_test.py` in this PR, it seems to be missing here, which will cause a `NameError` during test execution. Please add `import os` at the top of the file. ########## sdks/python/apache_beam/utils/subprocess_server_test.py: ########## @@ -37,6 +37,23 @@ class JavaJarServerTest(unittest.TestCase): + + # TODO(https://github.com/grpc/grpc/issues/37710): Remove once fixed. + @classmethod + def setUpClass(cls): + cls._old_fork_support = os.environ.get('GRPC_ENABLE_FORK_SUPPORT') + os.environ['GRPC_ENABLE_FORK_SUPPORT'] = 'false' + super().setUpClass() + + # TODO(https://github.com/grpc/grpc/issues/37710): Remove once fixed. + @classmethod + def tearDownClass(cls): + if cls._old_fork_support is None: + os.environ.pop('GRPC_ENABLE_FORK_SUPPORT', None) + else: + os.environ['GRPC_ENABLE_FORK_SUPPORT'] = cls._old_fork_support + super().tearDownClass() Review Comment:  Consider using `addClassCleanup` here as well to ensure the environment variable is restored even if `setUpClass` fails. This also allows removing the redundant `tearDownClass` and the duplicated TODO comment, improving maintainability. ```python @classmethod def setUpClass(cls): old_fork_support = os.environ.get('GRPC_ENABLE_FORK_SUPPORT') os.environ['GRPC_ENABLE_FORK_SUPPORT'] = 'false' def restore(): if old_fork_support is None: os.environ.pop('GRPC_ENABLE_FORK_SUPPORT', None) else: os.environ['GRPC_ENABLE_FORK_SUPPORT'] = old_fork_support cls.addClassCleanup(restore) super().setUpClass() ``` ########## sdks/python/apache_beam/runners/portability/portable_runner_test.py: ########## @@ -328,6 +329,22 @@ class PortableRunnerTestWithSubprocessesAndMultiWorkers( PortableRunnerTestWithSubprocesses): _use_subprocesses = True + # TODO(https://github.com/grpc/grpc/issues/37710): Remove once fixed. + @classmethod + def setUpClass(cls): + cls._old_fork_support = os.environ.get('GRPC_ENABLE_FORK_SUPPORT') + os.environ['GRPC_ENABLE_FORK_SUPPORT'] = 'false' + super().setUpClass() + + # TODO(https://github.com/grpc/grpc/issues/37710): Remove once fixed. + @classmethod + def tearDownClass(cls): + if cls._old_fork_support is None: + os.environ.pop('GRPC_ENABLE_FORK_SUPPORT', None) + else: + os.environ['GRPC_ENABLE_FORK_SUPPORT'] = cls._old_fork_support + super().tearDownClass() Review Comment:  Using `addClassCleanup` is more robust than manual `tearDownClass` because it ensures the environment variable is restored even if `setUpClass` (or a parent's `setUpClass`) fails. This prevents side effects on other test classes running in the same process and reduces boilerplate by removing the redundant `tearDownClass` method. ```python @classmethod def setUpClass(cls): old_fork_support = os.environ.get('GRPC_ENABLE_FORK_SUPPORT') os.environ['GRPC_ENABLE_FORK_SUPPORT'] = 'false' def restore(): if old_fork_support is None: os.environ.pop('GRPC_ENABLE_FORK_SUPPORT', None) else: os.environ['GRPC_ENABLE_FORK_SUPPORT'] = old_fork_support cls.addClassCleanup(restore) super().setUpClass() ``` -- 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]
