Skip to content

Conversation

@JoeWang1127
Copy link
Contributor

@JoeWang1127 JoeWang1127 commented Mar 29, 2024

In this PR:

  • In the integration test, run generate_repo.py and generate_pr_description.py in docker container.
  • Remove test image in cloud build.

Context:
In this log, the nightly generation job only took 3 mins. After investigation, we found out that removing main method in generate_repo.py (#2598) caused the CLI is not running despite that the job returns successfully.

This non-explicit failure demonstrated that there's difference between calling CLI (generate method in generate_repo.py) and its implementation (generate_from_yaml method in generate_repo.py).

We decided to change the integration test to run the CLI in docker container, rather than testing generate_from_yaml because it is the containerized CLI will be used in downstream libraries.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Mar 29, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 2, 2024
@JoeWang1127
Copy link
Contributor Author

I'll remove this cloud build trigger once the PR is approved.

)

@classmethod
def __run_entry_point_in_docker_container(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After creating entry_point.py, there will be only once docker run.

@JoeWang1127 JoeWang1127 marked this pull request as ready for review April 2, 2024 20:00
@JoeWang1127 JoeWang1127 requested a review from a team as a code owner April 2, 2024 20:00
Copy link
Contributor

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

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

LGTM

build_file = os.path.join(
repo_root_dir, ".cloudbuild", "library_generation", "library_generation.Dockerfile"
)
image_tag = f"test-image:latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to use f string as this is a hardcoded value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
os.remove(f"{description_file}")
def test_entry_point_running_in_container(self):
self.__build_image(docker_file=build_file, tag=image_tag, cwd=repo_root_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we want to expose tag as a parameter of IntegrationTest in the future, I don't think we need to pass around tag in this script, we can always use image_tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JoeWang1127 JoeWang1127 requested a review from blakeli0 April 2, 2024 21:34
@JoeWang1127 JoeWang1127 enabled auto-merge (squash) April 2, 2024 21:38
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 2, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 2, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@JoeWang1127 JoeWang1127 merged commit 56775a6 into main Apr 2, 2024
@JoeWang1127 JoeWang1127 deleted the chore/fix-integration-test branch April 2, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants