gemini-code-assist[bot] commented on code in PR #38328:
URL: https://github.com/apache/beam/pull/38328#discussion_r3163416548
##########
sdks/python/apache_beam/io/external/xlang_bigqueryio_it_test.py:
##########
@@ -228,17 +228,20 @@ def run_storage_write_test(
hamcrest_assert(p, bq_matcher)
def test_all_types(self):
+ self.skip_if_not_dataflow_runner()
Review Comment:
The method `skip_if_not_dataflow_runner` is called here, but it is not
defined in this class or file, unlike in the other test files modified in this
PR. Additionally, `self._runner` is not initialized in this class. This will
lead to an `AttributeError` when the tests are executed. Please ensure the
method and its required state are correctly implemented or inherited.
##########
sdks/python/apache_beam/io/gcp/bigquery_geography_it_test.py:
##########
@@ -54,8 +55,10 @@ class BigQueryGeographyIntegrationTests(unittest.TestCase):
def setUp(self):
self.test_pipeline = TestPipeline(is_integration_test=True)
+ self.args = self.test_pipeline.get_full_options_as_args()
self.runner_name = type(self.test_pipeline.runner).__name__
self.project = self.test_pipeline.get_option('project')
+ self._runner = PipelineOptions(self.args).get_all_options()['runner']
Review Comment:
Instead of manually parsing `PipelineOptions`, use
`self.test_pipeline.get_option('runner')`. This is safer and more idiomatic.
Furthermore, `self.runner_name` is already initialized on line 59 and could be
used for the runner check, making `self._runner` and the additional `self.args`
assignment on line 58 redundant.
```suggestion
self._runner = self.test_pipeline.get_option('runner')
```
##########
sdks/python/apache_beam/io/gcp/bigquery_geography_it_test.py:
##########
@@ -34,6 +34,7 @@
from apache_beam.io.gcp.bigquery_tools import BigQueryWrapper
from apache_beam.io.gcp.internal.clients import bigquery
from apache_beam.io.gcp.tests.bigquery_matcher import BigqueryFullResultMatcher
+from apache_beam.options.pipeline_options import PipelineOptions
Review Comment:
This import is unnecessary if `self.test_pipeline.get_option('runner')` is
used instead of manually instantiating `PipelineOptions` in `setUp`.
##########
sdks/python/apache_beam/io/gcp/bigquery_geography_it_test.py:
##########
@@ -485,6 +489,14 @@ def test_geography_storage_write_api(self):
hc.assert_that(p, hc.all_of(*pipeline_verifiers))
+ def skip_if_not_dataflow_runner(self) -> bool:
Review Comment:
The `skip_if_not_dataflow_runner` method is duplicated in
`sdks/python/apache_beam/io/gcp/bigtableio_it_test.py`. Consider refactoring
this logic into a shared utility module or a common base class for integration
tests to improve maintainability.
##########
sdks/python/apache_beam/io/gcp/bigtableio_it_test.py:
##########
@@ -74,6 +75,7 @@ def setUp(self):
self.test_pipeline = TestPipeline(is_integration_test=True)
self.args = self.test_pipeline.get_full_options_as_args()
self.project = self.test_pipeline.get_option('project')
+ self._runner = PipelineOptions(self.args).get_all_options()['runner']
Review Comment:
Directly accessing `'runner'` from the options dictionary can be unsafe. It
is recommended to use `self.test_pipeline.get_option('runner')`, which handles
missing options gracefully and is more consistent with Beam's test patterns.
```suggestion
self._runner = self.test_pipeline.get_option('runner')
```
--
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]
