Skip to content

fix(storage): Avoid integration test segfaults.#12419

Merged
BrennaEpp merged 4 commits intogoogleapis:mainfrom
cjc25:avoid-jwt-segfault
Jun 9, 2025
Merged

fix(storage): Avoid integration test segfaults.#12419
BrennaEpp merged 4 commits intogoogleapis:mainfrom
cjc25:avoid-jwt-segfault

Conversation

@cjc25
Copy link
Contributor

@cjc25 cjc25 commented Jun 6, 2025

If the JWT private key file is not specified in an environment variable, then skip the tests which would otherwise require it. This matches what other tests do. Without this change, jwt.Email results in a nil dereference.

If the JWT private key file is not specified in an environment variable,
then skip the tests which would otherwise require it. This matches what
other tests do. Without this change, `jwt.Email` results in a nil
dereference.
@cjc25 cjc25 requested review from a team June 6, 2025 13:35
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jun 6, 2025
@BrennaEpp
Copy link
Contributor

GCLOUD_TESTS_GOLANG_KEY is technically required to run the integration tests (see https://github.com/googleapis/google-cloud-go/blob/main/CONTRIBUTING.md#local-setup), so I think we should fail here instead of skipping.

I think we are inconsistent with the creds we use in the tests - a while back I started documenting this but never quite finished and we've expanded since then. If most other tests work without the env var set, maybe we can instead extract the info we need from the creds set on the client?

@cjc25
Copy link
Contributor Author

cjc25 commented Jun 7, 2025

Thanks for the integration test instruction pointer! I don't have a project set up like that but I can take a look. I made these all (and the other places where we currently skip when jwt == nil) into test failures.

Copy link
Contributor

@BrennaEpp BrennaEpp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

We can change it later to extract creds but we def don't want these to "silently" fail

@BrennaEpp BrennaEpp enabled auto-merge (squash) June 9, 2025 21:02
@BrennaEpp BrennaEpp merged commit a9dec07 into googleapis:main Jun 9, 2025
8 checks passed
@cjc25 cjc25 deleted the avoid-jwt-segfault branch June 17, 2025 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants