chore: add unit tests for `utilities.sh` #1948

JoeWang1127 · 2023-08-29T15:22:25Z

Fixes #1947 ☕️

The list of utility functions:

extract_folder_name*
remove_empty_files
mv_src_files
unzip_src_files
find_additional_protos_in_yaml*
search_additional_protos*
get_gapic_opts*
remove_grpc_version*
download_gapic_generator_pom_parent*
get_grpc_version*
get_protobuf_version*
download_tools
download_generator*
download_protobuf*
download_grpc_plugin*
download_from*
copy_from
download_fail

Functions with star (*) are tested in this PR.

suztomo · 2023-08-31T14:19:02Z

library_generation/utilities.sh

@@ -76,15 +76,15 @@ search_additional_protos() {

 # get gapic options from .yaml and .json files from proto_path.
 get_gapic_opts() {
-  gapic_config=$(find "${proto_path}" -type f -name "*gapic.yaml")
+  gapic_config=$(find "$proto_path" -type f -name "*gapic.yaml")


prefer "${var}" over "$var"

https://google.github.io/styleguide/shellguide.html

Thanks for the reference, I'll draft another PR to fix formatting issues.

Thanks Tomo for sharing this! This is really helpful, I see a section regarding when to use shell which I think it's worth discussing more in our next meeting.

blakeli0

Do you plan to have another PR for the unit testing of generate_library.sh?

library_generation/test/generate_library_unit_tests.sh

blakeli0 · 2023-08-31T20:25:44Z

library_generation/utilities.sh

@@ -76,15 +76,15 @@ search_additional_protos() {

 # get gapic options from .yaml and .json files from proto_path.
 get_gapic_opts() {
-  gapic_config=$(find "${proto_path}" -type f -name "*gapic.yaml")
+  gapic_config=$(find "$proto_path" -type f -name "*gapic.yaml")


Thanks Tomo for sharing this! This is really helpful, I see a section regarding when to use shell which I think it's worth discussing more in our next meeting.

blakeli0 · 2023-08-31T20:37:45Z

library_generation/test/generate_library_unit_tests.sh

+  "$gapic_opts"
+}
+
+remove_grpc_version_test() {


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.

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.

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.

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.

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.

JoeWang1127 · 2023-08-31T23:32:59Z

Do you plan to have another PR for the unit testing of generate_library.sh?

I think generate_library.sh is tested through the integration tests since it "hooks" everything.
The result is verified through googleapis-gen (and google-cloud-java later).

blakeli0 · 2023-09-01T18:27:26Z

Do you plan to have another PR for the unit testing of generate_library.sh?

I think generate_library.sh is tested through the integration tests since it "hooks" everything. The result is verified through googleapis-gen (and google-cloud-java later).

Unit tests and Integration tests serve different purposes. Integration tests treat the script somewhat like a black box and make sure it works in a real life scenario. Unit tests treat the script as a white box and attempt to test every combinations with test data.
For example, we have logics to get protobuf version from gapic-generator-java if it is not specified, so we need to have test cases for both protobuf version is specified and is not specified, and technically we need to have two test cases for each if condition.
Now how do we achieve that may not be that simple. For the above case, we could have a test gapic-generator-java pom that has a different protobuf version than the specified protobuf version through the script. If we can achieve the same results(testing every combinations) through googleapis-gen integration tests, that works too, but I feel it may be more complicated.

diegomarquezp · 2023-09-01T18:38:23Z

library_generation/test/generate_library_unit_tests.sh

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


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?

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)

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!

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

diegomarquezp · 2023-09-01T18:44:00Z

library_generation/test/generate_library_unit_tests.sh

+download_generator_success_with_valid_version_test() {
+  download_generator "2.24.0"
+  __assertFileExists "gapic-generator-java-2.24.0.jar"
+  rm "gapic-generator-java-2.24.0.jar"


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

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

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.

JoeWang1127 · 2023-09-02T00:10:46Z

Unit tests and Integration tests serve different purposes. Integration tests treat the script somewhat like a black box and make sure it works in a real life scenario. Unit tests treat the script as a white box and attempt to test every combinations with test data. For example, we have logics to get protobuf version from gapic-generator-java if it is not specified, so we need to have test cases for both protobuf version is specified and is not specified, and technically we need to have two test cases for each if condition. Now how do we achieve that may not be that simple. For the above case, we could have a test gapic-generator-java pom that has a different protobuf version than the specified protobuf version through the script. If we can achieve the same results(testing every combinations) through googleapis-gen integration tests, that works too, but I feel it may be more complicated.

Unit test should test against public APIs so I think we can test generate_library.sh with golden tests: fix the protos and gapic-generator-java version (protobuf and grpc version may or may not provide, but the version should be the same as in gapic-generator-java), verify the script can generate the same source code (compare with golden files). Also, I can write test cases with invalid version of generator, protbuf and grpc to verify generate_library.sh exits with non-zero code. WDYT?

sonarqubecloud · 2023-09-02T00:39:40Z

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarqubecloud · 2023-09-02T00:43:58Z

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarqubecloud · 2023-09-02T00:48:06Z

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

blakeli0 · 2023-09-05T16:45:31Z

I can write test cases with invalid version of generator, protbuf and grpc to verify generate_library.sh exits with non-zero code

SGTM!

I think we can test generate_library.sh with golden tests: fix the protos and gapic-generator-java version (protobuf and grpc version may or may not provide, but the version should be the same as in gapic-generator-java), verify the script can generate the same source code (compare with golden files)

I like the idea, this is similar to the golden unit/integration tests in gapic-generator-java.

blakeli0 · 2023-09-05T16:51:06Z

library_generation/test/resources/alloydb/alloydb_v1.yaml

Do the content of these test files matter? If not or only partially matters, can we simplify the content to only what we need? In addition, it would be great if we can name these test files based on what we are testing, and/or add additional comments on each yaml to indicate what this testing yaml is for. e.g. Something like This yaml is only intended for testing search_additional_protos function with a yaml name search_additional_protos_scenario_xyz.yaml.

This makes sense.

I'll change the name and contents of yaml files in the reformat PR.

blakeli0

LGTM pending a question regarding the testing files. There are a few additional follow ups that need to be done(unit tests and golden tests for generate_library.sh, style change etc), but they don't have to completed as part of this PR.

chore: add unit tests for generate_library.sh

8554d7b

product-auto-label bot added the size: m Pull request size is medium. label Aug 29, 2023

JoeWang1127 added 4 commits August 29, 2023 14:16

add tests

488a67f

merge main

20c7550

add test execution loop

823c974

add test for search_additional_protos

9dcdd8f

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Aug 30, 2023

JoeWang1127 added 2 commits August 30, 2023 10:37

add tests for search_additional_protos

3b1995a

add tests for get_gapic_opts

6cb4180

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Aug 30, 2023

add test for remove_grpc_version

8bfd885

JoeWang1127 changed the title ~~chore: add unit tests for generate_library.sh~~ chore: add unit tests for utilities.sh Aug 30, 2023

add tests for download protobuf and grpc plugin

b8d56d5

JoeWang1127 marked this pull request as ready for review August 30, 2023 18:30

JoeWang1127 requested a review from a team as a code owner August 30, 2023 18:30

JoeWang1127 requested review from suztomo, blakeli0 and diegomarquezp August 30, 2023 18:30

suztomo approved these changes Aug 31, 2023

View reviewed changes

blakeli0 reviewed Aug 31, 2023

View reviewed changes

JoeWang1127 added 3 commits September 1, 2023 12:34

add tests for invalid download test

0f7c164

code reformat

272627a

fix typo

a5db4d2

diegomarquezp reviewed Sep 1, 2023

View reviewed changes

test exit code when download failed

5143528

blakeli0 reviewed Sep 5, 2023

View reviewed changes

blakeli0 approved these changes Sep 5, 2023

View reviewed changes

JoeWang1127 merged commit 6e9ef3f into main Sep 5, 2023

JoeWang1127 deleted the chore/add-unit-tests branch September 5, 2023 17:02

This was referenced Sep 5, 2023

chore: shell code format #1961

Merged

chore: add unit tests for generate_library.sh #1964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add unit tests for `utilities.sh` #1948

chore: add unit tests for `utilities.sh` #1948

JoeWang1127 commented Aug 29, 2023 •

edited

Loading

suztomo Aug 31, 2023

JoeWang1127 Aug 31, 2023 •

edited

Loading

blakeli0 Aug 31, 2023

blakeli0 left a comment

blakeli0 Aug 31, 2023

blakeli0 Aug 31, 2023

JoeWang1127 Aug 31, 2023

blakeli0 Sep 1, 2023 •

edited

Loading

JoeWang1127 Sep 1, 2023 •

edited

Loading

blakeli0 Sep 5, 2023

JoeWang1127 commented Aug 31, 2023 •

edited

Loading

blakeli0 commented Sep 1, 2023 •

edited

Loading

diegomarquezp Sep 1, 2023

diegomarquezp Sep 1, 2023

diegomarquezp Sep 1, 2023

JoeWang1127 Sep 1, 2023

diegomarquezp Sep 1, 2023

diegomarquezp Sep 1, 2023 •

edited

Loading

JoeWang1127 Sep 5, 2023

JoeWang1127 commented Sep 2, 2023

sonarqubecloud bot commented Sep 2, 2023

sonarqubecloud bot commented Sep 2, 2023

sonarqubecloud bot commented Sep 2, 2023

blakeli0 commented Sep 5, 2023

blakeli0 Sep 5, 2023

JoeWang1127 Sep 5, 2023

blakeli0 left a comment

chore: add unit tests for utilities.sh #1948

chore: add unit tests for utilities.sh #1948

Conversation

JoeWang1127 commented Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

JoeWang1127 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 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

blakeli0 Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

JoeWang1127 Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeWang1127 commented Aug 31, 2023 • edited Loading

blakeli0 commented Sep 1, 2023 • edited Loading

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

diegomarquezp Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeWang1127 commented Sep 2, 2023

sonarqubecloud bot commented Sep 2, 2023

sonarqubecloud bot commented Sep 2, 2023

sonarqubecloud bot commented Sep 2, 2023

blakeli0 commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

chore: add unit tests for `utilities.sh` #1948

chore: add unit tests for `utilities.sh` #1948

JoeWang1127 commented Aug 29, 2023 •

edited

Loading

JoeWang1127 Aug 31, 2023 •

edited

Loading

blakeli0 Sep 1, 2023 •

edited

Loading

JoeWang1127 Sep 1, 2023 •

edited

Loading

JoeWang1127 commented Aug 31, 2023 •

edited

Loading

blakeli0 commented Sep 1, 2023 •

edited

Loading

diegomarquezp Sep 1, 2023 •

edited

Loading