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.

iennae · 2025-04-11T00:07:04Z

@gemini review

gemini-code-assist

Code Review

The pull request introduces a utility class for ServiceAccount tests, refactors existing tests to use this utility, and removes unnecessary environment variable requirements. The changes seem well-structured and improve the maintainability of the tests. However, there are a few areas that could be improved, particularly around test organization and cleanup.

Summary of Findings

Test Organization: The original ServiceAccountTests used a stage-based approach (stage1, stage2, etc.) which is now replaced with more descriptive test names. This is a good improvement for readability and maintainability.
Environment Variable Requirements: The removal of the IAM_CREDENTIALS environment variable requirement from DenyIT and RoleIT tests simplifies the setup and reduces external dependencies, making the tests easier to run.
Utility Class for Setup and Teardown: The introduction of the Util class centralizes the setup and teardown logic for service account tests, promoting code reuse and consistency across tests.
Test Cleanup: The tests create and delete service accounts and keys. It's important to ensure that these resources are always cleaned up, even if a test fails. The cleanup logic is present, but it's crucial to verify that it's robust and handles all possible failure scenarios.

Merge Readiness

The pull request is a good step towards improving the test infrastructure. However, the testServiceAccount_deleteServiceAccount test in ServiceAccountTests.java has a potential issue where the assertion assertThat(got, !containsString(serviceAccountName).matches(got)) might not work as intended. It's recommended to address this before merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

gemini-code-assist · 2025-04-11T00:07:27Z

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

+    Util.test_listServiceAccounts(PROJECT_ID);
+    got = bout.toString();
+    assertThat(got, !containsString(serviceAccountName).matches(got));


The assertion assertThat(got, !containsString(serviceAccountName).matches(got)) might not work as intended. The matches method expects a regular expression, and got is not a regular expression. Consider using !got.contains(serviceAccountName) instead.

assertThat(got, !got.contains(serviceAccountName));

gemini-code-assist · 2025-04-11T00:07:27Z

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

+  public void testServiceAccount_createServiceAccount() throws IOException {
+    // Prepare
+    String serviceAccountName = generateServiceAccountName();
+
+    // Act
+    ServiceAccount serviceAccount =
+        CreateServiceAccount.createServiceAccount(PROJECT_ID, serviceAccountName);
    String got = bout.toString();
-    assertThat(got, containsString("Created service account: " + SERVICE_ACCOUNT));
+
+    // Assert
+    assertThat(got, containsString("Created service account: " + serviceAccountName));
    assertNotNull(serviceAccount);
-    assertThat(serviceAccount.getName(), containsString(SERVICE_ACCOUNT));
+    assertThat(serviceAccount.getName(), containsString(serviceAccountName));

+    // Cleanup
+    Util.tearDownTest_deleteServiceAccount(PROJECT_ID, serviceAccountName);


Consider adding a try-finally block to ensure that the service account is always deleted, even if the test fails. This will prevent resource leaks and ensure that subsequent tests can run without interference.

Also, it might be useful to add a small delay before attempting to delete the service account, as sometimes deletion can fail if the account is not fully created yet.

String serviceAccountName = generateServiceAccountName(); ServiceAccount serviceAccount = null; try { // Act serviceAccount = CreateServiceAccount.createServiceAccount(PROJECT_ID, serviceAccountName); String got = bout.toString(); // Assert assertThat(got, containsString("Created service account: " + serviceAccountName)); assertNotNull(serviceAccount); assertThat(serviceAccount.getName(), containsString(serviceAccountName)); } finally { // Cleanup if (serviceAccount != null) { Util.tearDownTest_deleteServiceAccount(PROJECT_ID, serviceAccountName); } }

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

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?).

telpirion · 2025-04-11T17:00:22Z

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

@@ -37,7 +37,6 @@
 public class DenyIT {

  private static final String PROJECT_ID = System.getenv("IAM_PROJECT_ID");
-  private static final String GOOGLE_APPLICATION_CREDENTIALS = System.getenv("IAM_CREDENTIALS");


Okay, I finally realized what this is for. The sample is explicitly pulling the service account ADC from the environment for ... some reason. Is the sample creating a new service account and assigning it to the env var?

telpirion · 2025-04-11T17:04:33Z

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

+  }
+
+  private static String generateServiceAccountName() {
+    return "service-account-" + UUID.randomUUID().toString().substring(0, 8);
  }

  @BeforeClass


I wonder if we can avoid the issues we're seeing by creating and destroying a new service account for each test case. Maybe we need to switch to @BeforeEach?

+1; @BeforeEach is intended for use when there is a common operation performed in all or almost all tests.

Another question is regarding a need to create a delete new SA for each test. Is it required or the same SA can be reused? If there is one test for "deleting SA", it can create a designated SA for the tests.

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.

minherz · 2025-04-12T17:07:27Z

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

+  }
+
+  private static String generateServiceAccountName() {
+    return "service-account-" + UUID.randomUUID().toString().substring(0, 8);
  }

  @BeforeClass


+1; @BeforeEach is intended for use when there is a common operation performed in all or almost all tests.

Another question is regarding a need to create a delete new SA for each test. Is it required or the same SA can be reused? If there is one test for "deleting SA", it can create a designated SA for the tests.

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

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

alarconesparza assigned telpirion Apr 10, 2025

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

View reviewed changes

use try/finally blocks to ensure clean up

daa7898

alarconesparza marked this pull request as draft April 11, 2025 16:56

use coreMatchers.not to invert Matcher logic

b41f1a0

telpirion reviewed Apr 11, 2025

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alarconesparza commented Apr 4, 2025 •

edited

Loading

gemini-code-assist bot left a comment

gemini-code-assist bot left a comment

gemini-code-assist bot Apr 4, 2025

gemini-code-assist bot Apr 4, 2025

gemini-code-assist bot Apr 4, 2025

gemini-code-assist bot Apr 4, 2025

iennae commented Apr 11, 2025 •

edited

Loading

gemini-code-assist bot left a comment

gemini-code-assist bot Apr 11, 2025

gemini-code-assist bot Apr 11, 2025

telpirion left a comment

telpirion Apr 11, 2025

telpirion Apr 11, 2025

minherz Apr 12, 2025

telpirion commented Apr 11, 2025

minherz left a comment

minherz Apr 12, 2025

telpirion commented Apr 16, 2025

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

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

Are you sure you want to change the base?

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

Conversation

alarconesparza commented Apr 4, 2025 • edited Loading

Description

Checklist

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

gemini-code-assist bot Apr 4, 2025

Choose a reason for hiding this comment

iennae commented Apr 11, 2025 • edited Loading

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

gemini-code-assist bot Apr 11, 2025

Choose a reason for hiding this comment

gemini-code-assist bot Apr 11, 2025

Choose a reason for hiding this comment

telpirion left a comment

Choose a reason for hiding this comment

telpirion Apr 11, 2025

Choose a reason for hiding this comment

telpirion Apr 11, 2025

Choose a reason for hiding this comment

minherz Apr 12, 2025

Choose a reason for hiding this comment

telpirion commented Apr 11, 2025

minherz left a comment

Choose a reason for hiding this comment

minherz Apr 12, 2025

Choose a reason for hiding this comment

telpirion commented Apr 16, 2025

alarconesparza commented Apr 4, 2025 •

edited

Loading

iennae commented Apr 11, 2025 •

edited

Loading