test(managed_folder): optimize managed_folders e2e-test by skipping unnecessary IAM propagation waits#4795
test(managed_folder): optimize managed_folders e2e-test by skipping unnecessary IAM propagation waits#4795raj-prince wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! 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! This pull request optimizes the execution time of managed folder integration tests by refining how permissions are managed during the test lifecycle. By moving permission setup and teardown logic directly into the test loop, the suite avoids redundant operations and improves the efficiency of the testing process. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the setup and teardown of managed folder permissions in the integration tests by moving the permission provisioning and revocation logic from the struct's Setup and Teardown methods into the main test loop. The review feedback suggests wrapping this logic in an anonymous function and using defer to guarantee that permissions are properly revoked even if the tests fail or panic.
b6cb8af to
f9caa79
Compare
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes the integration tests for managed folders by avoiding unnecessary recreation of managed folders and IAM policy propagation delays (60-second sleeps) between individual test cases. It achieves this by moving the test directory setup to SetupSuite, checking if managed folders were actually recreated before applying permissions/sleeping, and adding local cleanup via defer in individual test cases.
Feedback on these changes suggests two improvements: first, since SetupTest no longer cleans up the entire test directory, files created during tests may persist locally and cause caching issues or flakiness, so it is recommended to clean up the contents of the managed folders locally in TearDownTest. Second, the IAM policy configuration in TestManagedFolders_FolderAdminPermission is fragile as it assumes managed folders already exist; pre-creating them before setting their IAM policies would make the test suite more robust and independent of scenario ordering.
| if ts.managedFoldersPermission != "nil" { | ||
| providePermissionToManagedFolder(testEnv.bucket, path.Join(testEnv.testDir, ManagedFolder1), testEnv.serviceAccount, ts.managedFoldersPermission, t) | ||
| providePermissionToManagedFolder(testEnv.bucket, path.Join(testEnv.testDir, ManagedFolder2), testEnv.serviceAccount, ts.managedFoldersPermission, t) | ||
| // Wait once per permission scenario for initial policy propagation. | ||
| time.Sleep(60 * time.Second) | ||
| } |
There was a problem hiding this comment.
The IAM policy configuration here is fragile because it assumes the managed folders already exist in the bucket. If the first scenario ("nil") is skipped, or if the order of scenarios is changed, providePermissionToManagedFolder will fail because the managed folders have not been created yet. Pre-creating the managed folders before setting their IAM policies makes the test suite robust and independent of scenario ordering.
| if ts.managedFoldersPermission != "nil" { | |
| providePermissionToManagedFolder(testEnv.bucket, path.Join(testEnv.testDir, ManagedFolder1), testEnv.serviceAccount, ts.managedFoldersPermission, t) | |
| providePermissionToManagedFolder(testEnv.bucket, path.Join(testEnv.testDir, ManagedFolder2), testEnv.serviceAccount, ts.managedFoldersPermission, t) | |
| // Wait once per permission scenario for initial policy propagation. | |
| time.Sleep(60 * time.Second) | |
| } | |
| if ts.managedFoldersPermission != "nil" { | |
| _ = client.CreateManagedFoldersInBucket(testEnv.ctx, testEnv.controlClient, path.Join(testEnv.testDir, ManagedFolder1), testEnv.bucket) | |
| _ = client.CreateManagedFoldersInBucket(testEnv.ctx, testEnv.controlClient, path.Join(testEnv.testDir, ManagedFolder2), testEnv.bucket) | |
| providePermissionToManagedFolder(testEnv.bucket, path.Join(testEnv.testDir, ManagedFolder1), testEnv.serviceAccount, ts.managedFoldersPermission, t) | |
| providePermissionToManagedFolder(testEnv.bucket, path.Join(testEnv.testDir, ManagedFolder2), testEnv.serviceAccount, ts.managedFoldersPermission, t) | |
| // Wait once per permission scenario for initial policy propagation. | |
| time.Sleep(60 * time.Second) | |
| } |
There was a problem hiding this comment.
SetupTest creates the folder if not already exist, applies the iam permission and wait for propagation.
Description
The managed_folders admin e2e suite was paying a 60-second IAM propagation sleep before every test case in a permission scenario. That made the package significantly slower, even when managed folders were not recreated and existing IAM remained valid.
Before the fix:

After the fix:

Since, managed-folders was the test package with highest-execution time, hence we see 15mins reduction in overall e2e test execution.
Changes wise::
Link to the issue in case of a bug fix.
b/526790288
Testing details
Any backward incompatible change? If so, please explain.