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

chore: make integration test infrastructure sane #904

Merged
merged 5 commits into from
Jan 21, 2022

Conversation

chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Jan 21, 2022

The you-can-do-anything-in-bazel mentality can easily lure one to a state where you are programming a build with a build tool instead of configuring a build.

I just extracted the shell script portion out of a .bzl file into a file.

This should also help switching to any build system from Bazel.

@chanseokoh chanseokoh requested review from a team as code owners January 21, 2022 18:29
@@ -0,0 +1,25 @@
#!/bin/sh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script is taken out of integration_test.bzl above.

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #904 (36f0382) into main (bfb35cd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #904   +/-   ##
=======================================
  Coverage   87.86%   87.86%           
=======================================
  Files         153      153           
  Lines       16024    16024           
  Branches     1166     1166           
=======================================
  Hits        14079    14079           
  Misses       1602     1602           
  Partials      343      343           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfb35cd...36f0382. Read the comment docs.

@chanseokoh
Copy link
Contributor Author

Hmm... I will look into the failures on GitHub.

@chanseokoh
Copy link
Contributor Author

Good to go.

@chanseokoh chanseokoh added the automerge Merge the pull request once unit tests and other checks pass. label Jan 21, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit 128039b into main Jan 21, 2022
@gcf-merge-on-green gcf-merge-on-green bot deleted the debazelize-ingetration-test branch January 21, 2022 21:34
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 21, 2022
@vam-google
Copy link
Contributor

vam-google commented Jan 21, 2022

@chanseokoh Please consider reverting this PR as a whole or heavily refactoring it to eliminate the issues.

The issues are:

  1. Before the generated code had also been compiled by bazel before going to golden files diff check, now compilation is not happening anymore. This is a big difference. This is so, because the integration_test rules were consuming gapic target directly (which produces a compiled java binary), and was getting the sources from it, which were guaranteed to be already compiled and formatted properly. Now the build depends on srcjar_raw intermediate artifact, which gets created before compilation, and since the subsequent compilation is not needed for the final test target to execute, bazel will truncate in from build execution tree as redundant.

  2. The build file now directly references the implementation-specific aspects of the rules (the names of intermediate build artifacts, which srcjar_raw is): https://github.com/googleapis/gapic-generator-java/pull/904/files#diff-d0ca181e8f4c33142243bd935d28e96c7fb6d1c3f99dbedb386f209c0f3f52a5R53. Like an oversimplified analogy to this would be accessing private variable form another class directly instead of having a getter method. Basically here bazel gets used as a script-calling tool (a severely complicated one), instead of using it as a rigorous powerful build system.

@chanseokoh
Copy link
Contributor Author

We have offline discussions.

FTR,

now compilation is not happening anymore. This is a big difference.

Turns out this wasn't the case (although it could have been), and therefore there's no difference after this PR.

If it is required to verify if generated libraries are compilable, we should do that. It can be done by adding tests to CI. And in fact, we already have such tests (recently added).

Basically here bazel gets used as a script-calling tool (a severely complicated one)

Bazel was used as a tool to call many severely complicated scripts (embedded ones) anyway. However, one could argue that the complexity is hidden as an implementation detail, where the following argument is in the same vein.

Like an oversimplified analogy to this would be accessing private variable form another class directly instead of having a getter method

I have arguments about this, but here, I'd just leave here that we don't see this as an issue. In fact, I think we are making this better by doing this.

suztomo pushed a commit that referenced this pull request Dec 16, 2022
The you-can-do-anything-in-bazel mentality can easily lure one to a state where you are programming a build with a build tool instead of configuring a build.

I just extracted the shell script portion out of a `.bzl` file into a file.

This should also help switching to any build system from Bazel.
suztomo pushed a commit that referenced this pull request Mar 21, 2023
Fixes #904

Source-Author: Neenu Shaji <[email protected]>
Source-Date: Wed Mar 24 15:50:02 2021 -0400
Source-Repo: googleapis/synthtool
Source-Sha: bb854b6c048619e3be4e8b8ce8ed10aa74ea78ef
Source-Link: googleapis/synthtool@bb854b6
suztomo pushed a commit that referenced this pull request Mar 21, 2023
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
suztomo pushed a commit that referenced this pull request Mar 21, 2023
…-info-reports-plugin to v3.4.1 (#904)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [org.apache.maven.plugins:maven-project-info-reports-plugin](https://maven.apache.org/plugins/) ([source](https://togithub.com/apache/maven-project-info-reports-plugin)) | `3.4.0` -> `3.4.1` | [![age](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-project-info-reports-plugin/3.4.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-project-info-reports-plugin/3.4.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-project-info-reports-plugin/3.4.1/compatibility-slim/3.4.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-project-info-reports-plugin/3.4.1/confidence-slim/3.4.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNTguMCIsInVwZGF0ZWRJblZlciI6IjMyLjE2MS4wIn0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants