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

[ggj][infra][3/5] feat: implement update golden bazel rules for dummy test #314

Merged
merged 45 commits into from
Sep 24, 2020

Conversation

xiaozhenliu-gg5
Copy link
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 commented Sep 18, 2020

Find more details in design doc: go/java-micro-file-diff-infra

  1. Added rules_bazel/java_diff_test.bzl where update_golden bazel rule is defined, including two parts: running JUnit test and overwritting goldens.
  2. Added SingleJUnitTestRunner which helps us to manually trigger the JUnit test, it takes test_class_name as arg and emits test result if any failure is detected.
  3. Added Utils.java in test/framework which helps to save the generated code from JUnit test to tmp file in folder $TEST_OUTPUT_HOME. System environment is set to interact with update golden bazel rules, the folder will get deleted once the bazel rules finish.
  4. update_golden rule is called from BUILD.bazel in dummy test, and junit_runner_binary is defined for running the JUnit test when updating goldens.

How we use it (check out this branch and modify the dummy/goldens/*.golden)

> bazel test //...
Tests have failures: [classWithHeader(com.google.api.generator.gapic.dummy.FileDiffInfraDummyTest): Differences found: 
--- golden
+++ codegen
@@ -17,3 +17,3 @@
 package com.google.showcase.v1beta1.stub;
 
-public class  {}
+public class EchoStubSettings {}]
//src/test/java/com/google/api/generator/gapic/dummy:FileDiffInfraDummyTest FAILED in 5.6s
  /private/var/tmp/_bazel_xiaozhenliu/27d546162d4fdfcb8b9c40b4c5bf4971/execroot/com_google_api_codegen/bazel-out/darwin-fastbuild/testlogs/src/test/java/com/google/api/generator/gapic/dummy/FileDiffInfraDummyTest/test.log


> bazel run //src/test/java/com/google/api/generator/gapic/dummy:FileDiffInfraDummyTest_update
> bazel test //...
.....
Executed 53 out of 66 tests: 66 tests pass.

Next step would be:

  1. Move the existing expected class string to file-diff infra, including
    (a)gapic/composer/*Test.
    (b)engine/astIntegrationTest
  2. Update developer doc to include this instruction for updating goldens.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

It looks good. I left quite a few comments, but most of them are minor and/or optional.

My only real concern is the general structure of the BUILD.bazel files (if we don't be careful here, we may end up in a very verbose layout, with many BUILD files everywhere and many targets being redeclared in each BUILD file, which will be very difficult to maintain).

dependencies.properties Show resolved Hide resolved
rules_bazel/java/java_diff_test.bzl Outdated Show resolved Hide resolved
rules_bazel/java/java_diff_test.bzl Outdated Show resolved Hide resolved
Copy link
Contributor

@miraleung miraleung left a comment

Choose a reason for hiding this comment

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

It looks good. I left quite a few comments, but most of them are minor and/or optional.

My only real concern is the general structure of the BUILD.bazel files (if we don't be careful here, we may end up in a very verbose layout, with many BUILD files everywhere and many targets being redeclared in each BUILD file, which will be very difficult to maintain).

The motivation for the BUILD file structure in this repo is to keep individual repos self-contained (following google3 style), so the proliferation of files is fine.

Otherwise +1 to Vadym's comments below.

rules_bazel/java/java_diff_test.bzl Show resolved Hide resolved
rules_bazel/java/java_diff_test.bzl Show resolved Hide resolved
rules_bazel/java/java_diff_test.bzl Show resolved Hide resolved
rules_bazel/java/java_diff_test.bzl Show resolved Hide resolved
rules_bazel/java/java_diff_test.bzl Show resolved Hide resolved
@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit 26611d7 into master Sep 24, 2020
@xiaozhenliu-gg5 xiaozhenliu-gg5 deleted the update_golden_bazel branch September 24, 2020 17:40
@miraleung
Copy link
Contributor

From a clean client, I tried running this update command, and got the error below, plus lots of warnings. Seems that this isn't working and requires each incremental target to be built first, whereas it should all just work with bazel run. I'll fix these issues too.

bazel run //src/test/java/com/google/api/generator/gapic/dummy:FileDiffInfraDummyTest_update

Target //src/test/java/com/google/api/generator/gapic/dummy:FileDiffInfraDummyTest_update up-to-date:
  bazel-bin/src/test/java/com/google/api/generator/gapic/dummy/FileDiffInfraDummyTest_update.sh
INFO: Elapsed time: 8.211s, Critical Path: 0.06s
INFO: 0 processes.
INFO: Build completed successfully, 4 total actions
INFO: Build completed successfully, 4 total actions
unzip:  cannot find or open bazel-out/k8-fastbuild/bin/src/test/java/com/google/api/generator/gapic/dummy/FileDiffInfraDummyTest_update_output.zip, bazel-out/k8-fastbuild/bin/src/test/java/com/google/api/generator/gapic/dummy/FileDiffInfraDummyTest_update_output.zip.zip or bazel-out/k8-fastbuild/bin/src/test/java/com/google/api/generator/gapic/dummy/FileDiffInfraDummyTest_update_output.zip.ZIP.

@miraleung
Copy link
Contributor

Another one I found: In a clean client, I get this error, even though it updates fine:

Error occurs when reading golden file

@miraleung
Copy link
Contributor

Fixed all of these in #363.

@xiaozhenliu-gg5
Copy link
Contributor Author

This implementation was based on the prerequisites of users running bazel test or bazel build first before updating any goldens files. The usage was described in the description, but it does not support directly bazel run testTarget_update for updating goldens before any test run.
Error occurs when reading golden file only happens when not finding/reading the goldens files, I did not have chance to repro that error, but thanks for the refactor to allow the direct usage of update_goldens.

suztomo pushed a commit that referenced this pull request Dec 16, 2022
* chore: fix auto-release

* chore: remove codecov.yml

* chore: update license headers for yaml files
Source-Link: googleapis/synthtool@5b77727
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:ebc2104854c5b81c6fd72ca79400a2e20e0d510c5e0654fd1a19e5c9be160ca6
suztomo pushed a commit that referenced this pull request Dec 16, 2022
🤖 I have created a release *beep* *boop*
---


### [1.2.11](googleapis/java-iam@v1.2.10...v1.2.11) (2022-03-25)


### Dependencies

* update dependency com.google.api:api-common to v2.1.5 ([#313](googleapis/java-iam#313)) ([08a700e](googleapis/java-iam@08a700e))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
suztomo pushed a commit that referenced this pull request Mar 21, 2023
not needed since we removed v2 solution - googleapis/synthtool#964

Source-Author: Emily Ball <emilyball@google.com>
Source-Date: Mon Mar 29 14:47:37 2021 -0700
Source-Repo: googleapis/synthtool
Source-Sha: 572ef8f70edd9041f5bcfa71511aed6aecfc2098
Source-Link: googleapis/synthtool@572ef8f
suztomo pushed a commit that referenced this pull request Mar 21, 2023
🤖 I have created a release \*beep\* \*boop\* 
---
### [1.93.10](https://www.github.com/googleapis/java-core/compare/v1.93.9...v1.93.10) (2020-10-30)


### Dependencies

* update core dependencies ([#294](https://www.github.com/googleapis/java-core/issues/294)) ([5f6b784](https://www.github.com/googleapis/java-core/commit/5f6b784ad94b71553d339e3450b17f70dd307e6d))
* update core transport dependencies ([#295](https://www.github.com/googleapis/java-core/issues/295)) ([39fdd06](https://www.github.com/googleapis/java-core/commit/39fdd06a84a92c2d314c9de8b1baba0cd5b6589d))
* update dependency com.google.api:api-common to v1.10.1 ([#302](https://www.github.com/googleapis/java-core/issues/302)) ([6ebd6b1](https://www.github.com/googleapis/java-core/commit/6ebd6b186dc99bc0a306e2b28150d84c6e6d8125))
* update dependency com.google.api.grpc:proto-google-iam-v1 to v1.0.2 ([#312](https://www.github.com/googleapis/java-core/issues/312)) ([e416c1c](https://www.github.com/googleapis/java-core/commit/e416c1cafdafac7165f986194a028096ff936993))
* update dependency com.google.guava:guava-bom to v30 ([#310](https://www.github.com/googleapis/java-core/issues/310)) ([1026809](https://www.github.com/googleapis/java-core/commit/1026809177b3460a7170e13cb205b8505bde1ddb))
* update dependency io.grpc:grpc-bom to v1.33.0 ([#309](https://www.github.com/googleapis/java-core/issues/309)) ([e021cb0](https://www.github.com/googleapis/java-core/commit/e021cb0943b7b538df79941f98c46cbc53633959))
* update dependency org.threeten:threetenbp to v1.4.5 ([#297](https://www.github.com/googleapis/java-core/issues/297)) ([9286f76](https://www.github.com/googleapis/java-core/commit/9286f761449ae51bd7ae4fe84494f9b987a45623))
* update dependency org.threeten:threetenbp to v1.5.0 ([#314](https://www.github.com/googleapis/java-core/issues/314)) ([35bcf4d](https://www.github.com/googleapis/java-core/commit/35bcf4d3ff1f47295a466ff37685b7b526af2b1a))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants