From 0c71b921024ee39cff3dc9077a04c5286349a6ab Mon Sep 17 00:00:00 2001 From: Yu-Han Liu Date: Fri, 2 Jun 2023 14:55:33 -0700 Subject: [PATCH 1/6] fix: mock return_value should not populate oneof message fields --- .../gapic/%name_%version/%sub/test_macros.j2 | 10 +++-- .../test_oneof_imported_response.proto | 39 +++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/fragments/test_oneof_imported_response.proto diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 index 952751323b..967f9ef03d 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 @@ -34,11 +34,15 @@ def test_{{ method_name }}(request_type, transport: str = 'grpc'): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} + {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} - {% with field = oneof_fields[0] %} + {# Take the first non-message oneof field. In the corner case when all the fields of a oneof are messages, the oneof will not be populated #} + {# Use an outer if-statement here because `first` raises an error if called on an empty sequence #} + {% if (oneof_fields | rejectattr('message') | list) %} + {% for field in oneof_fields | rejectattr('message') | first %} {{ field.name }}={{ field.mock_value }}, - {% endwith %} + {% endfor %} + {% endif %} {% endfor %} ) {% endif %} diff --git a/tests/fragments/test_oneof_imported_response.proto b/tests/fragments/test_oneof_imported_response.proto new file mode 100644 index 0000000000..be12cbd867 --- /dev/null +++ b/tests/fragments/test_oneof_imported_response.proto @@ -0,0 +1,39 @@ +// Copyright (C) 2021 Google LLC +// +// Licensed 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. + +syntax = "proto3"; + +package google.fragment; + +import "google/api/client.proto"; +import "import.proto"; + +service MyService { + option (google.api.default_host) = "my.example.com"; + rpc MyMethod(MethodRequest) returns (MethodResponse) {} +} + +message MethodRequest { + string input = 1; +} + +message Container { + Import import = 1; +} + +message MethodResponse { + oneof format { + Container container = 1; + } +} From d87e47fa9e699cf02dfb1b9f987a433e2e45d584 Mon Sep 17 00:00:00 2001 From: Yu-Han Liu Date: Fri, 2 Jun 2023 15:06:40 -0700 Subject: [PATCH 2/6] the first filter returns an element --- .../tests/unit/gapic/%name_%version/%sub/test_macros.j2 | 4 ++-- tests/fragments/test_oneof_imported_response.proto | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 index 967f9ef03d..d8cc8a3b6a 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 @@ -39,9 +39,9 @@ def test_{{ method_name }}(request_type, transport: str = 'grpc'): {# Take the first non-message oneof field. In the corner case when all the fields of a oneof are messages, the oneof will not be populated #} {# Use an outer if-statement here because `first` raises an error if called on an empty sequence #} {% if (oneof_fields | rejectattr('message') | list) %} - {% for field in oneof_fields | rejectattr('message') | first %} + {% with field = (oneof_fields | rejectattr('message') | first) %} {{ field.name }}={{ field.mock_value }}, - {% endfor %} + {% endwith %} {% endif %} {% endfor %} ) diff --git a/tests/fragments/test_oneof_imported_response.proto b/tests/fragments/test_oneof_imported_response.proto index be12cbd867..2537fa2b81 100644 --- a/tests/fragments/test_oneof_imported_response.proto +++ b/tests/fragments/test_oneof_imported_response.proto @@ -35,5 +35,6 @@ message Container { message MethodResponse { oneof format { Container container = 1; + string name = 2; } } From 536865664a5eb83c699c9b48a63c80132b8a5857 Mon Sep 17 00:00:00 2001 From: Yu-Han Liu Date: Fri, 2 Jun 2023 15:27:24 -0700 Subject: [PATCH 3/6] duplicate the change in ads-template --- .../unit/gapic/%name_%version/%sub/test_%service.py.j2 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index c174d37e88..823a3debba 100644 --- a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -545,11 +545,15 @@ def test_{{ method_name }}(request_type, transport: str = 'grpc'): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} + {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} - {% with field = oneof_fields[0] %} + {# Take the first non-message oneof field. In the corner case when all the fields of a oneof are messages, the oneof will not be populated #} + {# Use an outer if-statement here because `first` raises an error if called on an empty sequence #} + {% if (oneof_fields | rejectattr('message') | list) %} + {% with field = (oneof_fields | rejectattr('message') | first) %} {{ field.name }}={{ field.mock_value }}, {% endwith %} + {% endif %} {% endfor %} ) {% endif %} From 17b2c2aa68de32bea47b91c865d4bdd869ea1485 Mon Sep 17 00:00:00 2001 From: Yu-Han Liu Date: Fri, 2 Jun 2023 18:40:29 -0700 Subject: [PATCH 4/6] update golden files --- .../tests/unit/gapic/logging_v2/test_config_service_v2.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py b/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py index a11636125c..86991e1277 100755 --- a/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py +++ b/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py @@ -3248,7 +3248,6 @@ def test_get_sink(request_type, transport: str = 'grpc'): output_version_format=logging_config.LogSink.VersionFormat.V2, writer_identity='writer_identity_value', include_children=True, - bigquery_options=logging_config.BigQueryOptions(use_partitioned_tables=True), ) response = client.get_sink(request) @@ -3509,7 +3508,6 @@ def test_create_sink(request_type, transport: str = 'grpc'): output_version_format=logging_config.LogSink.VersionFormat.V2, writer_identity='writer_identity_value', include_children=True, - bigquery_options=logging_config.BigQueryOptions(use_partitioned_tables=True), ) response = client.create_sink(request) @@ -3780,7 +3778,6 @@ def test_update_sink(request_type, transport: str = 'grpc'): output_version_format=logging_config.LogSink.VersionFormat.V2, writer_identity='writer_identity_value', include_children=True, - bigquery_options=logging_config.BigQueryOptions(use_partitioned_tables=True), ) response = client.update_sink(request) From 4304768ece80f654a8670900fce7a53d51ebf88d Mon Sep 17 00:00:00 2001 From: Yu-Han Liu Date: Fri, 2 Jun 2023 21:22:03 -0700 Subject: [PATCH 5/6] duplicate template code --- .../%name_%version/%sub/test_%service.py.j2 | 8 ++++-- .../gapic/%name_%version/%sub/test_macros.j2 | 26 +++++++++++-------- .../test_oneof_imported_response.proto | 5 ++-- .../logging_v2/test_config_service_v2.py | 3 --- 4 files changed, 24 insertions(+), 18 deletions(-) diff --git a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index 823a3debba..cd45ee386a 100644 --- a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -965,9 +965,11 @@ def test_{{ method_name }}_rest(request_type, transport: str = 'rest'): {% endif %}{% endfor %} {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} - {% with field = oneof_fields[0] %} + {% if (oneof_fields | rejectattr('message') | list) %} + {% with field = (oneof_fields | rejectattr('message') | first) %} {{ field.name }}={{ field.mock_value }}, {% endwith %} + {% endif %} {% endfor %} ) {% endif %} @@ -1035,9 +1037,11 @@ def test_{{ method.name|snake_case }}_rest(request_type): {% endif %}{% endfor %} {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} - {% with field = oneof_fields[0] %} + {% if (oneof_fields | rejectattr('message') | list) %} + {% with field = (oneof_fields | rejectattr('message') | first) %} {{ field.name }}={{ field.mock_value }}, {% endwith %} + {% endif %} {% endfor %} ) {% endif %} diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 index d8cc8a3b6a..0bc8eea936 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 @@ -907,11 +907,13 @@ def test_{{ method_name }}_rest(request_type): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} - {% for oneof_fields in method.extended_lro.operation_type.oneof_fields().values() %} - {% with field = oneof_fields[0] %} - {{ field.name }}={{ field.mock_value }}, - {% endwith %} + {# This is a hack to only pick one field #} + {% for oneof_fields in method.output.oneof_fields().values() %} + {% if (oneof_fields | rejectattr('message') | list) %} + {% with field = (oneof_fields | rejectattr('message') | first) %} + {{ field.name }}={{ field.mock_value }}, + {% endwith %} + {% endif %} {% endfor %} ) {% else %} @@ -920,12 +922,14 @@ def test_{{ method_name }}_rest(request_type): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} - {% for oneof_fields in method.output.oneof_fields().values() %} - {% with field = oneof_fields[0] %} - {{ field.name }}={{ field.mock_value }}, - {% endwith %} - {% endfor %} + {# This is a hack to only pick one field #} + {% for oneof_fields in method.output.oneof_fields().values() %} + {% if (oneof_fields | rejectattr('message') | list) %} + {% with field = (oneof_fields | rejectattr('message') | first) %} + {{ field.name }}={{ field.mock_value }}, + {% endwith %} + {% endif %} + {% endfor %} ) {% endif %} diff --git a/tests/fragments/test_oneof_imported_response.proto b/tests/fragments/test_oneof_imported_response.proto index 2537fa2b81..53f2a2cbd2 100644 --- a/tests/fragments/test_oneof_imported_response.proto +++ b/tests/fragments/test_oneof_imported_response.proto @@ -33,8 +33,9 @@ message Container { } message MethodResponse { + string parent = 1; oneof format { - Container container = 1; - string name = 2; + Container container = 2; + string name = 3; } } diff --git a/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py b/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py index 86991e1277..079e9cd542 100755 --- a/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py +++ b/tests/integration/goldens/logging/tests/unit/gapic/logging_v2/test_config_service_v2.py @@ -8543,7 +8543,6 @@ def test_get_sink_rest(request_type): output_version_format=logging_config.LogSink.VersionFormat.V2, writer_identity='writer_identity_value', include_children=True, - bigquery_options=logging_config.BigQueryOptions(use_partitioned_tables=True), ) # Wrap the value into a proper Response obj @@ -8791,7 +8790,6 @@ def test_create_sink_rest(request_type): output_version_format=logging_config.LogSink.VersionFormat.V2, writer_identity='writer_identity_value', include_children=True, - bigquery_options=logging_config.BigQueryOptions(use_partitioned_tables=True), ) # Wrap the value into a proper Response obj @@ -9045,7 +9043,6 @@ def test_update_sink_rest(request_type): output_version_format=logging_config.LogSink.VersionFormat.V2, writer_identity='writer_identity_value', include_children=True, - bigquery_options=logging_config.BigQueryOptions(use_partitioned_tables=True), ) # Wrap the value into a proper Response obj From c669ef06246940430c6e08008db1c38bbf83ff31 Mon Sep 17 00:00:00 2001 From: Yu-Han Liu Date: Fri, 2 Jun 2023 21:24:02 -0700 Subject: [PATCH 6/6] format template comments --- .../tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 | 4 ++-- .../tests/unit/gapic/%name_%version/%sub/test_macros.j2 | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index cd45ee386a..3ad8ef7689 100644 --- a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -963,7 +963,7 @@ def test_{{ method_name }}_rest(request_type, transport: str = 'rest'): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} + {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} {% if (oneof_fields | rejectattr('message') | list) %} {% with field = (oneof_fields | rejectattr('message') | first) %} @@ -1035,7 +1035,7 @@ def test_{{ method.name|snake_case }}_rest(request_type): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} + {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} {% if (oneof_fields | rejectattr('message') | list) %} {% with field = (oneof_fields | rejectattr('message') | first) %} diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 index 0bc8eea936..2d895cab03 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 @@ -907,7 +907,7 @@ def test_{{ method_name }}_rest(request_type): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} + {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} {% if (oneof_fields | rejectattr('message') | list) %} {% with field = (oneof_fields | rejectattr('message') | first) %} @@ -922,7 +922,7 @@ def test_{{ method_name }}_rest(request_type): {% if not field.oneof or field.proto3_optional %} {{ field.name }}={{ field.mock_value }}, {% endif %}{% endfor %} - {# This is a hack to only pick one field #} + {# This is a hack to only pick one field #} {% for oneof_fields in method.output.oneof_fields().values() %} {% if (oneof_fields | rejectattr('message') | list) %} {% with field = (oneof_fields | rejectattr('message') | first) %}