tvalentyn commented on code in PR #28564: URL: https://github.com/apache/beam/pull/28564#discussion_r1483765959
########## sdks/python/apache_beam/transforms/environments_test.py: ########## @@ -21,20 +21,40 @@ # pytype: skip-file import logging +import tempfile import unittest from apache_beam.options.pipeline_options import PortableOptions +from apache_beam.options.pipeline_options import SetupOptions from apache_beam.portability import common_urns from apache_beam.runners import pipeline_context +from apache_beam.runners.portability import stager from apache_beam.transforms import environments from apache_beam.transforms.environments import DockerEnvironment from apache_beam.transforms.environments import EmbeddedPythonEnvironment from apache_beam.transforms.environments import EmbeddedPythonGrpcEnvironment from apache_beam.transforms.environments import Environment from apache_beam.transforms.environments import ExternalEnvironment from apache_beam.transforms.environments import ProcessEnvironment +from apache_beam.transforms.environments import PyPIArtifactRegistry from apache_beam.transforms.environments import SubprocessSDKEnvironment +# create a temp directory so that all artifacts are put in the same directory. +tmp_dir = tempfile.mkdtemp() + + +def mock_python_sdk_dependencies(options, tmp_dir=tmp_dir): Review Comment: Would mocking tempfile.mkdtemp() be sufficient? In this case we avoid maintaining a copy of a larger fuction. (optionally), see: https://docs.python.org/3/library/unittest.mock.html - it provides decorators that allows applying and reverting mocks. ########## sdks/python/apache_beam/transforms/environments_test.py: ########## @@ -21,20 +21,40 @@ # pytype: skip-file import logging +import tempfile import unittest from apache_beam.options.pipeline_options import PortableOptions +from apache_beam.options.pipeline_options import SetupOptions from apache_beam.portability import common_urns from apache_beam.runners import pipeline_context +from apache_beam.runners.portability import stager from apache_beam.transforms import environments from apache_beam.transforms.environments import DockerEnvironment from apache_beam.transforms.environments import EmbeddedPythonEnvironment from apache_beam.transforms.environments import EmbeddedPythonGrpcEnvironment from apache_beam.transforms.environments import Environment from apache_beam.transforms.environments import ExternalEnvironment from apache_beam.transforms.environments import ProcessEnvironment +from apache_beam.transforms.environments import PyPIArtifactRegistry from apache_beam.transforms.environments import SubprocessSDKEnvironment +# create a temp directory so that all artifacts are put in the same directory. +tmp_dir = tempfile.mkdtemp() Review Comment: I'd move this inside some TestClass.setUp(). Also consider `class tempfile.TemporaryDirectory` instead - then it will also clean up after the test. ########## sdks/python/apache_beam/runners/dataflow/dataflow_runner_test.py: ########## @@ -211,15 +211,13 @@ def test_environment_override_translation_legacy_worker_harness_image(self): p | ptransform.Create([1, 2, 3]) | 'Do' >> ptransform.FlatMap(lambda x: [(x, x)]) | ptransform.GroupByKey()) - self.assertEqual( - list(remote_runner.proto_pipeline.components.environments.values()), - [ - beam_runner_api_pb2.Environment( - urn=common_urns.environments.DOCKER.urn, - payload=beam_runner_api_pb2.DockerPayload( - container_image='LEGACY').SerializeToString(), - capabilities=environments.python_sdk_docker_capabilities()) - ]) + first = remote_runner.proto_pipeline.components.environments.values() + second = beam_runner_api_pb2.Environment( + urn=common_urns.environments.DOCKER.urn, + payload=beam_runner_api_pb2.DockerPayload( + container_image='LEGACY').SerializeToString(), + capabilities=environments.python_sdk_docker_capabilities()) + self.assertTrue(first.__eq__(second)) Review Comment: do we still need to make this change? same question for line 232 below -- 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]
