Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP: implement custom_metadata interop, status_code_and_message, unimplemented_method tests #6631

Merged
merged 4 commits into from
Jun 17, 2016

Conversation

stanley-cheung
Copy link
Contributor

Implement custom_metadata interop test for PHP. Fixes #6579

@stanley-cheung stanley-cheung self-assigned this May 18, 2016
@stanley-cheung stanley-cheung force-pushed the php-custom-metadata-interop branch from 392bf9f to 6cd7122 Compare May 18, 2016 21:37
@stanley-cheung
Copy link
Contributor Author

This needs #6632 to be merged first

@stanley-cheung
Copy link
Contributor Author

Friendly ping

@stanley-cheung stanley-cheung force-pushed the php-custom-metadata-interop branch 2 times, most recently from e0946b2 to 28408b8 Compare June 15, 2016 03:02
@stanley-cheung
Copy link
Contributor Author

Another ping: this is just the custom_metadata interop test for PHP now

@murgatroid99
Copy link
Member

LGTM

@murgatroid99
Copy link
Member

test this please

@stanley-cheung
Copy link
Contributor Author

re-based and PHP tests passed

@jtattermusch
Copy link
Contributor

Let's un-blacklist the custom_metadata interop test for PHP, otherwise this change is pointless.

https://github.com/grpc/grpc/blob/master/tools/run_tests/run_interop_tests.py#L255

@stanley-cheung stanley-cheung force-pushed the php-custom-metadata-interop branch from 28408b8 to cb18b66 Compare June 17, 2016 18:20
@stanley-cheung stanley-cheung force-pushed the php-custom-metadata-interop branch from cb18b66 to 8939140 Compare June 17, 2016 18:52
@stanley-cheung
Copy link
Contributor Author

@murgatroid99 Please review this again, thanks!

So I end up implementing all of custom_metadata, status_code_and_message, unimplemented_method interop tests so that I can remove the _SKIP_ADVANCED blacklist.

Two things:

  1. Since PHP is still on proto2, I need to copy over some missing fields for messages.proto.
  2. For the unimplemented_method interop test, I cheated a little bit - if the method is truly undefined even in the proto, the method won't be in the generated stub, and if I try to call that, PHP will throw a Fatal Error. So I make an unimplemented method in the proto so that I can call it, which correspond to an unimplemented method on the server, and I get the desired response code.

@murgatroid99
Copy link
Member

The existing test.proto file already has an UnimplementedService definition. You should copy that to your proto file instead of making a different one.

@stanley-cheung
Copy link
Contributor Author

@murgatroid99 Thanks for the pointer. Fixed by copying and using UnimplementedService.

@murgatroid99
Copy link
Member

LGTM

@jtattermusch jtattermusch changed the title PHP: implement custom_metadata interop test PHP: implement custom_metadata interop, status_code_and_message, unimplemented_method tests Jun 17, 2016
@jtattermusch jtattermusch merged commit 0ab51a3 into grpc:master Jun 17, 2016
@stanley-cheung stanley-cheung deleted the php-custom-metadata-interop branch June 21, 2016 21:47
@lock lock bot locked as resolved and limited conversation to collaborators Jan 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants