feat: add binding testing and multiple bindings test data to compliance service #1150

viacheslav-rostovtsev · 2022-07-21T22:15:14Z

Split into semantically meaningful commits for reading convenience.

…pliance.proto

…ated handlers

… Context

…rification

…multiple bindings

vchudnov-g

Did a first pass, but let me do a second pass next week. I think it's looking good, but I want to think some things through.

vchudnov-g · 2022-07-21T23:55:01Z

cmd/gapic-showcase/endpoint_test.go

@@ -174,7 +175,7 @@ func TestRESTCalls(t *testing.T) {
 			log.Fatal(err)
 		}
 		if got, want := string(body), testCase.want; noSpace(got) != noSpace(want) {
-			t.Errorf("testcase %2d: body: got `%s`, want %s", idx, got, want)
+			t.Errorf("testcase %2d: body: got `%s`, want `%s`", idx, noSpace(got), noSpace(want))


use %q instead of backticking the %s.

Also, for the error message, isn';t it clearer to leave the whitespace in for readability?

👍
I think being able to see the real assert condition is more convenient. In my case e.g. there was an issue with a tab, invisible in the 'readable' version.

Ah, so the problem might be that noSpace does not remove \t. Maybe it should, and then you can have the asserts use noSpace but the t.Errorf remain readable.

vchudnov-g · 2022-07-22T00:02:16Z

schema/google/showcase/v1beta1/compliance.proto

@@ -116,10 +119,13 @@ message RepeatRequest {
  optional int32 p_int32 = 7;
  optional int64 p_int64 = 8;
  optional double p_double = 9;
+
+  optional string intended_binding_uri = 10;


I would place this near the server_verify field, since it's not testing the data fromat preservation like the rest of the compliance suite is

vchudnov-g · 2022-07-22T00:12:57Z

util/genrest/resttools/constants.go

@@ -56,3 +56,6 @@ func init() {

 	RegexLiteral = fmt.Sprintf(`^[%s]+`, CharsLiteral)
 }
+
+// A key-type for storing binding URI value in the Context
+type BindingURIKey string


all the uses of this are BindingUriKey("bindingUri"), AFAICT. Why not define const BindingUriKey string="bindingUri"?

the constant has it's own string-derived type instead of string because (as I understand) the Context is a shared space so if someone else wants to have a "bindingUri" it'll be in a separate type.

Oh, I see what you mean about the type. The doc stipulates using non-built-in types to avoid collisions with other packages, since the type, not just the name, presumably plays a part in the lookup.

vchudnov-g · 2022-07-25T23:03:17Z

cmd/gapic-showcase/compliance_suite_test.go

@@ -217,6 +217,16 @@ func prepRepeatDataPathResourceTest(request *genproto.RepeatRequest) (verb strin
 	name = "Compliance.RepeatDataPathResource"
 	info := request.GetInfo()

+	if strings.HasPrefix(info.GetFString(), "first/") {
+		return prepRepeatDataPathResourceTestFirstBinding(request)


(no action needed) I think this is fine, but it makes me think in the future we should change the first/ and second/ in the URL templates to other names for clarity, like blue and cyan or something.....

vchudnov-g · 2022-07-25T23:03:49Z

server/services/compliance_service.go

+	// Separately checking that the binding in the client request matches binding in the test suite.
+	// This guards against the test suite file being wrong on the client.
+	if expectedRequest.GetIntendedBindingUri() != received.GetIntendedBindingUri() {
+		return fmt.Errorf("(ComplianceSuiteRequestBindingMismatchError) intended binding of request %q do not match test suite (expected: %s, received: %s)", name, expectedRequest.GetIntendedBindingUri(), received.GetIntendedBindingUri())


nit: s/do not/does not/

vchudnov-g · 2022-07-25T23:22:25Z

util/genrest/resttools/constants.go

@@ -56,3 +56,6 @@ func init() {

 	RegexLiteral = fmt.Sprintf(`^[%s]+`, CharsLiteral)
 }
+
+// A key-type for storing binding URI value in the Context
+type BindingURIKey string


Oh, I see what you mean about the type. The doc stipulates using non-built-in types to avoid collisions with other packages, since the type, not just the name, presumably plays a part in the lookup.

vchudnov-g · 2022-07-25T23:22:46Z

cmd/gapic-showcase/endpoint_test.go

@@ -174,7 +175,7 @@ func TestRESTCalls(t *testing.T) {
 			log.Fatal(err)
 		}
 		if got, want := string(body), testCase.want; noSpace(got) != noSpace(want) {
-			t.Errorf("testcase %2d: body: got `%s`, want %s", idx, got, want)
+			t.Errorf("testcase %2d: body: got `%s`, want `%s`", idx, noSpace(got), noSpace(want))


Ah, so the problem might be that noSpace does not remove \t. Maybe it should, and then you can have the asserts use noSpace but the t.Errorf remain readable.

viacheslav-rostovtsev added 10 commits July 21, 2022 12:00

add binding uri fields for server-side and client-side testing to com…

1d8ef12

…pliance.proto

downstream regeneration from adding binding_uri fields

ad8c916

updating genrest so that the binding uri is added to Context in gener…

2a2458b

…ated handlers

downstream regeneration of request handlers with binding uri added to…

7af580d

… Context

add testing intended_binding_uri to the compliance service request ve…

197f536

…rification

add additional_bindings to an RPC in a compliance.proto

397cabe

downstream changes from adding additional_bindings to an RPC

1a3949e

update compliance test suite with the testing payloads for binding ve…

1a543b8

…rification

test changes for the requestMatchesExpectation update

e1fd930

integration test updates related to additional fields in request and …

672cc07

…multiple bindings

viacheslav-rostovtsev requested a review from a team as a code owner July 21, 2022 22:15

golint

ceb9b52

viacheslav-rostovtsev requested review from noahdietz and vchudnov-g July 21, 2022 22:29

vchudnov-g reviewed Jul 22, 2022

View reviewed changes

review changes

d7554aa

viacheslav-rostovtsev requested a review from vchudnov-g July 22, 2022 18:18

proto changes re CR

3f792ca

vchudnov-g approved these changes Jul 25, 2022

View reviewed changes

viacheslav-rostovtsev and others added 2 commits July 26, 2022 17:22

wording fixup

9dec208

Merge branch 'main' into dev/virost/feat/compliance_multiple_bindings

2180714

viacheslav-rostovtsev merged commit 9d43ed0 into googleapis:main Jul 27, 2022

release-please bot mentioned this pull request Jul 27, 2022

chore(main): release 0.23.0 #1157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add binding testing and multiple bindings test data to compliance service #1150

feat: add binding testing and multiple bindings test data to compliance service #1150

viacheslav-rostovtsev commented Jul 21, 2022

vchudnov-g left a comment

vchudnov-g Jul 21, 2022

viacheslav-rostovtsev Jul 22, 2022

vchudnov-g Jul 25, 2022

vchudnov-g Jul 22, 2022

viacheslav-rostovtsev Jul 22, 2022

vchudnov-g Jul 22, 2022 •

edited

Loading

viacheslav-rostovtsev Jul 22, 2022

viacheslav-rostovtsev Jul 22, 2022

vchudnov-g Jul 25, 2022

vchudnov-g Jul 25, 2022

vchudnov-g Jul 25, 2022

vchudnov-g Jul 25, 2022

vchudnov-g Jul 25, 2022

feat: add binding testing and multiple bindings test data to compliance service #1150

feat: add binding testing and multiple bindings test data to compliance service #1150

Conversation

viacheslav-rostovtsev commented Jul 21, 2022

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g Jul 22, 2022 •

edited

Loading