Skip to content

refactor(storage): replace manual retries in tests#6510

Merged
BrennaEpp merged 7 commits into
googleapis:mainfrom
BrennaEpp:test-cleanup
Aug 28, 2022
Merged

refactor(storage): replace manual retries in tests#6510
BrennaEpp merged 7 commits into
googleapis:mainfrom
BrennaEpp:test-cleanup

Conversation

@BrennaEpp

@BrennaEpp BrennaEpp commented Aug 12, 2022

Copy link
Copy Markdown
Contributor

Fixes #5032

Also adds a retry to the HMAC key test that fixes #6544

@BrennaEpp BrennaEpp requested review from a team August 12, 2022 18:55
@product-auto-label product-auto-label Bot added size: l Pull request size is large. api: storage Issues related to the Cloud Storage API. labels Aug 12, 2022

@tritone tritone 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.

Generally looks good, a few minor comments

Comment thread storage/integration_test.go Outdated
t.Errorf("ACL.List: object ACL list failed: %v", err)
}

if !containsACL(acls, aclEntity, RoleReader) {

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.

Unfortunately this check needs to be retried as well, since it's possible/likely that the call will succeed but the check will fail.

It might be better to leave the original code for stuff with this pattern.

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.

Oh interesting... so it's possible for a situation where the metadata is updated for bucket level access policy but the acls are not yet populated?

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.

Actually I looked at this again and I think I was wrong, I believe that as long as ACL.List() succeeds then it should contain the correct items in this instance. Feel free to revert to what you had originally!

@product-auto-label product-auto-label Bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Aug 26, 2022

@tritone tritone 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.

LGTM, you can revert the changes you made for the UBLA/BPO tests based on my last review

@product-auto-label product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Aug 26, 2022
@BrennaEpp BrennaEpp merged commit c220ca2 into googleapis:main Aug 28, 2022
@BrennaEpp BrennaEpp deleted the test-cleanup branch August 28, 2022 00:41
BrennaEpp added a commit to BrennaEpp/google-cloud-go that referenced this pull request Aug 29, 2022
tritone pushed a commit that referenced this pull request Aug 31, 2022
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. size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

storage: TestIntegration_HMACKey failed storage: replace retry wrapping in tests with built-in configurability

2 participants