Allow generating test stubs separately from the client implementation #402

mpetrov · 2019-03-12T21:39:42Z

This allows generating test stubs in a separate file. Generating an implementation can be disabled with
--swiftgrpc_opt=Imlpementations=false.

A new ExtraModuleImports option allows adding import statement for the implementation. This enables generating test stubs in a separate module from implementation files. The name of the implementation module might very depending on the build system, so it can't be reliably inferred.

One use case for this would be generating different bazel build targets: a client target that's usable in production, and a stub target that's testonly = True (and only usable from test targets).

For example, a client could be generated in a echo_swift_grpc_client module follows:

protoc Sources/Examples/Echo/echo.proto \
--proto_path=Sources/Examples/Echo \
--plugin=.build/debug/protoc-gen-swift \
--plugin=.build/debug/protoc-gen-swiftgrpc \
--swiftgrpc_out=/tmp/echo_swift_grpc_client \
--swiftgrpc_opt=Client=true \
--swiftgrpc_opt=Server=false \
--swiftgrpc_opt=Visibility=Public

With a separate module for test stubs:

protoc Sources/Examples/Echo/echo.proto \
--proto_path=Sources/Examples/Echo \
--plugin=.build/debug/protoc-gen-swift \
--plugin=.build/debug/protoc-gen-swiftgrpc\
--swiftgrpc_out=/tmp/echo_swift_grpc_stub \
--swiftgrpc_opt=TestStubs=true \
--swiftgrpc_opt=Imlpementations=false \
--swiftgrpc_opt=Client=true \
--swiftgrpc_opt=Server=false \
--swiftgrpc_opt=ExtraModuleImports=echo_swift_grpc_client

MrMage · 2019-03-13T09:14:54Z

Sorry, it's still unclear to me how the logic has changed, especially with regards to how it used to behave depending on the Client/Server/TestStubs flags and how it behaves now. Would you mind elaborating further? It also looks like your changes only affect the generated code for the client, not the server? Should this be done for both?

mpetrov · 2019-03-13T11:39:38Z

There are effectively 3 changes here:

Rather than interleaving generated clients and stubs, this separates them into two distinct sections of the generated source file. The primary goal of this is to allow turning either client generation or stub generation off with a single options check rather than sprinkling around if/else checks throughout the client generator source code. This is similar to how printNIOGRPCClient only gets called if options.generateNIOImplementation is true, rather than adding if options.generateNIOImplementation { ... } checks all throughout the generator.
Allow injecting import statements into the generated source via the ExtraModuleImports option. This is to allow splitting generated source into various modules that can import each other. It would also help handle any weirdnesses specific to a particular build system that might come up.
Generate client stubs when TestStubs=true is on, even if client code isn't included Client=false. I don't think I was fully factoring server-test stubs here, so this is probably the most half-baked part of this PR. I could alternatively (a) Introduce a Implementation=false flag to turn off actual implementation code OR (b) Introduce TestClientStubs and TestServerStubs options to allow finer-grained control. Explicitly setting TestStubs could modify both for backwards compatibility.

Feedback is welcome. I'm happy to farm off smaller PRs if you feel any parts of this are less controversial, or would make it easier to review.

MrMage · 2019-03-14T12:06:58Z

@mpetrov thank you for the elaboration! 1. and 2. sound reasonable to me. Same for 3., but I agree that we should have more granular options for this.

Sounds like we have four different binary flags here, but not all combinations would make sense, for example this one:

	Client	Server
Impls	✓	✗
TestStubs	✗	✓

So far, I tend towards having the following options:

Impls (naming to be discussed): Defaults to true. If set, the regular interfaces implementations will be generated.
TestStubs: If set, the test stubs implementations will be generated. Can not stand on its own without the interfaces generated by Impls; that's where ExtraModuleImports comes in.
Client/Server: Whether implementations/stubs should be generated for client, server, both, or none (with the last option not being very useful, same for Impls = false combined with TestStubs = false).

So these would be the conditions to generate the corresponding code:

	Client	Server
Impls	`Client` && `Impls`	`Server` && `Impls`
TestStubs	`Client` && `TestStubs`	`Server` && `TestStubs`

Thoughts?

This will make it simpler to conditionally turn of generating the implementation, so that test stubs can be compiled separately. See PR grpc#402 for more discussion.

mpetrov · 2019-03-14T12:45:39Z

Thanks for the feedback @MrMage

I split out the client generator updates (1.) in #403 since that's the bulk of the deltas in this PR.

An Impls or Implementations flag sounds good to me. I don't have strong preferences on naming, so happy to take suggestions.

This will make it simpler to conditionally turn of generating the implementation, so that test stubs can be compiled separately. See PR #402 for more discussion.

MrMage · 2019-03-14T13:30:39Z

