feat(iam): add utility class for ServiceAccount test #10058

alarconesparza · 2025-04-04T22:25:17Z

Description

Checklist

gemini-code-assist

Hello @alarconesparza, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request introduces a utility class named Util.java to assist with testing the IAM Service Account functionality. It includes methods for creating, disabling, and deleting service accounts and their keys, as well as methods for retrieving service account information. The utility class aims to simplify the setup and teardown processes for integration tests related to IAM service accounts.

Highlights

Utility Class Creation: A new utility class Util.java is created to encapsulate common operations for managing IAM Service Accounts during testing.
Service Account Management: The utility class provides methods for creating, disabling, and deleting service accounts, streamlining the testing process.
Service Account Key Management: The utility class includes methods for creating, disabling, deleting, listing, and retrieving service account keys.
Awaiting Creation: The utility class includes methods to await service account and service account key creation, to ensure tests don't start before the resources are available.

Changelog

Click here to see the changelog

iam/snippets/src/test/java/Util.java
- Created a new utility class Util.java.
- Added methods for creating service accounts (setUpTest_createServiceAccount).
- Added methods for disabling service accounts (setUpTest_disableSertviceAccount).
- Added methods for deleting service accounts (tearDownTest_deleteServiceAccount).
- Added methods for listing service accounts (test_listServiceAccounts).
- Added methods for getting service account details (test_getServiceAccount).
- Added methods for creating service account keys (setUpTest_createServiceAccountKey).
- Added methods for disabling service account keys (setUpTest_disableServiceAccountKey).
- Added methods for deleting service account keys (tearDownTest_deleteServiceAccountKey).
- Added methods for listing service account keys (test_listServiceAccountKeys).
- Added methods for getting service account key details (test_getServiceAccountKey).
- Added methods to await service account and service account key creation (awaitForServiceAccountCreation, awaitForServiceAccountKeyCreation).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

A key is made,
Accounts managed with new code,
Tests now run smooth.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

The pull request introduces a utility class for ServiceAccount testing, which is a helpful addition. The code appears to be well-structured and addresses the intended functionality. However, there are a few areas that could be improved for clarity and efficiency.

Summary of Findings

Naming Consistency: Inconsistent naming of methods (e.g., setUpTest_createServiceAccount vs. test_listServiceAccounts). Consider standardizing the naming convention for better readability.
Redundant String Formatting: Multiple instances of string formatting for email and account names. Consider creating a utility method to handle this to avoid redundancy and potential errors.
Exception Handling in Await Methods: The awaitForServiceAccountCreation and awaitForServiceAccountKeyCreation methods catch Exception broadly. It would be better to catch specific exceptions to handle them appropriately.

Merge Readiness

The pull request is almost ready for merging. Addressing the naming inconsistencies, redundant string formatting, and exception handling would improve the code quality and maintainability. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.

gemini-code-assist · 2025-04-04T22:25:41Z

iam/snippets/src/test/java/Util.java

+  private static void awaitForServiceAccountCreation(String projectId, String serviceAccountName)
+      throws InterruptedException {
+    boolean isAccountCreated = false;
+    long time = 1000;
+    long timeLimit = 60000;
+    while (!isAccountCreated) {
+      try {
+        test_getServiceAccount(projectId, serviceAccountName);
+        isAccountCreated = true;
+      } catch (Exception e) {
+        Thread.sleep(time);
+        time *= 2;
+        if (time > timeLimit) {
+          break;
+        }
+      }
+    }


Catching Exception is too broad. It's better to catch specific exceptions like IOException or InterruptedException and handle them appropriately. Also, consider logging the exception to provide more context when the await fails.

} catch (IOException | InterruptedException e) { Thread.sleep(time); time *= 2; if (time > timeLimit) { // Log the exception here break; } }

gemini-code-assist · 2025-04-04T22:25:41Z

iam/snippets/src/test/java/Util.java

+  private static void awaitForServiceAccountKeyCreation(
+      String projectId, String serviceAccountName, String serviceAccountKeyId)
+      throws InterruptedException {
+    boolean isAccountCreated = false;
+    long time = 1000;
+    long timeLimit = 60000;
+    while (!isAccountCreated) {
+      try {
+        test_getServiceAccountKey(projectId, serviceAccountName, serviceAccountKeyId);
+        isAccountCreated = true;
+      } catch (Exception e) {
+        if (time > timeLimit) {
+          break;
+        }
+        Thread.sleep(time);
+        time *= 2;
+      }
+    }


Similar to awaitForServiceAccountCreation, catching Exception is too broad here. Catch specific exceptions and log them for better error handling.

} catch (IOException | InterruptedException e) { if (time > timeLimit) { // Log the exception here break; } Thread.sleep(time); time *= 2; }

gemini-code-assist · 2025-04-04T22:25:42Z

iam/snippets/src/test/java/Util.java

+    return serviceAccount;
+  }
+
+  public static void setUpTest_disableSertviceAccount(String projectId, String serviceAccountName)


Typo in method name: setUpTest_disableSertviceAccount. Should be setUpTest_disableServiceAccount.

Suggested change

public static void setUpTest_disableSertviceAccount(String projectId, String serviceAccountName)

public static void setUpTest_disableServiceAccount(String projectId, String serviceAccountName)

throws IOException {

iam/snippets/src/test/java/Util.java

gemini-code-assist · 2025-04-04T22:25:42Z

iam/snippets/src/test/java/Util.java

+  public static String getServiceAccountKeyIdFromKey(ServiceAccountKey key) {
+    return key.getName().substring(key.getName().lastIndexOf("/") + 1).trim();
+  }


Consider adding a null check for the key parameter to avoid a potential NullPointerException if key is null.

Suggested change

public static String getServiceAccountKeyIdFromKey(ServiceAccountKey key) {

return key.getName().substring(key.getName().lastIndexOf("/") + 1).trim();

}

public static String getServiceAccountKeyIdFromKey(ServiceAccountKey key) {

return key != null ? key.getName().substring(key.getName().lastIndexOf("/") + 1).trim() : null;

}

Stop reusing resources from other tests. Use Util functions to set up and tear down resources.

telpirion

I think I understand what might be happening here. These samples, I suspect, intend to swap out the environment's ADC with a service account created by the sample. However, the client libraries can't find the ADC because they aren't saved to disk (perhaps?).

iam/snippets/src/test/java/DenyIT.java

iam/snippets/src/test/java/ServiceAccountTests.java

telpirion · 2025-04-11T17:07:50Z

Follow-up: the code here looks correct as written. I suspect that there's something more causing the build failures.

minherz

I did not finish reviewing all code changes in all tests. Given the tendency in changes that I see, please extrapolate the remarks to the rest of the changed files.

iam/snippets/src/test/java/ServiceAccountTests.java

Change assertions to test sample instead of api calls Use google assertThat Flush System.out

telpirion · 2025-04-16T17:36:19Z

@minherz the samples under test are about SA management. That's why these tests create and delete SAs outside of our testing infra. Does that answer some of your concerns?

GoogleCloudPlatform#10082

alarconesparza · 2025-05-02T16:17:50Z

@gemini review

gemini-code-assist · 2025-05-02T16:18:00Z

I have reviewed the code again after the latest changes. I focused on correctness, efficiency, maintainability, and security, and I have not found any issues. The code looks good to me.

I also reviewed the changes with respect to the Sample Format Guide and have not found any issues.

Thanks for your contribution!

telpirion

@alarconesparza I'm sorry that it's taken so long for me to get back to this! Thank you SO much for all your hard work. This is truly excellent.

No longer needed.

minherz

PR does not look like adding an utility class but a full rewriting of tests.
Distribution tests across tens of test files should be explained.
Currently it seems to produce a lot of code duplication and to complicate the maintenance.

Please, avoid using stdout for test assertions. Output is usually ambiguous and hard to validate that leads to false positives due to small changes in source code.

minherz · 2025-05-15T19:09:30Z

iam/snippets/src/test/java/CreateServiceAccountIT.java

+  public void beforeTest() {
+    bout = new ByteArrayOutputStream();
+    System.setOut(new PrintStream(bout));
+


it is recommended to avoid capturing stdout for test validation due to high risk of ambiguity that invalidates test assertions

minherz · 2025-05-15T19:09:49Z

iam/snippets/src/test/java/CreateServiceAccountKeyIT.java

+    bout = new ByteArrayOutputStream();
+    System.setOut(new PrintStream(bout));


minherz · 2025-05-15T19:11:08Z

iam/snippets/src/test/java/DeleteServiceAccountIT.java

+    bout = new ByteArrayOutputStream();
+    System.setOut(new PrintStream(bout));


minherz · 2025-05-15T19:11:30Z

iam/snippets/src/test/java/DeleteServiceAccountKeyIT.java

+    bout = new ByteArrayOutputStream();
+    System.setOut(new PrintStream(bout));


minherz · 2025-05-15T19:12:19Z

iam/snippets/src/test/java/DisableServiceAccountIT.java

+    bout = new ByteArrayOutputStream();
+    System.setOut(new PrintStream(bout));


minherz · 2025-05-15T19:17:21Z

iam/snippets/src/test/java/Util.java

util class looks to have a replica of the code in the SA code samples. What is a value of having this file?

feat(iam): add utility class for ServiceAccount test

3f45bf5

product-auto-label bot added api: iam Issues related to the Identity and Access Management API. samples Issues that are directly related to samples. labels Apr 4, 2025

gemini-code-assist bot reviewed Apr 4, 2025

View reviewed changes

gemini-code-assist bot suggested changes Apr 4, 2025

View reviewed changes

alarconesparza added 2 commits April 4, 2025 22:29

fix typo

0001e63

feat(iam): refactor ServiceAccountTests.java

484951a

Stop reusing resources from other tests. Use Util functions to set up and tear down resources.

alarconesparza added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 7, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 7, 2025

alarconesparza added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 7, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 7, 2025

alarconesparza added 2 commits April 8, 2025 00:04

make await functions match

243a71c

add await for key disabling

fe3a1f6

alarconesparza added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 8, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 8, 2025

use UUID instead of time for naming

dd239a3

alarconesparza added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 9, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 9, 2025

alarconesparza added 2 commits April 10, 2025 18:35

remove unused env var

1ece256

use Util functions in QuickstartTests

bfc6a6d

alarconesparza marked this pull request as ready for review April 10, 2025 22:18

alarconesparza requested review from yoshi-approver and a team as code owners April 10, 2025 22:18

blunderbuss-gcf bot assigned minherz Apr 10, 2025

telpirion reviewed Apr 11, 2025

View reviewed changes

iam/snippets/src/test/java/DenyIT.java Show resolved Hide resolved

iam/snippets/src/test/java/ServiceAccountTests.java Outdated Show resolved Hide resolved

minherz requested changes Apr 12, 2025

View reviewed changes

alarconesparza added 5 commits April 15, 2025 19:15

split tests by sample so they are in their own files

f9e4143

rename test functions after splitting them to avoid redundancy

edeb2ed

unify tests format and practices

792f372

Change assertions to test sample instead of api calls Use google assertThat Flush System.out

fix testRenameServiceAccount

b3870c2

fix testListServiceAccounts

48e10d9

alarconesparza added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

add waiting time for assertions after enable/disable operations

1a3d583

alarconesparza added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

alarconesparza added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

alarconesparza added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2025

alarconesparza added 3 commits April 21, 2025 19:30

Undo changes to quickstartTest

90137d8

fix lint issue quickstartTest

fd86f71

Ignore test until issue is resolved

243b94b

GoogleCloudPlatform#10082

alarconesparza marked this pull request as ready for review April 28, 2025 23:22

alarconesparza requested review from minherz and telpirion April 28, 2025 23:26

telpirion approved these changes May 15, 2025

View reviewed changes

minherz reviewed May 15, 2025

View reviewed changes

iennae merged commit add7429 into GoogleCloudPlatform:main May 15, 2025
7 checks passed

	public static void setUpTest_disableSertviceAccount(String projectId, String serviceAccountName)
	public static void setUpTest_disableServiceAccount(String projectId, String serviceAccountName)
	throws IOException {

		bout = new ByteArrayOutputStream();
		System.setOut(new PrintStream(bout));

feat(iam): add utility class for ServiceAccount test #10058

feat(iam): add utility class for ServiceAccount test #10058

Uh oh!

Conversation

alarconesparza commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

telpirion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

telpirion commented Apr 11, 2025

Uh oh!

minherz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

telpirion commented Apr 16, 2025

Uh oh!

alarconesparza commented May 2, 2025

Uh oh!

gemini-code-assist bot commented May 2, 2025

Uh oh!

telpirion left a comment

Choose a reason for hiding this comment

Uh oh!

minherz left a comment

Choose a reason for hiding this comment

Uh oh!

minherz May 15, 2025

Choose a reason for hiding this comment

Uh oh!

minherz May 15, 2025

Choose a reason for hiding this comment

Uh oh!

minherz May 15, 2025

Choose a reason for hiding this comment

Uh oh!

minherz May 15, 2025

Choose a reason for hiding this comment

Uh oh!

minherz May 15, 2025

alarconesparza commented Apr 4, 2025 •

edited

Loading