tools: Add protoxform tests #9241

Shikugawa · 2019-12-05T16:08:46Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add testing for protoxform scripts. We apply golden testing method for this.
Risk Level: Low
Testing: none
Docs Changes: none
Release Notes:
[Optional Fixes #Issue] #8428
[Optional Deprecated:]

Signed-off-by: shikugawa <[email protected]>

tools/api_proto_plugin/utils.py

tools/protoxform/options.py

tools/protoxform_test_helper.py

htuch

This is super cool @Shikugawa, big thanks for putting this together. I think the PR is largely correctly structured, just working through some Python style stuff.
/wait

tools/run_command.py

tools/type_whisperer/typedb_gen.py

tools/protoxform_test_helper.py

tools/protoxform_test.sh

Signed-off-by: shikugawa <[email protected]>

htuch

Cool, thanks for following up on the comments. A few more.
/wait

tools/spelling_dictionary.txt

tools/type_whisperer/type_database.bzl

tools/type_whisperer/typedb_gen.py

tools/testdata/protoxform/envoy/v2/sample.proto.v2.gold

htuch · 2019-12-10T23:17:52Z

tools/run_command.py

+# stdout and stderr as a string array.
+def runCommand(command):
+  logging.info("%s" % command)
+  proc = subprocess.run([command], shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


You can just do capture_output=True.

@htuch we can't use capture_output because envoy_ci internal python runtime version is less than 3.7

I was able to use it in https://github.com/envoyproxy/envoy/pull/9258/files#diff-bba1df52a76edae166f0c52c4fed21dbR38, I'm wondering what version issues we're hitting here.

@htuch it seems that api_boost.py is not defined to run on envoy_ci internal docker runtime because the execution definition isn't written in do_ci.sh so test should be passed. But if we add this python script execution definition to that shell script, it may be failed... so unless update docker internal python version, this argument should not work....

anyway, add issue #9339

@Shikugawa is this blocking? I think I will have a similar issue with a test I'm working on, so I can take a look. Is there anything else here that we need to sort out?

@htuch hmm...can't pass formatting check because of confusing exception...
https://dev.azure.com/cncf/envoy/_build/results?buildId=23684&view=logs&jobId=9fd74226-df5b-5f29-39da-76fc341cc763&j=9fd74226-df5b-5f29-39da-76fc341cc763&t=068b98b1-acf0-52d0-fc6e-f6d1a5b1d5ef

so is capture_output=True is the reason for python >= 3.7? I think using subprocess.PIPE can workaround it.

@Shikugawa the exception seems from yapf so it's failing on Python formatter somewhere, were you able to run ci/run_envoy_docker.sh 'ci/check_and_fix_format.sh' locally?

tools/run_command.py

tools/protoxform_test_helper.py

htuch · 2019-12-10T23:20:26Z

tools/protoxform_test.sh

+  //tools/testdata/protoxform:protos --aspects //tools/protoxform:protoxform.bzl%protoxform_aspect --output_groups=proto \
+  --action_env=CPROFILE_ENABLED=1 --host_force_python=PY3
+
+./tools/protoxform_test_helper.py ${PROTO_TARGETS}


I wonder if @lizan has any Bazel Fu to make the tests work as Bazel aspects.

Yes, you can write a rule that depends on aspect (like what type_database rule does depend on type_whisperer aspect), then define a test target on that. but we're good to go with this for now.

Signed-off-by: shikugawa <[email protected]>

htuch

LGTM modulo the one question I have for @lizan on aspect magic. Thanks!

tools/protoxform_test_helper.py

Signed-off-by: shikugawa <[email protected]>

htuch · 2019-12-13T19:23:24Z

@Shikugawa LGTM, can you fix_format so that CI passes and we can merge? Thanks again for this work, this is really nice and will be a huge velocity improvement for work in protoxform (we can now make changes with confidence).

htuch · 2019-12-17T01:59:37Z

tools/protoxform_test_helper.py

+  messages += Run(path, filename, 'v3alpha')
+
+  if len(messages) == 0:
+    logging.warning("PASS")


I think you are going to want to sys.exit() with different success/failure values here (was just looking again at this PR as I'm writing some simple golden source test as well right now). Thanks.

Signed-off-by: shikugawa <[email protected]>

htuch · 2019-12-18T04:00:40Z

tools/protoxform_test_helper.py

+  """
+  base = "./bazel-bin/"
+  delimited_path = path.split('/')
+  base += os.path.join(*delimited_path, "protos", *delimited_path)


I think this might be where the pyformat failure is happening; how come you want to squash the path twice here and join it twice? Why not just use path instead of delimited_path?

@htuch automatically generated proto file is located on ./bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/.
So, this instruction could not avoid. But, It seems for me to use path instead of delimited_path.

Signed-off-by: shikugawa <[email protected]>

htuch

Thanks!

Signed-off-by: Harvey Tuch <[email protected]>

* master: (167 commits) stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy (envoyproxy#8779) Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. (envoyproxy#9362) tools: API boosting support for using decls and enum constants. (envoyproxy#9418) Fix incorrect cluster InitializePhase type (envoyproxy#9379) build: fix merge race between envoyproxy#9241 and envoyproxy#9413. (envoyproxy#9427) fuzz: fix incorrect evaluator test (envoyproxy#9402) server: fix bogus startup log message (envoyproxy#9404) tools: Add protoxform tests (envoyproxy#9241) api: options after import (envoyproxy#9413) misc: use std::move instead of constructing a copy (envoyproxy#9415) tools: API boosting support for rewriting elaborated types. (envoyproxy#9375) docs: fix invalid transport_socket value (envoyproxy#9403) fix typo in docs (envoyproxy#9394) srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. (envoyproxy#9366) api: generate whole directory and sync (envoyproxy#9382) bazel: Add load statements for proto_library (envoyproxy#9367) Fix typo (envoyproxy#9388) Correct test of OptionsImpl argc type (Was: Correct type for std::array size() result) (envoyproxy#9290) http1 encode trailers in chunk encoding (envoyproxy#8667) Add mode to PipeInstance (envoyproxy#8423) ...

Add testing for protoxform scripts. We apply golden testing method for this. Risk Level: Low Testing: none Docs Changes: none Release Notes: Fixes envoyproxy#8428 Signed-off-by: shikugawa <[email protected]> Signed-off-by: Prakhar <[email protected]>

…nvoyproxy#9427) Signed-off-by: Harvey Tuch <[email protected]> Signed-off-by: Prakhar <[email protected]>

Shikugawa added 3 commits December 5, 2019 15:55

add protoxform_test basis implmentation

7fa5ee1

Signed-off-by: shikugawa <[email protected]>

fix python format

46290c0

Signed-off-by: shikugawa <[email protected]>

return invalid changes

caca8d5

Signed-off-by: shikugawa <[email protected]>

Shikugawa changed the title ~~Add protoxform tests~~ tools: Add protoxform tests Dec 5, 2019

lizan requested a review from htuch December 5, 2019 21:01

Shikugawa added 2 commits December 6, 2019 09:05

fix helper codes

c4a7eef

Signed-off-by: shikugawa <[email protected]>

fix regex pattern

db92556

Signed-off-by: shikugawa <[email protected]>

Shikugawa marked this pull request as ready for review December 6, 2019 09:55

lizan assigned htuch Dec 6, 2019

htuch reviewed Dec 6, 2019

View reviewed changes

tools/api_proto_plugin/utils.py Outdated Show resolved Hide resolved

tools/protoxform/options.py Outdated Show resolved Hide resolved

tools/protoxform_test_helper.py Outdated Show resolved Hide resolved

htuch suggested changes Dec 6, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 6, 2019

Shikugawa added 2 commits December 10, 2019 10:53

reflect and refactor

660f1fe

Signed-off-by: shikugawa <[email protected]>

fix dictionary

dbd6f4b

Signed-off-by: shikugawa <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 10, 2019

htuch reviewed Dec 10, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 10, 2019

reflect reviews

6851f4e

Signed-off-by: shikugawa <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 12, 2019

revert

266886f

Signed-off-by: shikugawa <[email protected]>

htuch reviewed Dec 12, 2019

View reviewed changes

tools/protoxform_test_helper.py Outdated Show resolved Hide resolved

htuch added the waiting:any label Dec 12, 2019

repokitteh-read-only bot removed the waiting:any label Dec 13, 2019

Shikugawa added 4 commits December 13, 2019 06:16

capitalize for pydoc

bd44286

Signed-off-by: shikugawa <[email protected]>

Kick CI

7366a4a

Signed-off-by: shikugawa <[email protected]>

Kick CI

2667652

Signed-off-by: shikugawa <[email protected]>

Merge remote-tracking branch 'upstream/master' into add-protoxform-tests

e2c07d6

Signed-off-by: shikugawa <[email protected]>

htuch added the waiting label Dec 16, 2019

htuch reviewed Dec 17, 2019

View reviewed changes

add sys.exit()

9b42b16

Signed-off-by: shikugawa <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 17, 2019

Shikugawa added 5 commits December 17, 2019 04:08

Merge remote-tracking branch 'upstream/master' into add-protoxform-tests

ad60337

Signed-off-by: shikugawa <[email protected]>

spelling was not passed

4df9c39

Signed-off-by: shikugawa <[email protected]>

kick ci

959e815

Signed-off-by: shikugawa <[email protected]>

tiny fix and kick ci

d8591eb

Signed-off-by: shikugawa <[email protected]>

Kick CI

ff147d8

Signed-off-by: shikugawa <[email protected]>

htuch reviewed Dec 18, 2019

View reviewed changes

Shikugawa added 4 commits December 19, 2019 05:21

rewrite path construction

89ce863

Signed-off-by: shikugawa <[email protected]>

resolve conflict

f33d48d

Signed-off-by: shikugawa <[email protected]>

fix unused section

fef4641

Signed-off-by: shikugawa <[email protected]>

Kick CI

407ac8d

Signed-off-by: shikugawa <[email protected]>

htuch approved these changes Dec 19, 2019

View reviewed changes

htuch merged commit 6992b00 into envoyproxy:master Dec 19, 2019

htuch added a commit to htuch/envoy that referenced this pull request Dec 19, 2019

build: fix merge race between envoyproxy#9241 and envoyproxy#9413.

f871c19

Signed-off-by: Harvey Tuch <[email protected]>

htuch added a commit that referenced this pull request Dec 19, 2019

build: fix merge race between #9241 and #9413. (#9427)

b8558af

Signed-off-by: Harvey Tuch <[email protected]>

wrowe mentioned this pull request Dec 20, 2019

Fix win32 breakage 18 dec #9399

Merged

prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020

build: fix merge race between envoyproxy#9241 and envoyproxy#9413. (e…

f28e237

…nvoyproxy#9427) Signed-off-by: Harvey Tuch <[email protected]> Signed-off-by: Prakhar <[email protected]>

adisuissa mentioned this pull request Jul 16, 2021

tooling: Add protolock bazel dependency and tests #17375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: Add protoxform tests #9241

tools: Add protoxform tests #9241

Shikugawa commented Dec 5, 2019

htuch left a comment

htuch left a comment

htuch Dec 10, 2019

Shikugawa Dec 12, 2019

htuch Dec 12, 2019

Shikugawa Dec 13, 2019

Shikugawa Dec 13, 2019

htuch Dec 17, 2019

Shikugawa Dec 17, 2019

lizan Dec 19, 2019

htuch Dec 10, 2019

lizan Dec 11, 2019

htuch left a comment

htuch commented Dec 13, 2019 •

edited

Loading

htuch Dec 17, 2019

htuch Dec 18, 2019

Shikugawa Dec 19, 2019

htuch left a comment

tools: Add protoxform tests #9241

tools: Add protoxform tests #9241

Conversation

Shikugawa commented Dec 5, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch 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

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch commented Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch commented Dec 13, 2019 •

edited

Loading