Skip to content

test(storage): Parameterized test setup for RCU integration testing #14530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nidhiii-27 wants to merge 5 commits into from
+956 −99
Open

test(storage): Parameterized test setup for RCU integration testing #14530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba14c63
test(storage): Parameterized test setup for RCU Integration Testing
nidhiii-27 Sep 28, 2026
f67b25c
fix(storage): resolve PR feedback for raw URI path and Zonal Rapid lo…
nidhiii-27 Sep 29, 2026
79acb1d
fix(storage): address PR review feedback on URI path and CrossRun Jav…
nidhiii-27 Nov 3, 2026
ffb85c3
test(storage): resolve PR review comments for RCU parameterized setup
nidhiii-27 Nov 7, 2026
695027e
test(storage): add baseline equivalence test for HttpStorageRpc non-p…
nidhiii-27 Nov 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions ...ge/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@
import java.io.OutputStream;
import java.math.BigInteger;
import java.net.FileNameMap;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLConnection;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -158,11 +160,30 @@ public HttpStorageRpc(StorageOptions options, JsonFactory jsonFactory) {
initializer = censusHttpModule.getHttpRequestInitializer(initializer);
initializer = new InvocationIdInitializer(initializer, applicationName, tm);
batchRequestInitializer = censusHttpModule.getHttpRequestInitializer(null);
storage =
String host = options.getHost();
Storage.Builder storageBuilder =
new Storage.Builder(transport, jsonFactory, initializer)
.setRootUrl(options.getHost())
.setApplicationName(applicationName)
.build();
.setApplicationName(applicationName);
try {
URI uri = new URI(host);
String path = uri.getRawPath();
String normalizedPath =
path != null && path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
String preprodServicePath = "/storage/v1_preprod";
if (normalizedPath != null && normalizedPath.endsWith(preprodServicePath)) {
int servicePathIdx = normalizedPath.length() - preprodServicePath.length();
String rootPath = normalizedPath.substring(0, servicePathIdx + 1);
String rootUrl =
new URI(uri.getScheme(), uri.getAuthority(), rootPath, null, null).toString();
String servicePath = path.substring(servicePathIdx + 1);
storageBuilder.setRootUrl(rootUrl).setServicePath(servicePath);
} else {

@Dhriti07 Dhriti07 Oct 29, 2026

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.

This change is in production code, so I think it might change behavior for existing users who set a host with a path, e.g. https://proxy.example.com/gcs/. Today the whole host is the root URL, so requests go to https://proxy.example.com/gcs/storage/v1/b/.... With this change the path replaces storage/v1/, so the same config sends requests to https://proxy.example.com/gcs/b/....

@nidhiii-27 nidhiii-27 Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated the host parsing logic in HttpStorageRpc so that servicePath is only overridden when the URI path explicitly contains a /storage/v1... segment (such as /storage/v1_preprod/), preserving the existing rootUrl behavior for reverse-proxy paths like https://proxy.example.com/gcs/.

Co-authored by AI Agent

@Dhriti07 Dhriti07 Nov 5, 2026

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.

The check is indexOf("/storage/v1"), a substring match, so hosts like https://proxy/storage/v1-mirror/ or /storage/v1beta/ still change from storage/v1/b/... to b/...

@nidhiii-27 nidhiii-27 Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restricted the service path split in HttpStorageRpc to match only /storage/v1_preprod (or /storage/v1_preprod/), and added unit tests in HttpStorageRpcUriTest covering /storage/v1-mirror/ and /storage/v1beta/.

Co-authored by AI Agent

@Dhriti07 Dhriti07 Nov 9, 2026

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.

Looks safe to me but since HttpStorageRpc ships to customers, but under the test: commit type release-please leaves it out of the changelog, to be doubly sure can we validate that this has no impact on users.

@Dhriti07 Dhriti07 Nov 9, 2026

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.

The PR description still says "custom /storage/v1* service paths". It should say only /storage/v1_preprod can you update it accordingly

@nidhiii-27 nidhiii-27 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Validated that this change has zero impact on users:

  1. Code path: ServiceOptions and HttpStorageOptions.Builder.build() always resolve options.getHost() to a non-null string (defaulting to "https://storage.googleapis.com/" or "https://storage.<universeDomain>/"), and HttpStorageRpc only splits rootUrl/servicePath when normalizedPath.endsWith("/storage/v1_preprod"). Every other host (default prod, custom universe domain, local TestBench/emulator, IPv6, reverse-proxy paths, or even malformed URI strings) executes storageBuilder.setRootUrl(host), which is identical to main.
  2. Baseline equivalence test: Added nonPreprodHosts_matchMainBaselineExactly in HttpStorageRpcUriTest comparing HttpStorageRpc's Storage client against main's new Storage.Builder(...).setRootUrl(options.getHost()).build() (getRootUrl(), getServicePath(), getBaseUrl(), and buildHttpRequestUrl()) across all user-facing StorageOptions configurations.

Co-authored by AI Agent

@nidhiii-27 nidhiii-27 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Updated the PR description to specify /storage/v1_preprod only.

Co-authored by AI Agent

storageBuilder.setRootUrl(host);
}
} catch (URISyntaxException e) {
storageBuilder.setRootUrl(host);
}
storage = storageBuilder.build();
}

