robertwb commented on code in PR #27804: URL: https://github.com/apache/beam/pull/27804#discussion_r1293902971
########## sdks/python/apache_beam/yaml/yaml_provider_ut_test.py: ########## @@ -0,0 +1,643 @@ +# +# 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. +# + +"""Unit tests for yaml transform provider.""" +import collections +import copy +import json +import logging +import subprocess +import unittest + +import mock + +import apache_beam as beam +from apache_beam import beam_expansion_api_pb2, ExternalTransform, SchemaAwareExternalTransform, ParDo, \ Review Comment: Or, preferably, follow https://google.github.io/styleguide/pyguide.html#22-imports (though for test files I think it's OK to bend these a little for brevity/clarity). ########## sdks/python/apache_beam/yaml/yaml_provider_ut_test.py: ########## @@ -0,0 +1,643 @@ +# +# 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. +# + +"""Unit tests for yaml transform provider.""" +import collections +import copy +import json +import logging +import subprocess +import unittest + +import mock + +import apache_beam as beam +from apache_beam import beam_expansion_api_pb2, ExternalTransform, SchemaAwareExternalTransform, ParDo, \ Review Comment: Put all imports on their own line. (Similarly below.) ########## sdks/python/apache_beam/yaml/yaml_provider_ut_test.py: ########## @@ -0,0 +1,643 @@ +# +# 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. +# + +"""Unit tests for yaml transform provider.""" +import collections +import copy +import json +import logging +import subprocess +import unittest + +import mock + +import apache_beam as beam +from apache_beam import beam_expansion_api_pb2, ExternalTransform, SchemaAwareExternalTransform, ParDo, \ + JavaJarExpansionService +from apache_beam.transforms import external +from apache_beam.transforms.ptransform import _NamedPTransform +from apache_beam.version import __version__ as beam_version +from apache_beam.yaml.yaml_provider import ExternalProvider, InlineProvider, MetaInlineProvider, Provider, \ + ExternalPythonProvider, ExternalJavaProvider, PypiExpansionService, RemoteProvider, merge_providers, as_provider, \ + parse_external_providers, as_provider_list + +# pytype: skip-file + + +class ProviderTest(unittest.TestCase): + """ + Base class for test cases that involve creating a transform Provider for + YAML pipelines. + """ + def __init__(self, method_name='runProviderTest'): + super().__init__(method_name) + self._type = 'MockProvider' + self.providers = {self._type: Provider()} + + def test_available(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.available() + + def test_provided_transforms(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.provided_transforms() + + def test_create_transform(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.create_transform("", {}, lambda: beam.PTransform()) + + def test_affinity(self): + """Test the affinity score of various provider comparisons""" + for provider in self.providers.values(): + + class MockProvider(Provider): + """Mock provider class for testing affinity""" + def available(self): + pass + + def provided_transforms(self): + pass + + def create_transform(self, typ, args, yaml_create_transform): + pass + + self.assertEqual(200, provider.affinity(provider)) + self.assertEqual(20, provider.affinity(copy.copy(provider))) + self.assertEqual(0, provider.affinity(MockProvider())) + + +class InlineProviderTest(ProviderTest): + """Test class for InlineProvider tests.""" + def __init__(self, method_name='runInlineProviderTest'): + super().__init__(method_name) + self._type = 'MockInline' + self._transform_name = 'MockTransform' + self._called_inline = False + + def mock_transform(fn): + """Mock transform callable supplied to Provider's transform factory""" + self._called_inline = True + return self._transform_name >> beam.ParDo(fn) + + self.providers = {self._type: InlineProvider({self._type: mock_transform})} + + def test_available(self): Review Comment: So, one good rule of thumb for tests is that they should be simpler/more straightforward than the code that they're testing. While it could be useful to have some base class/framework for validating common properties across all providers, for these methods it does not make it easier to follow (e.g. why is there a loop? Where is this defined?) If Providers were complicated to set up, there's be more justification here as well. Instead, I'd do something like ``` def test_available(self): self.assertTrue(InlineProvider({})).available()) def test_provided_transforms(self): self.assertEqual(['SomeTransform', ], list(list(InlineProvider({'SomeTransform': lambda x: None}).provided_transforms()))) ... ``` ########## sdks/python/apache_beam/yaml/yaml_provider_ut_test.py: ########## @@ -0,0 +1,643 @@ +# +# 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. +# + +"""Unit tests for yaml transform provider.""" +import collections +import copy +import json +import logging +import subprocess +import unittest + +import mock + +import apache_beam as beam +from apache_beam import beam_expansion_api_pb2, ExternalTransform, SchemaAwareExternalTransform, ParDo, \ + JavaJarExpansionService +from apache_beam.transforms import external +from apache_beam.transforms.ptransform import _NamedPTransform +from apache_beam.version import __version__ as beam_version +from apache_beam.yaml.yaml_provider import ExternalProvider, InlineProvider, MetaInlineProvider, Provider, \ + ExternalPythonProvider, ExternalJavaProvider, PypiExpansionService, RemoteProvider, merge_providers, as_provider, \ + parse_external_providers, as_provider_list + +# pytype: skip-file + + +class ProviderTest(unittest.TestCase): + """ + Base class for test cases that involve creating a transform Provider for + YAML pipelines. + """ + def __init__(self, method_name='runProviderTest'): + super().__init__(method_name) + self._type = 'MockProvider' + self.providers = {self._type: Provider()} + + def test_available(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.available() + + def test_provided_transforms(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.provided_transforms() + + def test_create_transform(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.create_transform("", {}, lambda: beam.PTransform()) + + def test_affinity(self): + """Test the affinity score of various provider comparisons""" + for provider in self.providers.values(): + + class MockProvider(Provider): + """Mock provider class for testing affinity""" + def available(self): Review Comment: You don't need any of these methods for affinity testing. Also, rather than MockProvider (which this isn't really) I'd call this `AlternativeProvider` or similar as that captures the intent. ########## sdks/python/apache_beam/yaml/yaml_provider_ut_test.py: ########## @@ -0,0 +1,643 @@ +# +# 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. +# + +"""Unit tests for yaml transform provider.""" +import collections +import copy +import json +import logging +import subprocess +import unittest + +import mock + +import apache_beam as beam +from apache_beam import beam_expansion_api_pb2, ExternalTransform, SchemaAwareExternalTransform, ParDo, \ + JavaJarExpansionService +from apache_beam.transforms import external +from apache_beam.transforms.ptransform import _NamedPTransform +from apache_beam.version import __version__ as beam_version +from apache_beam.yaml.yaml_provider import ExternalProvider, InlineProvider, MetaInlineProvider, Provider, \ + ExternalPythonProvider, ExternalJavaProvider, PypiExpansionService, RemoteProvider, merge_providers, as_provider, \ + parse_external_providers, as_provider_list + +# pytype: skip-file + + +class ProviderTest(unittest.TestCase): + """ + Base class for test cases that involve creating a transform Provider for + YAML pipelines. + """ + def __init__(self, method_name='runProviderTest'): + super().__init__(method_name) + self._type = 'MockProvider' + self.providers = {self._type: Provider()} + + def test_available(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.available() + + def test_provided_transforms(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.provided_transforms() + + def test_create_transform(self): + """Assert that the base method is not implemented""" + with self.assertRaises(NotImplementedError): + for provider in self.providers.values(): + provider.create_transform("", {}, lambda: beam.PTransform()) + + def test_affinity(self): + """Test the affinity score of various provider comparisons""" + for provider in self.providers.values(): + + class MockProvider(Provider): + """Mock provider class for testing affinity""" + def available(self): + pass + + def provided_transforms(self): + pass + + def create_transform(self, typ, args, yaml_create_transform): + pass + + self.assertEqual(200, provider.affinity(provider)) + self.assertEqual(20, provider.affinity(copy.copy(provider))) + self.assertEqual(0, provider.affinity(MockProvider())) + + +class InlineProviderTest(ProviderTest): + """Test class for InlineProvider tests.""" + def __init__(self, method_name='runInlineProviderTest'): + super().__init__(method_name) + self._type = 'MockInline' + self._transform_name = 'MockTransform' + self._called_inline = False + + def mock_transform(fn): + """Mock transform callable supplied to Provider's transform factory""" + self._called_inline = True + return self._transform_name >> beam.ParDo(fn) + + self.providers = {self._type: InlineProvider({self._type: mock_transform})} + + def test_available(self): + """Test that the provider is available (Inline providers should always be available).""" + for provider in self.providers.values(): + self.assertTrue(provider.available()) + + def test_provided_transforms(self): + """Test that the provided transforms include the given transforms.""" + for provider_type, provider in self.providers.items(): + self.assertEqual([provider_type], list(provider.provided_transforms())) + + def test_create_transform(self): Review Comment: Similarly here, the risk is that you're testing implementation details not relevant to the class at hand (e.g. _NamedPTransform) and spreading out the test between here and `__init__`. Instead, do something like ``` def test_create_transform(self): class MyTransform(beam.PTransform): def __init__(self, arg): self.arg = arg ptransform = InlineProvider({'MyTransform': MyTransform()}).create_transform('MyTransform', {'arg': 'some_arg'}) self.assertIsinstance(ptransform, MyTransform) self.assertEqual(ptransform.arg, 'some_arg') ``` -- 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]
