From d5a151e5606c1cf4acf317fd74d8813f4be50400 Mon Sep 17 00:00:00 2001 From: kirl Date: Thu, 12 Nov 2026 21:32:29 +0000 Subject: [PATCH 1/2] chore(bigquery-jdbc): add testing skills --- .../skills/bigquery-jdbc-testing/SKILL.md | 127 ++++++++++++++++++ .../.claude/skills/bigquery-jdbc-testing | 1 + .../.gemini/skills/bigquery-jdbc-testing | 1 + java-bigquery-jdbc/.gitignore | 4 - 4 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 java-bigquery-jdbc/.agents/skills/bigquery-jdbc-testing/SKILL.md create mode 120000 java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing create mode 120000 java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing diff --git a/java-bigquery-jdbc/.agents/skills/bigquery-jdbc-testing/SKILL.md b/java-bigquery-jdbc/.agents/skills/bigquery-jdbc-testing/SKILL.md new file mode 100644 index 000000000000..12f1fee4873a --- /dev/null +++ b/java-bigquery-jdbc/.agents/skills/bigquery-jdbc-testing/SKILL.md @@ -0,0 +1,127 @@ +--- +name: bigquery-jdbc-testing +description: Conventions for writing and refactoring unit and integration tests in the BigQuery JDBC driver (java-bigquery-jdbc). Use this skill when adding, reviewing, or cleaning up tests under java-bigquery-jdbc/src/test, including tests for connection/credential setup, Storage Read/Write API clients, TPC/universe-domain behavior, and ITBase-derived integration tests. +--- + +# BigQuery JDBC Driver: Writing Tests + +Applies to `java-bigquery-jdbc/src/test/java/com/google/cloud/bigquery/jdbc/**`. + +- **Unit tests** live in `com.google.cloud.bigquery.jdbc` and extend + `BigQueryJdbcBaseTest` (directly or through `BigQueryJdbcLoggingBaseTest`). +- **Integration tests** live in `com.google.cloud.bigquery.jdbc.it`, are named + `IT*`, and extend `ITBase`. + +Aim for tests that are short and read top to bottom: setup comes from shared +fixtures, the test body holds only what is specific to the case, and repeated +assertions live in one helper. + +## 1. Reuse existing fixtures. Don't hand-roll them + +Before writing setup code, check `BigQueryJdbcBaseTest`, +`BigQueryJdbcLoggingBaseTest`, `utils/TestUtilities`, and `utils/URIBuilder`. + +| Need | Use | Avoid | +| --- | --- | --- | +| A private key / PEM string | `fake_pkcs8_key` | `KeyPairGenerator`, Base64 encoding, building `-----BEGIN PRIVATE KEY-----` by hand | +| A service-account connection URL | `getUriOAuthServiceAccount()` | Building `jdbc:bigquery://...;OAuthType=0;...` by string concatenation | +| Other auth types | `getUriOAuthUserAccount()`, `getUriOAuthToken()`, `getUriOAuthApplicationDefault()`, `getUriOAuthExternal()` | Copy-pasting URL fragments | +| Extra URL properties | `.append("Key", value)` on the `URIBuilder`, then `.toString()` | Manual `";"` handling | + +Example: + +```java +String url = + getUriOAuthServiceAccount() + .append("ProjectId", "project") + .append("universeDomain", TPC_UNIVERSE) + .toString(); +``` + +If a fixture you need doesn't exist and more than one test would use it, add it +to the base class instead of keeping a private copy in one test file. + +## 2. Pull repeated assertions into a helper + +When a test asserts the same thing for two or more objects (for example the +Storage **Read** and **Write** clients, or the Arrow and JSON result sets), move +the assertions into one `private static` helper that takes the varying parts +(credentials, endpoint URI, expected scopes, expected universe) as parameters. +Each test then makes one call per object. + +```java +assertStorageCredentialsScoped( + readClient.getSettings().getCredentialsProvider().getCredentials(), + TPC_UNIVERSE, TPC_READ_URI, BigQueryReadSettings.getDefaultServiceScopes()); +assertStorageCredentialsScoped( + writeClient.getSettings().getCredentialsProvider().getCredentials(), + TPC_UNIVERSE, TPC_WRITE_URI, BigQueryWriteSettings.getDefaultServiceScopes()); +``` + +Put repeated literals (universe domains, endpoint URIs, scope lists) in +`private static final` constants at the top of the test class. + +## 3. Cover every branch of the code under test + +For each conditional in the production change, write at least one test that +takes each side. In particular: + +- **Credential type.** If the code special-cases `ServiceAccountCredentials` or + `GoogleCredentials`, also test a credential that is *not* a service account, + such as `getUriOAuthToken()` / `GoogleCredentials.create(AccessToken)`. Check + that it passes through unchanged (`assertSame` where possible). +- **Universe domain.** If the code behaves differently for TPC + (`universeDomain=apis-tpczero.goog`), also test the **default** universe (no + `universeDomain` property, so `googleapis.com`). +- **Already configured input.** For example, credentials that already have + explicit scopes must keep them. + +When you are asked to *refactor* tests, keep coverage the same: every assertion +the old tests made must still be made, possibly through a helper. Don't drop +cases or weaken assertions (for example, changing `assertEquals` to +`assertNotNull`). + +## 4. Integration tests: shared pieces belong in `ITBase` + +- If more than one `IT*` class uses the same query template or check, define it + once in `ITBase` as a `public static final` constant or a `protected`/`public + static` helper. Don't redefine it per class or inline the literal. Current + shared pieces: + - `GENERATE_ARRAY_QUERY` = `"SELECT * FROM UNNEST(GENERATE_ARRAY(1, %s));"`, + used with `String.format(GENERATE_ARRAY_QUERY, n)`. + - `assertArrowResultSet(ResultSet)`, which checks that the High-Throughput + (Storage Read) API path was taken. + - `resultSetRowCount(ResultSet)` and `validateStatement(...)`. +- Don't shadow `ITBase` members with a same-named `private static final` field + that means something else (for example a class-local `BASE_QUERY` that is a + different query). +- Use try-with-resources for `Connection`, `Statement`, and `ResultSet`, or close + them explicitly. IT suites share quotas. +- `@Tag("disable_tpc")` excludes a test from TPC runs. Remove it only when the + feature is known to work against TPC, and in that case add an assertion that + proves the intended path was used (for example `assertArrowResultSet`). + +## 5. Style and verification + +- Follow the repo `AGENTS.md`: least visibility (`private static` helpers), + short names over fully qualified names, no `@Deprecated`/`@ObsoleteApi` + calls, and no unrelated formatting changes. +- Remove imports that the refactor makes unused (for example `KeyPairGenerator`, + `PrivateKey`, `Base64`). +- Run the formatter from `java-bigquery-jdbc/`: + ```bash + make lint # mvn com.spotify.fmt:fmt-maven-plugin:format + ``` + If the formatter fails with `NoSuchMethodError`, your JDK is too new for + google-java-format. Run it under an older JDK, for example + `JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 make lint`. +- Run the affected unit tests: + ```bash + make unittest test=BigQueryConnectionTest + make unittest test='BigQueryConnectionTest#testReadAndWrite*' + ``` +- Integration tests need real credentials (`GOOGLE_APPLICATION_CREDENTIALS`). + If they can't run locally, at least compile them: + ```bash + mvn -q -Denforcer.skip=true -Dclirr.skip=true test-compile + ``` diff --git a/java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing b/java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing new file mode 120000 index 000000000000..dffed6f65a1d --- /dev/null +++ b/java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing @@ -0,0 +1 @@ +../../.agents/skills/bigquery-jdbc-testing \ No newline at end of file diff --git a/java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing b/java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing new file mode 120000 index 000000000000..dffed6f65a1d --- /dev/null +++ b/java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing @@ -0,0 +1 @@ +../../.agents/skills/bigquery-jdbc-testing \ No newline at end of file diff --git a/java-bigquery-jdbc/.gitignore b/java-bigquery-jdbc/.gitignore index b919e39bbe0f..eb60678e7791 100644 --- a/java-bigquery-jdbc/.gitignore +++ b/java-bigquery-jdbc/.gitignore @@ -3,7 +3,6 @@ target-it/** **/*logs*/** **/ITBigQueryJDBCLocalTest.java **/BigQueryStatementE2EBenchmark.java -.agents/ tools/**/*.class tools/**/drivers/** @@ -11,6 +10,3 @@ tools/**/logs/** tools/**/*.jfr tools/**/odbc/ tools/**/native_odbc_perf - -# Gemini/Jetski agent custom skills -.agents/ \ No newline at end of file From 03b49fef16521870168c869a8c89b061222f54f2 Mon Sep 17 00:00:00 2001 From: kirl Date: Thu, 12 Nov 2026 22:48:08 +0000 Subject: [PATCH 2/2] chore(bigquery-jdbc): add prepare-pr skill; share skills via .agents/skills --- .../.agents/skills/prepare-pr/SKILL.md | 189 ++++++++++++++++++ java-bigquery-jdbc/.claude/skills | 1 + .../.claude/skills/bigquery-jdbc-testing | 1 - .../.gemini/skills/bigquery-jdbc-testing | 1 - java-bigquery-jdbc/.gitignore | 4 + 5 files changed, 194 insertions(+), 2 deletions(-) create mode 100644 java-bigquery-jdbc/.agents/skills/prepare-pr/SKILL.md create mode 120000 java-bigquery-jdbc/.claude/skills delete mode 120000 java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing delete mode 120000 java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing diff --git a/java-bigquery-jdbc/.agents/skills/prepare-pr/SKILL.md b/java-bigquery-jdbc/.agents/skills/prepare-pr/SKILL.md new file mode 100644 index 000000000000..a9339996ce3f --- /dev/null +++ b/java-bigquery-jdbc/.agents/skills/prepare-pr/SKILL.md @@ -0,0 +1,189 @@ +--- +name: prepare-pr +description: Prepare a pull request for the BigQuery JDBC driver (java-bigquery-jdbc) and get it independently reviewed. Before a PR is created, pushed, or marked ready for review, a reviewer agent on a different model checks the code and the PR description. The author fixes or argues against each finding, and a fresh reviewer re-checks. The loop repeats until both the author and the reviewer are satisfied. Use this skill whenever you are about to open, update, or finish a PR. +--- + +# Preparing a PR with external review + +When you (the **author agent**) finish a change, you aren't done when your own +tests pass. Get an independent review from an agent running a **different +model** first, and repeat until you and the reviewer agree the PR is ready. + +```mermaid +flowchart TD + A["1. Finish change, run local checks"] --> B["2. Write PR description, build context bundle"] + B --> C["3. Start a FRESH reviewer on a different model"] + C --> D{"Reviewer verdict"} + D -->|"APPROVE, nothing blocking open"| G{"Author satisfied?"} + D -->|"CHANGES_REQUESTED"| E["4. Author answers every finding: FIXED or PUSHBACK with a reason"] + E --> F["5. Re-run local checks, update description and ledger"] + F --> C + G -->|"yes"| H["6. Done: push or open the PR, report to user"] + G -->|"no, author has open concerns"| E +``` + +## 1. Finish the change and run local checks + +Before you ask anyone to review, the change must build and pass its tests: + +- Format: `make lint` from `java-bigquery-jdbc/`. +- Unit tests for the classes you touched: `make unittest test=`. +- Integration tests compile at least: `mvn -q -Denforcer.skip=true -Dclirr.skip=true test-compile`. +- Follow the repo `AGENTS.md` and, for test changes, the `bigquery-jdbc-testing` skill. + +## 2. Write the PR description and build the context bundle + +Write the PR description now, not after the review. The reviewer checks it too. +It has: + +- **Title** in Conventional Commits form: `fix(bigquery-jdbc): ...`. +- **What** changed and **why** (the bug, the issue link, or the requirement). +- **How it was tested**: the commands you ran, new tests, and anything you + couldn't run (for example integration tests that need credentials). +- **Risks / follow-ups**, if any. + +Build a **context bundle** for the reviewer: + +1. The PR description, word for word. +2. The full diff against the base branch: `git diff origin/main...HEAD` plus any + uncommitted changes (`git diff HEAD`). +3. The list of changed files (`git diff --name-only origin/main...HEAD`) and the + repo path, so the reviewer can open surrounding code itself. +4. From round 2 on, the **review ledger** (see step 4). + +Keep the bundle and ledger in a scratch location **outside the repository** (for +example the agent's scratch or artifact directory, or `/tmp`). Never commit them. + +## 3. Start a fresh reviewer on a different model + +The reviewer must run on a **different model from yours**, so its blind spots +differ from yours. Start a **new** reviewer every round. Don't continue an old +reviewer conversation, because it is already anchored on its earlier opinions. +The ledger gives the new reviewer the history it needs. + +How to start a reviewer, depending on your harness: + +| Harness | How to start a different-model reviewer | +| --- | --- | +| Jetski / Antigravity | `invoke_subagent` with a `Model` other than your own (for example `pro` if you aren't on it), read-only research type. | +| Claude Code | Gemini CLI from the shell: `gemini -p "$(cat )"`. If it isn't installed, use a Task subagent pinned to a different Claude model. | +| Gemini CLI | Claude Code from the shell: `claude -p "$(cat )"`. If it isn't installed, use a subagent on a different Gemini model. | + +If you can't get a different model at all, stop and tell the user. Don't quietly +use your own model to review your own work. + +### Reviewer prompt template + +```text +You are an independent senior reviewer for a pull request to the google-cloud-java +monorepo, module java-bigquery-jdbc (the BigQuery JDBC driver). You did not write +this code. Be rigorous and specific, and don't rubber-stamp it. + +Repository: (you may read any file for context) +Base branch: origin/main + +== PR DESCRIPTION == + + +== CHANGED FILES == + + +== DIFF == + + +== REVIEW LEDGER FROM EARLIER ROUNDS (empty in round 1) == + + +Review for: correctness and edge cases, concurrency and resource leaks +(connections, statements, result sets, threads), JDBC spec compliance, backward +compatibility and public API surface (see AGENTS.md and the api-lifecycle skill), +security (credentials, logging of secrets), test coverage of every new branch, +test quality (see the bigquery-jdbc-testing skill), and whether the PR +description is accurate and complete. + +For each earlier finding marked PUSHBACK in the ledger, say whether you ACCEPT +the author's reason (the finding is then closed) or MAINTAIN it, with a new +argument. Don't repeat a finding that is marked FIXED unless the fix is wrong or +incomplete. + +Reply in exactly this format: + +VERDICT: APPROVE | CHANGES_REQUESTED +FINDINGS: +- id: R- + severity: blocking | major | minor | nit + location: : | PR description + issue: + suggestion: +PUSHBACK_RESPONSES: +- id: + decision: ACCEPT | MAINTAIN + reason: + +APPROVE only if no blocking or major findings are open. +``` + +## 4. Answer every finding: fix it or push back + +Answer **every** finding in the ledger with exactly one of: + +- **FIXED**: say what you changed and where. +- **PUSHBACK**: you disagree. You **must** give a concrete reason backed by + evidence: a code reference, the JDBC spec, an existing convention in the repo, + a test result, or scope ("out of scope for this PR; tracked in "). + "I prefer it this way" is not a reason. + +Rules: + +- Take the findings seriously. Push back only when you are confident the + reviewer is wrong or the request is out of scope. Don't push back to save + effort. +- `nit` findings may be answered `WONTFIX` with a short reason. +- If a reviewer maintains a pushback for a second round, either fix it or + escalate it to the user (see step 6). Don't argue a third round. +- You may also add your **own** open concerns to the ledger, things you are + still unsure about. The loop doesn't end while any of them are open. + +### Ledger format + +Keep one ledger file for the whole loop and append to it every round: + +```markdown +## Round , reviewer model: , verdict: + +| id | severity | finding | author response | status | +| --- | --- | --- | --- | --- | +| R1-1 | blocking | ResultSet leaked on error path in Foo.java:120 | FIXED: try-with-resources in Foo.java:118-131 | closed | +| R1-2 | major | Missing test for default universe | FIXED: added testXForDefaultUniverse | closed | +| R1-3 | minor | Rename helper to assertY | PUSHBACK: name matches existing assertX convention in BarTest | open, awaiting reviewer | + +Author open concerns: +``` + +## 5. Re-run checks and loop + +After you address the findings: + +1. Re-run the local checks from step 1. Every round, not just the first. +2. Update the PR description if the change or the testing changed. +3. Rebuild the context bundle (new diff, updated ledger). +4. Go back to step 3 with a **new** reviewer. + +## 6. When to stop + +The loop **ends successfully** only when all of these are true: + +- The latest reviewer's verdict is `APPROVE`. +- No `blocking` or `major` finding is open, and every pushback is either + ACCEPTED or resolved by a fix. +- You (the author) have no open concerns. +- The local checks pass on the final code. + +**Safety limit:** if you haven't reached agreement after **5 rounds**, or a +pushback is maintained twice, stop. Report the unresolved items to the user +with both sides' arguments and let them decide. Don't loop forever, and don't +declare agreement that didn't happen. + +When the loop ends, report to the user: the number of rounds, the reviewer +model(s) used, what changed because of review, any accepted pushbacks, and the +final PR description. Push or open the PR only if the user asked you to. diff --git a/java-bigquery-jdbc/.claude/skills b/java-bigquery-jdbc/.claude/skills new file mode 120000 index 000000000000..2b7a412b8fa0 --- /dev/null +++ b/java-bigquery-jdbc/.claude/skills @@ -0,0 +1 @@ +../.agents/skills \ No newline at end of file diff --git a/java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing b/java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing deleted file mode 120000 index dffed6f65a1d..000000000000 --- a/java-bigquery-jdbc/.claude/skills/bigquery-jdbc-testing +++ /dev/null @@ -1 +0,0 @@ -../../.agents/skills/bigquery-jdbc-testing \ No newline at end of file diff --git a/java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing b/java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing deleted file mode 120000 index dffed6f65a1d..000000000000 --- a/java-bigquery-jdbc/.gemini/skills/bigquery-jdbc-testing +++ /dev/null @@ -1 +0,0 @@ -../../.agents/skills/bigquery-jdbc-testing \ No newline at end of file diff --git a/java-bigquery-jdbc/.gitignore b/java-bigquery-jdbc/.gitignore index eb60678e7791..fda14508dd4d 100644 --- a/java-bigquery-jdbc/.gitignore +++ b/java-bigquery-jdbc/.gitignore @@ -4,6 +4,10 @@ target-it/** **/ITBigQueryJDBCLocalTest.java **/BigQueryStatementE2EBenchmark.java +# Agent files: ignore local/generated content, but track shared skills. +.agents/* +!.agents/skills/ + tools/**/*.class tools/**/drivers/** tools/**/logs/**