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.

Sign up for GitHub

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

chore: add unit tests for utilities.sh #1948

Merged
merged 13 commits into from
Sep 5, 2023
Merged

chore: add unit tests for utilities.sh #1948

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8554d7b
chore: add unit tests for `generate_library.sh`
JoeWang1127 Aug 29, 2023
488a67f
add tests
JoeWang1127 Aug 29, 2023
20c7550
merge main
JoeWang1127 Aug 29, 2023
823c974
add test execution loop
JoeWang1127 Aug 29, 2023
9dcdd8f
add test for `search_additional_protos`
JoeWang1127 Aug 30, 2023
3b1995a
add tests for `search_additional_protos`
JoeWang1127 Aug 30, 2023
6cb4180
add tests for `get_gapic_opts`
JoeWang1127 Aug 30, 2023
8bfd885
add test for `remove_grpc_version`
JoeWang1127 Aug 30, 2023
b8d56d5
add tests for download protobuf and grpc plugin
JoeWang1127 Aug 30, 2023
0f7c164
add tests for invalid download test
JoeWang1127 Sep 1, 2023
272627a
code reformat
JoeWang1127 Sep 1, 2023
a5db4d2
fix typo
JoeWang1127 Sep 1, 2023
5143528
test exit code when download failed
JoeWang1127 Sep 2, 2023
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
Prev Previous commit
Next Next commit
code reformat
  • Loading branch information
@JoeWang1127
JoeWang1127 committed Sep 1, 2023
commit 272627a37f12dadb392a16731c0265f731d1414a
140 changes: 77 additions & 63 deletions library_generation/test/generate_library_unit_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,138 +8,150 @@ succeed_num=0
failed_num=0
# Unit tests against ./utilities.sh
script_dir=$(dirname "$(readlink -f "$0")")
source "$script_dir"/../utilities.sh
source "${script_dir}"/../utilities.sh

assertEquals() {
# Helper functions, they shouldn't be called outside this file.
__assertEquals() {
expected=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

go/shell-style#use-local-variables
Functions should declare their local variables with the local keyword. We have several functions (not only in this PR) around that are not following this style. cc: @blakeli0 @suztomo would it make sense to have a "styling up" PR to make our scripts more compliant with the guidelines?

Copy link
Contributor

Choose a reason for hiding this comment

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

go/shell-style#constants-environment-variables-and-readonly-variables
Another example, to have constants with upper case (e.g. SCRIPT_DIR instead of script_dir)

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, I saw at the end of the changes we have https://github.com/googleapis/sdk-platform-java/pull/1948/files#r1311724425 to address this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I plan to propose a PR to format code after merging this one.

actual=$2
if [[ "$expected" == "$actual" ]]; then
if [[ "${expected}" == "${actual}" ]]; then
return 0
fi

echo "Error: expected $expected, got $actual instead."
echo "Error: expected ${expected}, got ${actual} instead."
return 1
}

assertFileExists() {
__assertFileExists() {
expected_file=$1
if [ -e "$expected_file" ]; then
if [ -e "${expected_file}" ]; then
return 0
fi

echo "Error: $expected_file does not exist."
echo "Error: ${expected_file} does not exist."
return 1
}

assertFileDoesNotExist() {
__assertFileDoesNotExist() {
expected_file=$1
if [ ! -f "$expected_file" ]; then
if [ ! -f "${expected_file}" ]; then
return 0
fi

echo "Error: $expected_file exists."
echo "Error: ${expected_file} exists."
return 1
}

test_executed() {
__test_executed() {
total_num=$((1 + total_num))
}

test_succeed() {
__test_succeed() {
succeed_num=$((1 + succeed_num))
}

test_failed() {
__test_failed() {
failed_num=$((1 + failed_num))
}

# Unit tests
extract_folder_name_test() {
path="google/cloud/aiplatform/v1/google-cloud-aiplatform-v1-java"
folder_name=$(extract_folder_name "$path")
assertEquals "google-cloud-aiplatform-v1-java" "$folder_name"
folder_name=$(extract_folder_name "${path}")
__assertEquals "google-cloud-aiplatform-v1-java" "${folder_name}"
}

get_grpc_version_test() {
get_grpc_version_succeed_with_valid_generator_version_test() {
actual_version=$(get_grpc_version "2.24.0")
assertEquals "1.56.1" "$actual_version"
__assertEquals "1.56.1" "${actual_version}"
rm "gapic-generator-java-pom-parent-2.24.0.pom"
}

get_protobuf_version_test() {
get_grpc_version_failed_with_invalid_generator_version_test() {
actual_version=$(get_grpc_version "1.99.0")
__assertEquals "" "${actual_version}"
}

get_protobuf_version_succeed_with_valid_generator_version_test() {
actual_version=$(get_protobuf_version "2.24.0")
assertEquals "23.2" "$actual_version"
__assertEquals "23.2" "${actual_version}"
rm "gapic-generator-java-pom-parent-2.24.0.pom"
}

get_protobuf_version_failed_with_invalid_generator_version_test() {
actual_version=$(get_protobuf_version "1.99.0")
__assertEquals "" "${actual_version}"
}

search_additional_protos_common_resources_test() {
proto_path="$script_dir/resources/monitoring"
proto_path="${script_dir}/resources/monitoring"
addition_protos=$(search_additional_protos)
assertEquals "google/cloud/common_resources.proto" "$addition_protos"
__assertEquals "google/cloud/common_resources.proto" "${addition_protos}"
}

search_additional_protos_iam_test() {
proto_path="$script_dir/resources/pubsub"
proto_path="${script_dir}/resources/pubsub"
addition_protos=$(search_additional_protos)
assertEquals \
__assertEquals \
"google/cloud/common_resources.proto google/iam/v1/iam_policy.proto" \
"$addition_protos"
"${addition_protos}"
}

search_additional_protos_location_test() {
proto_path="$script_dir/resources/firestore"
proto_path="{$script_dir}/resources/firestore"
addition_protos=$(search_additional_protos)
assertEquals \
__assertEquals \
"google/cloud/common_resources.proto google/cloud/location/locations.proto" \
"$addition_protos"
"${addition_protos}"
}

search_additional_protos_iam_location_test() {
proto_path="$script_dir/resources/alloydb"
proto_path="${script_dir}/resources/alloydb"
addition_protos=$(search_additional_protos)
assertEquals \
__assertEquals \
"google/cloud/common_resources.proto google/iam/v1/iam_policy.proto google/cloud/location/locations.proto" \
"$addition_protos"
"${addition_protos}"
}

get_gapic_opts_test() {
proto_path="$script_dir/resources/monitoring"
get_gapic_opts_with_rest_test() {
proto_path="${script_dir}/resources/monitoring"
transport="grpc"
rest_numeric_enums="true"
gapic_opts="$(get_gapic_opts)"
assertEquals \
"transport=grpc,rest-numeric-enums,grpc-service-config=$proto_path/monitoring_grpc_service_config.json,gapic-config=$proto_path/monitoring_gapic.yaml,api-service-config=$proto_path/monitoring.yaml" \
"$gapic_opts"
__assertEquals \
"transport=grpc,rest-numeric-enums,grpc-service-config=${proto_path}/monitoring_grpc_service_config.json,gapic-config=${proto_path}/monitoring_gapic.yaml,api-service-config=${proto_path}/monitoring.yaml" \
"${gapic_opts}"
}

get_gapic_opts_without_rest_test() {
proto_path="$script_dir/resources/monitoring"
proto_path="${script_dir}/resources/monitoring"
transport="grpc"
rest_numeric_enums="false"
gapic_opts="$(get_gapic_opts)"
assertEquals \
"transport=grpc,grpc-service-config=$proto_path/monitoring_grpc_service_config.json,gapic-config=$proto_path/monitoring_gapic.yaml,api-service-config=$proto_path/monitoring.yaml" \
__assertEquals \
"transport=grpc,grpc-service-config=${proto_path}/monitoring_grpc_service_config.json,gapic-config=${proto_path}/monitoring_gapic.yaml,api-service-config=${proto_path}/monitoring.yaml" \
"$gapic_opts"
}

remove_grpc_version_test() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not directly related to the tests, I wonder why we have to remove the gRPC version manually, what made the differences? Separately, I think it might be good to keep the version for troubleshooting/informational purposes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When using bazel build, there's a "by gRPC proto compiler" at the beginning of *Grpc.java file. However, the annotation becomes "by gRPC proto compiler xxx" when using shell script, where xxx is the grpc version.

I use this function to remove the version number so that two files are totally identical.

Copy link
Collaborator

@blakeli0 blakeli0 Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I thought bazel build eventually calls the same gRPC proto compiler? If not, we need to understand what exactly caused the differences, as there could be more differences when generating other services.

Copy link
Collaborator Author

@JoeWang1127 JoeWang1127 Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think bazel build download the gRPC proto compiler source code from github (defined here in gax/repository.bzl) and build from source.

In generate_library.sh, it download gPRC compiler executable from maven central. I don't know whether there are differences but the generated code are the same.

I haven't seen any code difference when generating other services and I'll add more libraries to the integration test to make sure the generated code is the same as bazel build.

I also post a question and hope someone from grpc team has the answer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think bazel build download the gRPC proto compiler source code from github (defined here in gax/repository.bzl) and build from source.

Thanks, this makes sense! Since we were building from source, maybe there is no concept of version on main branch, let's see if anyone answers the question.

destination_path="$script_dir/resources/monitoring"
cp "$destination_path/QueryServiceGrpc_copy.java" "$destination_path/QueryServiceGrpc.java"
destination_path="${script_dir}/resources/monitoring"
cp "${destination_path}/QueryServiceGrpc_copy.java" "${destination_path}/QueryServiceGrpc.java"
remove_grpc_version
return_code=0
if grep -q 'value = "by gRPC proto compiler",' "$destination_path/QueryServiceGrpc.java"; then
if grep -q 'value = "by gRPC proto compiler",' "${destination_path}/QueryServiceGrpc.java"; then
echo "grpc version is removed."
else
echo "Error: grpc version is not removed."
return_code=1
fi

rm "$destination_path/QueryServiceGrpc.java"
return "$return_code"
rm "${destination_path}/QueryServiceGrpc.java"
return "${return_code}"
}

download_generator_success_with_valid_version_test() {
download_generator "2.24.0"
assertFileExists "gapic-generator-java-2.24.0.jar"
__assertFileExists "gapic-generator-java-2.24.0.jar"
rm "gapic-generator-java-2.24.0.jar"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can have an "afterEach" function that cleans up the downloaded jars/tars, even if they don't exist. It could even be useful for local development where I personally find it a bit hard to read dir contents after I call the scripts

Copy link
Contributor

@diegomarquezp diegomarquezp Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This could be expanded with a git status to confirm we don't have any unexpected untracked files that aren't deleted by such function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not all tests have clean up actions so it's difficult to write a function and apply it after every test execution.

However, I'll think about it in the reformat PR.

}

Expand All @@ -150,64 +162,66 @@ download_generator_failed_with_invalid_version_test() {
# the other tests can continue executing in the current
# shell.
$(download_generator "1.99.0")
assertFileDoesNotExist "gapic-generator-java-1.99.0.jar"
__assertFileDoesNotExist "gapic-generator-java-1.99.0.jar"
}

download_protobuf_succeed_with_valid_version_linux_test() {
download_protobuf "23.2" "linux-x86_64"
assertFileExists "protobuf-23.2"
__assertFileExists "protobuf-23.2"
rm -rf "protobuf-23.2"
}

download_protobuf_succeed_with_valid_version_macos_test() {
download_protobuf "23.2" "osx-x86_64"
assertFileExists "protobuf-23.2"
__assertFileExists "protobuf-23.2"
rm -rf "protobuf-23.2" "google"
}

download_protobuf_failed_with_invalid_version_linux_test() {
$(download_protobuf "22.99" "linux-x86_64")
assertFileDoesNotExist "protobuf-22.99"
__assertFileDoesNotExist "protobuf-22.99"
}

download_protobuf_failed_with_invalid_arch_test() {
$(download_protobuf "23.2" "customized-x86_64")
assertFileDoesNotExist "protobuf-23.2"
__assertFileDoesNotExist "protobuf-23.2"
}

download_grpc_plugin_succeed_with_valid_version_linux_test() {
download_grpc_plugin "1.55.1" "linux-x86_64"
assertFileExists "protoc-gen-grpc-java-1.55.1-linux-x86_64.exe"
__assertFileExists "protoc-gen-grpc-java-1.55.1-linux-x86_64.exe"
rm "protoc-gen-grpc-java-1.55.1-linux-x86_64.exe"
}

download_grpc_plugin_succeed_with_valid_version_macos_test() {
download_grpc_plugin "1.55.1" "osx-x86_64"
assertFileExists "protoc-gen-grpc-java-1.55.1-osx-x86_64.exe"
__assertFileExists "protoc-gen-grpc-java-1.55.1-osx-x86_64.exe"
rm "protoc-gen-grpc-java-1.55.1-osx-x86_64.exe"
}

download_grpc_plugin_failed_with_invalid_version_linux_test() {
$(download_grpc_plugin "0.99.0" "linux-x86_64")
assertFileDoesNotExist "protoc-gen-grpc-java-0.99.0-linux-x86_64.exe"
__assertFileDoesNotExist "protoc-gen-grpc-java-0.99.0-linux-x86_64.exe"
}

download_grpc_plugin_failed_with_invalid_arch_test() {
$(download_grpc_plugin "1.55.1" "customized-x86_64")
assertFileDoesNotExist "protoc-gen-grpc-java-1.55.1-customized-x86_64.exe"
__assertFileDoesNotExist "protoc-gen-grpc-java-1.55.1-customized-x86_64.exe"
}

# Execute tests.
# One line per test.
test_list=(
extract_folder_name_test
get_grpc_version_test
get_protobuf_version_test
get_grpc_version_succeed_with_valid_generator_version_test
get_grpc_version_failed_with_invalid_generator_version_test
get_protobuf_version_succeed_with_valid_generator_version_test
get_protobuf_version_failed_with_invalid_generator_version_test
search_additional_protos_common_resources_test
search_additional_protos_iam_test
search_additional_protos_location_test
search_additional_protos_iam_location_test
get_gapic_opts_test
get_gapic_opts_with_rest_test
get_gapic_opts_without_rest_test
remove_grpc_version_test
download_protobuf_succeed_with_valid_version_linux_test
Expand All @@ -221,20 +235,20 @@ test_list=(
)

for ut in "${test_list[@]}"; do
pushd "$script_dir"
test_executed
pushd "${script_dir}"
__test_executed
result=0
"$ut" || result=$?
if [[ "$result" == 0 ]]; then
test_succeed
if [[ "${result}" == 0 ]]; then
__test_succeed
else
test_failed
__test_failed
fi
popd
done

echo "Test result: $total_num tests executed, $succeed_num succeed, $failed_num failed."
if [[ "$total_num" == "$succeed_num" ]]; then
echo "Test result: ${total_num} tests executed, ${succeed_num} succeed, ${failed_num} failed."
if [[ "${total_num}" == "${succeed_num}" ]]; then
echo "All tests passed."
exit
fi
Expand Down