@rebello95 any preference on naming? Otherwise, I would suggest Implementations. (@mpetrov, don't forget to document the flag in README.md.)

Fix unary test stub

mpetrov · 2019-03-14T15:52:49Z

Rebased to include #403 and add new option.

README.md

Sources/protoc-gen-swiftgrpc/Generator.swift

Sources/protoc-gen-swiftgrpc/options.swift

README.md

MrMage

Thank you for these changes! I'll merge once @rebello95 has taken a look as well.

rebello95

Adding these options is a useful change overall, some comments

rebello95 · 2019-03-14T17:59:32Z

README.md

@@ -102,8 +102,10 @@ separated from the output directory by a colon.
 | `Client` |  `true`/`false` | `true` | Whether to generate client code |
 | `Async` |  `true`/`false` | `true` | Whether to generate asynchronous code |
 | `Sync` |  `true`/`false` | `true` | Whether to generate synchronous code |
+| `Implementations` |  `true`/`false` | `true` | Whether to generate protocols and non-test service code. Toggling this to `false` is mostly useful when combined with `TestStubs=true` to generate files containing only test stub code.  |


Nit: Other options don't have a trailing period in their description

Removed trailing periods.

rebello95 · 2019-03-14T18:03:27Z

README.md

 | `TestStubs` |  `true`/`false` | `false` | Whether to generate test stub code |
 | `FileNaming` | `FullPath`/`PathToUnderscores`/`DropPath` | `FullPath` | How to handle the naming of generated sources |
+| `ExtraModuleImports` |  `String` | `` | Extra module to import in generated code |


The description is Extra module to import in generated code, but I assume multiple imports can be passed? If so, can we provide details on how that works?

Updated the text a bit. Imports could look as follows:
--swiftgrpc_opt=ExtraModuleImports=foo,ExtraModuleImports=qux --swiftgrpc_opt=ExtraModuleImports=baz

Sources/protoc-gen-swiftgrpc/Generator-Client.swift

rebello95 · 2019-03-14T18:03:50Z

Sources/protoc-gen-swiftgrpc/Generator.swift

@@ -88,7 +88,7 @@ class Generator {
        "SwiftGRPC",
        "SwiftProtobuf"]
    }
-    for moduleName in moduleNames {
+    for moduleName in moduleNames +  options.extraModuleImports {


Might be nice to sort these

rebello95 · 2019-03-14T18:04:27Z

Sources/protoc-gen-swiftgrpc/Generator.swift

@@ -118,6 +131,9 @@ class Generator {
      if options.generateTestStubs && options.generateNIOImplementation {
        fatalError("Generating test stubs is not yet supported for SwiftGRPC-NIO.")
      }
+      if options.generateTestStubs && !options.generateImplementations {
+        fatalError("Generating server test stubs without an implementation is not yet supported.")


Do we plan to support this? If not, maybe remove "yet"

Removed for now.

rebello95 · 2019-03-14T18:05:05Z

Sources/protoc-gen-swiftgrpc/options.swift

@@ -131,6 +140,13 @@ final class GeneratorOptions {
          throw GenerationError.invalidParameterValue(name: pair.key, value: pair.value)
        }

+      case "ExtraModuleImports":
+        if !pair.value.isEmpty {
+          extraModuleImports.append(pair.value)


Do we need to split the value here to handle multiple import statements? I.e., pair.value.components(separatedBy: ",")?

No, per other comment multiple imports would be done by repeating the parameter.

--swiftgrpc_opt=ExtraModuleImports=foo,ExtraModuleImports=qux --swiftgrpc_opt=ExtraModuleImports=baz

The options are already comma separated in parseParameter(string:), and changing the semantics there could break existing callers.

mpetrov · 2019-03-14T22:17:24Z

Took another pass, thanks for the feedback!

mpetrov mentioned this pull request Mar 13, 2019

Add public init to client call unary stubs. #401

Merged

mpetrov mentioned this pull request Mar 14, 2019

Generate client test stubs in a dedicated section. #403

Merged

Split out test-stub generation from impl code

4fe128d

Fix unary test stub

mpetrov force-pushed the fakes branch from 1945ec4 to 4fe128d Compare March 14, 2019 15:48

MrMage requested changes Mar 14, 2019

View reviewed changes

Applying suggested fixes

3bb0fe0

MrMage approved these changes Mar 14, 2019

View reviewed changes

rebello95 reviewed Mar 14, 2019

View reviewed changes

Incorporate feedback

bf71726

rebello95 approved these changes Mar 14, 2019

View reviewed changes

MrMage closed this Mar 15, 2019

MrMage reopened this Mar 15, 2019

MrMage merged commit 869c168 into grpc:master Mar 15, 2019

allevato mentioned this pull request May 14, 2020

do not add own module import in generated code #796

Merged

glbrntt mentioned this pull request May 10, 2023

Add ExtraModuleImports option apple/swift-protobuf#1400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow generating test stubs separately from the client implementation #402

Allow generating test stubs separately from the client implementation #402

mpetrov commented Mar 12, 2019 •

edited

Loading

MrMage commented Mar 13, 2019

mpetrov commented Mar 13, 2019

MrMage commented Mar 14, 2019 •

edited

Loading

mpetrov commented Mar 14, 2019

MrMage commented Mar 14, 2019

mpetrov commented Mar 14, 2019

MrMage left a comment

rebello95 left a comment

rebello95 Mar 14, 2019

mpetrov Mar 14, 2019

rebello95 Mar 14, 2019

mpetrov Mar 14, 2019

rebello95 Mar 14, 2019

rebello95 Mar 14, 2019

mpetrov Mar 14, 2019

rebello95 Mar 14, 2019

mpetrov Mar 14, 2019

mpetrov commented Mar 14, 2019

Allow generating test stubs separately from the client implementation #402

Allow generating test stubs separately from the client implementation #402

Conversation

mpetrov commented Mar 12, 2019 • edited Loading

MrMage commented Mar 13, 2019

mpetrov commented Mar 13, 2019

MrMage commented Mar 14, 2019 • edited Loading

mpetrov commented Mar 14, 2019

MrMage commented Mar 14, 2019

mpetrov commented Mar 14, 2019

MrMage left a comment

Choose a reason for hiding this comment

rebello95 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

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

mpetrov commented Mar 14, 2019

mpetrov commented Mar 12, 2019 •

edited

Loading

MrMage commented Mar 14, 2019 •

edited

Loading