public Storage getStorage() {
Expand Down
41 changes: 22 additions & 19 deletions ...oud-storage/src/test/java/com/google/cloud/storage/it/runner/CrossRunFrameworkMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.cloud.storage.it.runner.annotations.CrossRun;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import java.lang.annotation.Annotation;
import java.util.Optional;
import org.junit.runners.model.FrameworkMethod;

Expand All @@ -46,32 +45,36 @@ boolean isExcluded(CrossRunIntersection crossRunIntersection) {
}

private ImmutableSet<CrossRunIntersection> excludedAndNotIgnored() {
ImmutableSet<CrossRunIntersection> excludes =
findMethodOrClassAnnotation(fm, CrossRun.Exclude.class)
.map(CrossRunIntersection::expand)
.orElse(ImmutableSet.of());
ImmutableSet<CrossRunIntersection> excludes = excludes();
ImmutableSet<CrossRunIntersection> ignores = ignores();
return ImmutableSet.copyOf(Sets.difference(excludes, ignores));
}

private ImmutableSet<CrossRunIntersection> excludes() {
ImmutableSet<CrossRunIntersection> methodExcludes =
Optional.ofNullable(fm.getAnnotation(CrossRun.Exclude.class))
.map(CrossRunIntersection::expand)
.orElse(ImmutableSet.of());
ImmutableSet<CrossRunIntersection> classExcludes =
Optional.ofNullable(fm.getDeclaringClass().getAnnotation(CrossRun.Exclude.class))
.map(CrossRunIntersection::expand)
.orElse(ImmutableSet.of());
return ImmutableSet.copyOf(Sets.union(methodExcludes, classExcludes));
}

private ImmutableSet<CrossRunIntersection> ignores() {
return findMethodOrClassAnnotation(fm, CrossRun.Ignore.class)
.map(CrossRunIntersection::expand)
.orElse(ImmutableSet.of());
ImmutableSet<CrossRunIntersection> methodIgnores =
Optional.ofNullable(fm.getAnnotation(CrossRun.Ignore.class))
.map(CrossRunIntersection::expand)
.orElse(ImmutableSet.of());
ImmutableSet<CrossRunIntersection> classIgnores =
Optional.ofNullable(fm.getDeclaringClass().getAnnotation(CrossRun.Ignore.class))
.map(CrossRunIntersection::expand)
.orElse(ImmutableSet.of());
return ImmutableSet.copyOf(Sets.union(methodIgnores, classIgnores));
}

static CrossRunFrameworkMethod of(FrameworkMethod fm) {
return new CrossRunFrameworkMethod(fm);
}

private static <A extends Annotation> Optional<A> findMethodOrClassAnnotation(
FrameworkMethod child, Class<A> annotation) {
A methodA = child.getAnnotation(annotation);
A classA = child.getDeclaringClass().getAnnotation(annotation);
if (methodA != null) {
return Optional.of(methodA);
} else {
return Optional.ofNullable(classA);
}
}
}
105 changes: 80 additions & 25 deletions ...-cloud-storage/src/test/java/com/google/cloud/storage/it/runner/CrossRunIntersection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@
import com.google.cloud.storage.TransportCompatibility.Transport;
import com.google.cloud.storage.it.runner.annotations.Backend;
import com.google.cloud.storage.it.runner.annotations.CrossRun;
import com.google.cloud.storage.it.runner.annotations.LocationType;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
import java.util.Collections;
import java.util.Locale;
import java.util.Objects;
import java.util.Set;
import javax.annotation.concurrent.Immutable;
import javax.annotation.concurrent.ThreadSafe;
import org.checkerframework.checker.nullness.qual.Nullable;
Expand All @@ -39,10 +42,15 @@ public final class CrossRunIntersection {

private final @Nullable Backend backend;
private final @Nullable Transport transport;
private final @Nullable LocationType locationType;

private CrossRunIntersection(@Nullable Backend backend, @Nullable Transport transport) {
private CrossRunIntersection(
@Nullable Backend backend,
@Nullable Transport transport,
@Nullable LocationType locationType) {
this.backend = backend;
this.transport = transport;
this.locationType = locationType;
}

@Nullable
Expand All @@ -55,19 +63,32 @@ public Transport getTransport() {
return transport;
}

@Nullable
public LocationType getLocationType() {
return locationType;
}

public CrossRunIntersection clearBackend() {
if (backend == null) {
return this;
} else {
return new CrossRunIntersection(null, transport);
return new CrossRunIntersection(null, transport, locationType);
}
}

public CrossRunIntersection clearTransport() {
if (transport == null) {
return this;
} else {
return new CrossRunIntersection(backend, null);
return new CrossRunIntersection(backend, null, locationType);
}
}

public CrossRunIntersection clearLocationType() {
if (locationType == null) {
return this;
} else {
return new CrossRunIntersection(backend, transport, null);
}
}

Expand All @@ -76,7 +97,7 @@ public CrossRunIntersection withBackend(Backend backend) {
if (this.backend == backend) {
return this;
} else {
return new CrossRunIntersection(backend, transport);
return new CrossRunIntersection(backend, transport, locationType);
}
}

Expand All @@ -85,7 +106,16 @@ public CrossRunIntersection withTransport(Transport transport) {
if (this.transport == transport) {
return this;
} else {
return new CrossRunIntersection(backend, transport);
return new CrossRunIntersection(backend, transport, locationType);
}
}

public CrossRunIntersection withLocationType(LocationType locationType) {
requireNonNull(locationType, "locationType must be non null");
if (this.locationType == locationType) {
return this;
} else {
return new CrossRunIntersection(backend, transport, locationType);
}
}

Expand All @@ -107,6 +137,13 @@ public boolean anyMatch(CrossRunIntersection other) {
l = l.clearTransport();
}

if (l.locationType == null) {
r = r.clearLocationType();
}
if (r.locationType == null) {
l = l.clearLocationType();
}

return l.equals(r);
}

Expand All @@ -119,6 +156,9 @@ public boolean anyMatch(CrossRunIntersection other) {
public String fmtSuiteName() {
String t = transport != null ? transport.toString() : "NULL_TRANSPORT";
String b = backend != null ? backend.toString() : "NULL_BACKEND";
if (locationType != null) {
return String.format(Locale.US, "[%s][%s][%s]", t, b, locationType);
}
return String.format(Locale.US, "[%s][%s]", t, b);
}

Expand All @@ -131,54 +171,69 @@ public boolean equals(Object o) {
return false;
}
CrossRunIntersection crossRunIntersection = (CrossRunIntersection) o;
return backend == crossRunIntersection.backend && transport == crossRunIntersection.transport;
return backend == crossRunIntersection.backend
&& transport == crossRunIntersection.transport
&& locationType == crossRunIntersection.locationType;
}

@Override
public int hashCode() {
return Objects.hash(backend, transport);
return Objects.hash(backend, transport, locationType);
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("backend", backend)
.add("transport", transport)
.add("locationType", locationType)
.toString();
}

public static CrossRunIntersection of(@Nullable Backend t, @Nullable Transport s) {
return new CrossRunIntersection(t, s);
public static CrossRunIntersection of(@Nullable Backend b, @Nullable Transport t) {
return new CrossRunIntersection(b, t, null);
}

public static CrossRunIntersection of(
@Nullable Backend b, @Nullable Transport t, @Nullable LocationType lt) {
return new CrossRunIntersection(b, t, lt);
}

public static ImmutableSet<CrossRunIntersection> expand(CrossRun.Ignore i) {
ImmutableSet<Backend> backends = ImmutableSet.copyOf(i.backends());
ImmutableSet<Transport> transports = ImmutableSet.copyOf(i.transports());
return expand(backends, transports);
ImmutableSet<LocationType> locations = ImmutableSet.copyOf(i.locations());
return expand(backends, transports, locations);
}

public static ImmutableSet<CrossRunIntersection> expand(CrossRun.Exclude i) {
ImmutableSet<Backend> backends = ImmutableSet.copyOf(i.backends());
ImmutableSet<Transport> transports = ImmutableSet.copyOf(i.transports());
return expand(backends, transports);
ImmutableSet<LocationType> locations = ImmutableSet.copyOf(i.locations());
return expand(backends, transports, locations);
}

public static ImmutableSet<CrossRunIntersection> expand(
ImmutableSet<Backend> backends, ImmutableSet<@Nullable Transport> transports) {
if (backends.isEmpty() && transports.isEmpty()) {
ImmutableSet<Backend> backends,
ImmutableSet<@Nullable Transport> transports,
ImmutableSet<@Nullable LocationType> locations) {
if (backends.isEmpty() && transports.isEmpty() && locations.isEmpty()) {
return ImmutableSet.of();
} else if (!backends.isEmpty() && !transports.isEmpty()) {
return backends.stream()
.flatMap(t -> transports.stream().map(s -> new CrossRunIntersection(t, s)))
.collect(ImmutableSet.toImmutableSet());
} else if (!backends.isEmpty()) {
return backends.stream()
.map(t -> new CrossRunIntersection(t, null))
.collect(ImmutableSet.toImmutableSet());
} else {
return transports.stream()
.map(s -> new CrossRunIntersection(null, s))
.collect(ImmutableSet.toImmutableSet());
}

Set<@Nullable Backend> bSet =
backends.isEmpty() ? Collections.singleton((Backend) null) : backends;
Set<@Nullable Transport> tSet =
transports.isEmpty() ? Collections.singleton((Transport) null) : transports;
Set<@Nullable LocationType> lSet =
locations.isEmpty() ? Collections.singleton((LocationType) null) : locations;

return bSet.stream()
.flatMap(
b ->
tSet.stream()
.flatMap(t -> lSet.stream().map(l -> new CrossRunIntersection(b, t, l))))
.filter(i -> !(i.backend == null && i.transport == null && i.locationType == null))
.collect(ImmutableSet.toImmutableSet());
}
}
50 changes: 32 additions & 18 deletions ...oogle-cloud-storage/src/test/java/com/google/cloud/storage/it/runner/StorageITRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.cloud.storage.it.runner.annotations.Backend;
import com.google.cloud.storage.it.runner.annotations.CrossRun;
import com.google.cloud.storage.it.runner.annotations.CrossRun.AllowClassRule;
import com.google.cloud.storage.it.runner.annotations.LocationType;
import com.google.cloud.storage.it.runner.annotations.ParallelFriendly;
import com.google.cloud.storage.it.runner.annotations.Parameterized;
import com.google.cloud.storage.it.runner.annotations.Parameterized.Parameter;
Expand Down Expand Up @@ -166,7 +167,10 @@ private static List<Runner> computeRunners(Class<?> klass, Registry registry)
.flatMap(
b ->
ImmutableSet.copyOf(crossRun.transports()).stream()
.map(t -> CrossRunIntersection.of(b, t)))
.flatMap(
t ->
ImmutableSet.copyOf(crossRun.locations()).stream()
.map(l -> CrossRunIntersection.of(b, t, l))))
.flatMap(
c -> {
TestInitializer ti = registry.newTestInitializerForCell(c);
Expand All @@ -187,23 +191,33 @@ private static List<Runner> computeRunners(Class<?> klass, Registry registry)
.collect(ImmutableList.toImmutableList()));
} else {
Backend backend = singleBackend.value();
CrossRunIntersection crossRunIntersection = CrossRunIntersection.of(backend, null);
TestInitializer ti = registry.newTestInitializerForCell(crossRunIntersection);
if (parameters != null) {
return SneakyException.unwrap(
() ->
parameters.stream()
.map(
param ->
StorageITLeafRunner.unsafeOf(
testClass,
crossRunIntersection,
fmtParam(param),
ti.andThen(setFieldTo(testClass, param))))
.collect(ImmutableList.toImmutableList()));
} else {
return ImmutableList.of(StorageITLeafRunner.of(testClass, crossRunIntersection, null, ti));
}
boolean isDefault =
singleBackend.locations().length == 1
&& singleBackend.locations()[0] == LocationType.REGIONAL_STANDARD;

return SneakyException.unwrap(
() ->
ImmutableSet.copyOf(singleBackend.locations()).stream()
.map(l -> CrossRunIntersection.of(backend, null, l))
.flatMap(
c -> {
TestInitializer ti = registry.newTestInitializerForCell(c);
if (parameters != null) {
return parameters.stream()
.map(
param ->
StorageITLeafRunner.unsafeOf(
testClass,
c,
isDefault ? fmtParam(param) : fmtParam(c, param),
ti.andThen(setFieldTo(testClass, param))));
} else {
return Stream.of(
StorageITLeafRunner.unsafeOf(
testClass, c, isDefault ? null : c.fmtSuiteName(), ti));
}
})
.collect(ImmutableList.toImmutableList()));
}
}

Expand Down
Loading
Loading