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

tools: Add protoxform tests #9241

Merged
merged 23 commits into from
Dec 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,8 @@ elif [[ "$CI_TARGET" == "bazel.fuzzit" ]]; then
elif [[ "$CI_TARGET" == "fix_format" ]]; then
# proto_format.sh needs to build protobuf.
setup_clang_toolchain
echo "protoxform_test..."
./tools/protoxform_test.sh
echo "fix_format..."
./tools/check_format.py fix
./tools/format_python_tools.sh fix
Expand Down
9 changes: 9 additions & 0 deletions tools/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ envoy_py_test_binary(
],
)

py_library(
name = "run_command",
srcs = [
"run_command.py",
],
srcs_version = "PY3",
visibility = ["//visibility:public"],
)

envoy_cc_binary(
name = "bootstrap2pb",
srcs = ["bootstrap2pb.cc"],
Expand Down
34 changes: 9 additions & 25 deletions tools/check_format_test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from __future__ import print_function

from run_command import runCommand
import argparse
import logging
import os
Expand All @@ -21,30 +22,13 @@
errors = 0


# Echoes and runs an OS command, returning exit status and the captured
# stdout+stderr as a string array.
def runCommand(command):
stdout = []
status = 0
try:
out = subprocess.check_output(command, shell=True, stderr=subprocess.STDOUT).strip()
if out:
stdout = out.decode('utf-8').split("\n")
except subprocess.CalledProcessError as e:
status = e.returncode
for line in e.output.splitlines():
stdout.append(line)
logging.info("%s" % command)
return status, stdout


# Runs the 'check_format' operation, on the specified file, printing
# the comamnd run and the status code as well as the stdout, and returning
# all of that to the caller.
def runCheckFormat(operation, filename):
command = check_format + " " + operation + " " + filename
status, stdout = runCommand(command)
return (command, status, stdout)
status, stdout, stderr = runCommand(command)
return (command, status, stdout + stderr)


def getInputFile(filename, extra_input_files=None):
Expand Down Expand Up @@ -80,10 +64,10 @@ def fixFileExpectingSuccess(file, extra_input_files=None):
print("FAILED:")
emitStdoutAsError(stdout)
return 1
status, stdout = runCommand('diff ' + outfile + ' ' + infile + '.gold')
status, stdout, stderr = runCommand('diff ' + outfile + ' ' + infile + '.gold')
if status != 0:
print("FAILED:")
emitStdoutAsError(stdout)
emitStdoutAsError(stdout + stderr)
return 1
return 0

Expand All @@ -92,23 +76,23 @@ def fixFileExpectingNoChange(file):
command, infile, outfile, status, stdout = fixFileHelper(file)
if status != 0:
return 1
status, stdout = runCommand('diff ' + outfile + ' ' + infile)
status, stdout, stderr = runCommand('diff ' + outfile + ' ' + infile)
if status != 0:
logging.error(file + ': expected file to remain unchanged')
return 1
return 0


def emitStdoutAsError(stdout):
logging.error("\n".join(line.decode('utf-8') for line in stdout))
logging.error("\n".join(stdout))


def expectError(filename, status, stdout, expected_substring):
if status == 0:
logging.error("%s: Expected failure `%s`, but succeeded" % (filename, expected_substring))
return 1
for line in stdout:
if expected_substring in line.decode('utf-8'):
if expected_substring in line:
return 0
logging.error("%s: Could not find '%s' in:\n" % (filename, expected_substring))
emitStdoutAsError(stdout)
Expand Down Expand Up @@ -283,4 +267,4 @@ def runChecks():
if errors != 0:
logging.error("%d FAILURES" % errors)
exit(1)
logging.warning("PASS")
logging.warning("PASS")
2 changes: 1 addition & 1 deletion tools/protoxform/protoxform.py
Original file line number Diff line number Diff line change
Expand Up @@ -535,4 +535,4 @@ def Main():


if __name__ == '__main__':
Main()
Main()
13 changes: 13 additions & 0 deletions tools/protoxform_test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/bash

rm -rf bazel-bin/tools

declare -r PROTO_TARGETS=$(bazel query "labels(srcs, labels(deps, //tools/testdata/protoxform:protos))")

BAZEL_BUILD_OPTIONS+=" --remote_download_outputs=all"

bazel build ${BAZEL_BUILD_OPTIONS} --//tools/api_proto_plugin:default_type_db_target=//tools/testdata/protoxform:protos \
//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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

117 changes: 117 additions & 0 deletions tools/protoxform_test_helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
#!/usr/bin/env python3

from run_command import runCommand

import logging
import os
import re
import sys


def PathAndFilename(label):
Shikugawa marked this conversation as resolved.
Show resolved Hide resolved
"""Retrieve actual path and filename from bazel label

Args:
label: bazel label to specify target proto.

Returns:
actual path and filename
"""
if label.startswith('/'):
label = label.replace('//', '/', 1)
elif label.startswith('@'):
label = re.sub(r'@.*/', '/', label)
else:
return label
label = label.replace(":", "/")
splitted_label = label.split('/')
return ['/'.join(splitted_label[:len(splitted_label) - 1]), splitted_label[-1]]


def GoldenProtoFile(path, filename, version):
"""Retrieve golden proto file path. In general, those are placed in tools/testdata/protoxform.

Args:
path: target proto path
filename: target proto filename
version: api version to specify target golden proto filename

Returns:
actual golden proto absolute path
"""
base = "./"
base += path + "/" + filename + "." + version + ".gold"
return os.path.abspath(base)


def ResultProtoFile(path, filename, version):
"""Retrieve result proto file path. In general, those are placed in bazel artifacts.

Args:
path: target proto path
filename: target proto filename
version: api version to specify target result proto filename

Returns:
actual result proto absolute path
"""
base = "./bazel-bin"
base += os.path.join(path, "protos")
base += os.path.join(base, path)
base += "/{0}.{1}.proto".format(filename, version)
return os.path.abspath(base)


def Diff(result_file, golden_file):
"""Execute diff command with unified form

Args:
result_file: result proto file
golden_file: golden proto file

Returns:
output and status code
"""
command = 'diff -u '
command += result_file + ' '
command += golden_file
status, stdout, stderr = runCommand(command)
return [status, stdout, stderr]


def Run(path, filename, version):
"""Run main execution for protoxform test

Args:
path: target proto path
filename: target proto filename
version: api version to specify target result proto filename

Returns:
result message extracted from diff command
"""
message = ""
golden_path = GoldenProtoFile(path, filename, version)
test_path = ResultProtoFile(path, filename, version)

status, stdout, stderr = Diff(test_path, golden_path)

if status != 0:
message = '\n'.join([str(line) for line in stdout + stderr])

return message


if __name__ == "__main__":
messages = ""
logging.basicConfig(format='%(message)s')
path, filename = PathAndFilename(sys.argv[1])
messages += Run(path, filename, 'v2')
messages += Run(path, filename, 'v3alpha')

if len(messages) == 0:
logging.warning("PASS")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sys.exit(0)
else:
logging.error("FAILED:\n{}".format(messages))
sys.exit(1)
10 changes: 10 additions & 0 deletions tools/run_command.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import subprocess


# Echoes and runs an OS command, returning exit status and the captured
# stdout and stderr as a string array.
def runCommand(command):
Shikugawa marked this conversation as resolved.
Show resolved Hide resolved
proc = subprocess.run([command], shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just do capture_output=True.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyway, add issue #9339

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


return proc.returncode, proc.stdout.decode('utf-8').split('\n'), proc.stderr.decode(
'utf-8').split('\n')
7 changes: 7 additions & 0 deletions tools/spelling_dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ API
ASAN
ASCII
ASSERTs
AST
AWS
BACKTRACE
BSON
Expand Down Expand Up @@ -302,6 +303,7 @@ WKT
WRR
WS
Welford's
Wi
XDS
XFCC
XFF
Expand Down Expand Up @@ -419,6 +421,7 @@ codings
combinatorial
comparator
compat
compiation
completable
cond
condvar
Expand Down Expand Up @@ -525,6 +528,7 @@ evwatch
exe
execlp
expectable
extrahelp
Shikugawa marked this conversation as resolved.
Show resolved Hide resolved
faceplant
facto
failover
Expand Down Expand Up @@ -645,13 +649,15 @@ lexically
libc
libevent
libstdc
libtooling
lifecycle
lightstep
linearization
linearize
linearized
linux
livelock
llvm
localhost
lockless
login
Expand All @@ -665,6 +671,7 @@ lowp
ltrim
lua
lyft
mabye
maglev
malloc
matchable
Expand Down
9 changes: 9 additions & 0 deletions tools/testdata/protoxform/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
licenses(["notice"]) # Apache 2

proto_library(
name = "protos",
visibility = ["//visibility:public"],
deps = [
"//tools/testdata/protoxform/envoy/v2:protos",
],
)
12 changes: 12 additions & 0 deletions tools/testdata/protoxform/envoy/v2/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
licenses(["notice"]) # Apache 2

proto_library(
name = "protos",
srcs = [
"sample.proto",
],
visibility = ["//visibility:public"],
deps = [
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
15 changes: 15 additions & 0 deletions tools/testdata/protoxform/envoy/v2/sample.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
syntax = "proto3";

package envoy.v2;

import "udpa/annotations/migrate.proto";

message Sample {
message Entry {
string key = 1;
string value = 2;
}
repeated Entry entries = 1;
string will_deprecated = 2 [deprecated = true];
string will_rename_compoent = 3 [(udpa.annotations.field_migrate).rename = "renamed_component"];
}
23 changes: 23 additions & 0 deletions tools/testdata/protoxform/envoy/v2/sample.proto.v2.gold
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
syntax = "proto3";

package envoy.v2;

option java_package = "io.envoyproxy.envoy.v2";
option java_outer_classname = "SampleProto";
option java_multiple_files = true;
Shikugawa marked this conversation as resolved.
Show resolved Hide resolved

import "udpa/annotations/migrate.proto";

message Sample {
message Entry {
string key = 1;

string value = 2;
}

repeated Entry entries = 1;

string will_deprecated = 2 [deprecated = true];

string will_rename_compoent = 3 [(udpa.annotations.field_migrate).rename = "renamed_component"];
}
Loading