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

test: setup showcase testing for codegen modules #1957

Merged
merged 36 commits into from
Jul 7, 2023

Conversation

emmileaf
Copy link
Contributor

@emmileaf emmileaf commented Jun 15, 2023

This PR contains a MVP setup for testing spring-cloud-generator using showcase (context):

  • setup-showcase.sh: script to generate starter module for gapic-showcase: showcase-spring-starter
    • Two options: verify (default, used by the gh actions workflow) and update (-u flag, for updating “golden” showcase module in development of spring generator)
  • EchoAutoConfigurationTests: handwritten unit tests to migrate/replace existing coverage in LanguageAutoConfigurationTests (currently living alongside spring-cloud-previews/google-cloud-language-starter)
  • showcaseTests.yaml: new CI workflow to run showcase verification (generation diff from goldens, compile with handwritten unit tests) for spring-cloud-generator - example presubmit run

First iteration todos:

  • Troubleshoot outstanding errors in migrated handwritten unit tests
  • Script refactoring for generation/post-processing steps
    • Verify this doesn't break actual generation workflow (test run)
  • Add readme (comment)
  • Replace hardcoded gapic-showcase client library version (comment)

@emmileaf
Copy link
Contributor Author

emmileaf commented Jun 16, 2023

The two handwritten unit tests noted above still need more troubleshooting (EDIT: resolved), but marking this PR as ready for an initial review on the setup / approach for adopting showcase in testing spring-cloud-generator (example presubmit workflow run here).

@emmileaf emmileaf marked this pull request as ready for review June 16, 2023 23:04
@emmileaf emmileaf requested a review from a team as a code owner June 16, 2023 23:04
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Maybe this is for another PR, but can you add a short readme under /spring-cloud-generator on how to run the Showcase tests locally? Also, will it be feasible in the future to add integration tests using showcase for the Spring CodeGen starters?

go install github.com/bazelbuild/buildtools/buildozer@latest
export PATH=$PATH:$(go env GOPATH)/bin
buildozer --version
- name: Mvn install
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify what is being built and installed? Is it just the generator or also the showcase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it is the spring-cloud-gcp project from root (local snapshot version, for the generator and dependencies such as spring-cloud-gcp-core). It does not include the showcase client (or showcase spring starters) which are installed through the script. I'll add a comment to clarify.

git clone https://github.com/googleapis/sdk-platform-java.git
git checkout "v${GAPIC_GENERATOR_JAVA_VERSION}"

# Alternative considered: if showcase client library is available on Maven Central,
Copy link
Member

Choose a reason for hiding this comment

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

@blakeli0 Can we just make showcase available in Maven Central?
@emmileaf I'm surprised how complicated this shell script is. Is it because we have to rebuild Showcase from scratch or because we're not able to reuse the Spring CodeGen scripts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, @burkedavison and I discussed this briefly in the past, both of us think it's a good idea. It will make the local developer experience better, but may not simplify this script a lot, probably just this one line
cd sdk-platform-java && mvn clean install -B -ntp -DskipTests -Dclirr.skip -Dcheckstyle.skip that installs showcase to local Maven repo.

Copy link
Contributor Author

@emmileaf emmileaf Jul 4, 2023

Choose a reason for hiding this comment

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

Is it because we have to rebuild Showcase from scratch or because we're not able to reuse the Spring CodeGen scripts?

@meltsufin it's because of the latter (+1 to @blakeli0's comment above).

There are a number of nuances in bazel workspace modifications from the Spring Codegen scripts that rely on googleapis structure, and lines 68-115 is a working attempt to achieve the sdk-platform-java/showcase equivalent for generating the spring starters). Now that #1956 is in, I'll take a look at refactoring this script on top (#1957 (comment)).

Copy link
Member

@burkedavison burkedavison Jul 5, 2023

Choose a reason for hiding this comment

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

discussed this briefly in the past, both of us think it's a good idea.

It could be a good idea given the right use case. The previous use case was binary compatibility testing, but we've now implemented a local version of this check without needing a maven central artifact.

This codegen use case would still require that we checkout the sdk-platform-java repo -- so I'm not sure publishing to maven central buys us anything significant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a bit more context: currently the setup here checks out sdk-platform-java for two reasons:

  1. To locally install the generated showcase client library (not available on Maven Central)
  2. For leaning on bazel generation setup (sdk-platform-java/WORKSPACE, sdk-platform-java/showcase/BUILD.bazel, and protos) instead of writing one from scratch

If having showcase available in Maven Central eliminates the step in 1, a feasible alternative to 2 (to avoid cloning sdk-platform-java) is to lean on googleapis’s bazel generation setup (which the Spring Codegen workflow for actual client libraries use) and follow something similar to these steps - copy protos from gapic-showcase, write a short BUILD.bazel, and make corresponding modifications to googleapis/WORKSPACE.

This is probably not worth the effort if the end goal is just to simplify this script, but could be more relevant if there is preference to avoid cloning sdk-platform-java altogether and reduce the number of repos that Spring Codegen locally depends on.

meltsufin pushed a commit that referenced this pull request Jun 29, 2023
This PR refactors spring-cloud-generator scripts, with the following goals:
* Small fix for pattern-matching when sorting README table entries (follow-up on [1529-comment](#1529 (comment)))
* Post-migration follow-up: simplify amount of script processing and files
  - Different piecewise scripts (e.g. `generate-one.sh`, `setup-build-rule.sh`, ...) consolidated into functions living in `generate-steps.sh`
  - Main script of `generate.sh` contains end-to-end workflow that invokes the various functions defined above
  - This also sets up pieces of the generation generation steps for potential reuse / reducing duplication in the showcase testing generation script ([#1957](#1957)), though related changes are not made in this PR to keep the refactoring incremental.
@emmileaf
Copy link
Contributor Author

emmileaf commented Jul 5, 2023

Maybe this is for another PR, but can you add a short readme under /spring-cloud-generator on how to run the Showcase tests locally?

Ah yeah a readme is a good idea - will add one as part of this PR.

Also, will it be feasible in the future to add integration tests using showcase for the Spring CodeGen starters?

I believe this is doable with adding something like this github action step to install the showcase server, but didn’t scope it into this initial setup PR - I can try getting a PoC started in a separate one.

@@ -19,6 +19,7 @@ fi
SPRING_ROOT_DIR=${SPRING_GENERATOR_DIR}/..

# Reset target folder for generated code
cd ${SPRING_GENERATOR_DIR}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressing a previous suggestion in #1956 (comment): this PR contains small changes to generate.sh and generate_steps.sh to remove assumption that the SPRING_GENERATOR_DIR variable is set in individual helper functions

xmllint --debug --nsclean --xpath "//*[local-name()='module']/text()" $1 \
| sort | uniq | grep -q $2 || module_list_is_empty=1
function add_module_to_aggregator_pom() {
path_to_pom=$1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressing a previous suggestion in #1956 (comment): updated helper functions to consistently declare descriptive variable names for the arguments they expect ($1, $2, ...)

@@ -57,8 +59,6 @@ echo "Library list path: ${LIBRARY_LIST_PATH}"

cd ${SPRING_GENERATOR_DIR}

echo "executing install_spring_generator"
install_spring_generator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed since locally installing spring-cloud-gcp from root (in action) already includes the generator module: #1957 (comment)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants