Skip to content

test(integration): integrate shared chunk cache tests#4895

Open
PranjalC100 wants to merge 1 commit into
masterfrom
shared-chunk-cache-migrate
Open

test(integration): integrate shared chunk cache tests#4895
PranjalC100 wants to merge 1 commit into
masterfrom
shared-chunk-cache-migrate

Conversation

@PranjalC100

Copy link
Copy Markdown
Member

Description

This PR integrates the shared chunk cache tests into the integration test suite and updates the test configurations. Specifically, it includes the following changes:

  • Adds "shared_chunk_cache" to TEST_PACKAGES_COMMON in tools/integration_tests/improved_run_e2e_tests.sh.
  • Refactors bucket references in the shared_chunk_cache tests to use testEnv.cfg.TestBucket instead of setup.TestBucket().
  • Updates the flag parsing logic in shared_chunk_cache_test.go to use comma-separated values (strings.Split) instead of spaces (strings.Fields).
  • Adds the shared_chunk_cache test block to tools/integration_tests/test_config.yaml to run TestSharedChunkCacheTestSuite with the relevant experimental shared chunk cache flags on GKE. Note that this configuration was removed in an earlier PR (#4836), assuming it to be present in the test_config.yaml.

Link to the issue in case of a bug fix.

b/535670081

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - This PR specifically updates and configures the integration test suite for the shared chunk cache.

Any backward incompatible change? If so, please explain.

No.

@PranjalC100
PranjalC100 requested a review from raj-prince July 16, 2026 20:17
@PranjalC100 PranjalC100 added the execute-integration-tests Run only integration tests label Jul 16, 2026
@PranjalC100
PranjalC100 requested review from a team and meet2mky as code owners July 16, 2026 20:18
@PranjalC100 PranjalC100 added the execute-integration-tests-on-zb To run E2E tests on zonal bucket. label Jul 16, 2026
@github-actions github-actions Bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Jul 16, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request integrates the shared_chunk_cache package into the end-to-end integration tests, updates the test configuration, and switches the cleanup logic to use the configured test bucket (testEnv.cfg.TestBucket) instead of the default helper. Additionally, it updates the flag parsing logic to split on commas instead of whitespace. Feedback was provided to make this flag parsing more robust by trimming whitespace and filtering out empty strings to prevent potential test failures from malformed flag strings.

Comment on lines +353 to 357
primaryFlags := strings.Split(flagStr, ",")
var secondaryFlags []string
if len(cfg.SecondaryFlags) > i {
secondaryFlags = strings.Fields(cfg.SecondaryFlags[i])
secondaryFlags = strings.Split(cfg.SecondaryFlags[i], ",")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using strings.Split directly can lead to empty or whitespace-only strings in the flags slice if the config contains consecutive, leading, or trailing commas, or spaces around commas (e.g., "--flag1, ,--flag2"). This can cause unexpected test failures. A more robust approach is to trim whitespace from each part and filter out any empty strings.

                                var primaryFlags []string
				for _, f := range strings.Split(flagStr, ",") {
					if flag := strings.TrimSpace(f); flag != "" {
						primaryFlags = append(primaryFlags, flag)
					}
				}
				var secondaryFlags []string
				if len(cfg.SecondaryFlags) > i {
					secondaryFlags = []string{}
					for _, f := range strings.Split(cfg.SecondaryFlags[i], ",") {
						if flag := strings.TrimSpace(f); flag != "" {
							secondaryFlags = append(secondaryFlags, flag)
						}
					}
				}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

execute-integration-tests Run only integration tests execute-integration-tests-on-zb To run E2E tests on zonal bucket. remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant