[csharp] Initial C# codegen tests.#32734
Conversation
|
@jtattermusch for you to review. Once we're happy with how and where to write the tests I'll add tests for the new work. |
jtattermusch
left a comment
There was a problem hiding this comment.
overall looks good, but added a few comments to address. Also note that the BUILD file needs to be properly formatted (see failing sanity test) and the shell script needs to pass shellcheck (also see failing sanity test).
test/csharp/codegen/BUILD
Outdated
| data = [ | ||
| "//src/compiler:grpc_csharp_plugin", | ||
| "@com_google_protobuf//:protoc", | ||
| ] + glob(["simple/**"]), |
There was a problem hiding this comment.
I think glob() is considered an antipattern with bazel. It's better to simply list the actual files you'll use.
| option java_outer_classname = "HelloWorldProto"; | ||
| option objc_class_prefix = "HLW"; | ||
|
|
||
| package helloworld; |
There was a problem hiding this comment.
I think you'll need to use package test.csharp.codegen.simple.proto.helloworld to avoid internal conflict between .proto files.
| DIFFCMD="diff --strip-trailing-cr" | ||
|
|
||
| # diff expected files | ||
| [ "`$DIFFCMD ${DATA_DIR}/expected/Helloworld.cs \ |
There was a problem hiding this comment.
I think for now, we can get rid of Helloworld.cs and only test the code generated by grpc_csharp_plugin - which is what we should primarily be testing in this repo (we can add output of protoc as a followup if we want to).
| "//src/compiler:grpc_csharp_plugin", | ||
| "@com_google_protobuf//:protoc", | ||
| ] + glob(["simple/**"]), | ||
| uses_polling = False, |
There was a problem hiding this comment.
Adding
tags = [
"no_windows",
],
will automatically skip the test on windows (and I think we want that for now).
There was a problem hiding this comment.
while at it, you should probably also add noasan, notsan, noubsan, nomsan since the sanitizers are failing on the CI and we don't need any of them (at least initially).
| echo >&2 "Generated code does not match for Helloworld.cs" | ||
| exit 1 | ||
| } | ||
| [ "`$DIFFCMD ${DATA_DIR}/expected/HelloworldGrpc.cs \ |
There was a problem hiding this comment.
style: diff returns exit code depending on whether the files match or not. checking whether diff output is empty feels dirty.
|
@jtattermusch I've updated from the review comments. |
|
just as an update, I didn't forget about this but I'm working on resolving some issue that this change causes when imported to the internal codebase. |
| ], | ||
| tags = [ | ||
| "no_windows", | ||
| "noasan", |
There was a problem hiding this comment.
FTR the asan, tsan, msan and ubsan issues can also be resolved by adding //test/core/util:grpc_suppressions as a dependency I think.
There was a problem hiding this comment.
But for now we can leave as is.
jtattermusch
left a comment
There was a problem hiding this comment.
LGTM.
The ASAN failure is unrelated.
I think this will become mergeable once the internal change cl/524263844 is submitted (I'll then recheck and merge).
22d723c to
669a4a8
Compare
|
(I rebased and force-pushed to get a fresh test run). |
Initial bazel tests for C# protoc and grpc_protoc_plugin. This initial test just generated code from the proto file and compares the generated code against expected files. I've put the tests in `test/csharp/codegen` as that is similar to where the C++ tests are placed, but they could be moved to `src\csharp` if that is a better place. Further tests can be added once the initial framework for the tests is agreed.
Initial bazel tests for C# protoc and grpc_protoc_plugin. This initial test just generated code from the proto file and compares the generated code against expected files. I've put the tests in `test/csharp/codegen` as that is similar to where the C++ tests are placed, but they could be moved to `src\csharp` if that is a better place. Further tests can be added once the initial framework for the tests is agreed.
Initial bazel tests for C# protoc and grpc_protoc_plugin.
This initial test just generated code from the proto file and compares the generated code against expected files.
I've put the tests in
test/csharp/codegenas that is similar to where the C++ tests are placed, but they could be moved tosrc\csharpif that is a better place.Further tests can be added once the initial framework for the tests is agreed.