feat(storage): Add retry conformance test #18230

bajajneha27 · 2022-05-10T10:06:50Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea.
Follow the instructions in CONTRIBUTING. Most importantly, ensure the tests and linter pass by running bundle exec rake ci in the gem subdirectory.
Update code documentation if necessary.

closes: b/228274210

…operations

…orted, remove broken-stream and reset-connection tests

shivgautam · 2022-09-28T03:00:00Z

google-cloud-storage/Rakefile

  end
 end

+#Conformance Tests


Add space here

google-cloud-storage/test/helper.rb

google-cloud-storage/conformance/v1/proto/google/cloud/conformance/storage/v1/tests.proto

google-cloud-storage/test/google/cloud/storage/retry/conformance_test.rb

shivgautam · 2022-09-28T03:55:33Z

google-cloud-storage/test/google/cloud/storage/retry/conformance_test.rb

+
+      success_result = true
+      begin
+        run_retry_test test_id, lib_func, scenario["preconditionProvided"], method.resources


Why scenario["preconditionProvided"] & not scenario.preconditionProvided?

No specific reason. Since it was a JSON key, I used the string

shivgautam · 2022-09-28T03:56:59Z

google-cloud-storage/test/google/cloud/storage/retry/conformance_test.rb

+# The Retry Test API in the testbench is used to run the retry conformance tests. It offers a 
+# mechanism to describe more complex
+# retry scenarios while sending a single, constant header through all the HTTP requests from a
+# test program. The Retry Test API can be accessed by adding the path "/retry-test" to the host. # See also: https://github.com/googleapis/storage-testbench


Limit to 80 characters. # See also.... can move to next line.

shivgautam · 2022-09-28T04:17:36Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+    object = resources[:object]
+    object_2 = bucket.create_file StringIO.new(CONF_TEST_FILE_CONTENT), "my-test-file-2"
+    destination = "new-composite-object"
+    if _preconditions


underscored variable used.

shivgautam · 2022-09-28T04:18:45Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+  def self.delete_file client, _preconditions, **resources
+    bucket = resources[:bucket]
+    object = resources[:object]
+    if _preconditions


Same here. underscored variable

shivgautam · 2022-09-28T04:19:12Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+  def self.patch_file client, _preconditions, **resources
+    bucket = resources[:bucket]
+    object = resources[:object]
+    if _preconditions


shivgautam · 2022-09-28T04:19:23Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+  def self.rewrite_file client, _preconditions, **resources
+    bucket = resources[:bucket]
+    object = resources[:object]
+    if _preconditions


shivgautam · 2022-09-28T04:19:41Z

google-cloud-storage/Rakefile

  end
 end

+#Conformance Tests


google-cloud-storage/conformance/v1/proto/google/cloud/conformance/storage/v1/tests.proto

google-cloud-storage/test/google/cloud/storage/retry/conformance_test.rb

dazuma · 2022-09-28T05:28:10Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+  end
+
+  def self.insert_bucket client, _preconditions, **resources
+    bucket_name = "new_bucket" + Time.now.to_i.to_s


Agreed. It's best to append some random digits to prevent this.

dazuma · 2022-09-28T05:30:22Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+
+  def self.bucket_acl_public client, _preconditions, **resources
+    bucket = resources[:bucket]
+    if _preconditions


dazuma · 2022-09-28T05:37:35Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+
+  def self.insert_object client, _preconditions, **resources
+    bucket = resources[:bucket]
+    file = StringIO.new CONF_TEST_FILE_CONTENT * 1024 * 1024 # 1MB


Note this is actually 1024 * 1024 * CONF_TEST_FILE_CONTENT.size which is 12MB rather than 1MB. Just making sure that's what you intend for this object.

Yes, it's indeed 12 MB.
We needed a use case where we're doing multi chunk upload. The default chunk size is 8MB, so needed an input bigger than that.

shivgautam

LGTM

dazuma

LGTM

tritone

Overall looks great, very clear and well factored. I know this was a big effort so thank you @bajajneha27 !

tritone · 2022-09-29T01:27:13Z

google-cloud-storage/conformance/README.md

 ## Running the tests

 The conformance tests are included in the ordinary unit test suite. In the `google-cloud-storage` directory, run `rake test`.
+


Does the emulator need to be launched separately in order to run the tests? If so worth noting how to do that.

Yes, it does. Updated README to include that. Thanks.

tritone · 2022-09-29T01:32:02Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+
+  def self.get
+  {
+    "storage.bucket_acl.list" => [:list_bucket_acls],


Nit: could you sort these by resource / alphabetically? Otherwise it's a bit difficult to see all of what's implemented.

Makes sense. Updated it alphabetically.

tritone · 2022-09-29T01:38:04Z

google-cloud-storage/test/google/cloud/storage/retry/utils.rb

+    "storage.buckets.setIamPolicy" => [:set_bucket_policy],
+    "storage.hmacKey.update" => [:update_hmac_key],
+    "storage.resumable.upload" => [
+      :blob_upload_from_string,


Also, I notice that I think these still need to be implemented, correct? Fine to do it in a separate PR.

I've implemented them as part of storage.objects.insert . If we need to implement more varieties of it, can be done in separate PR.

product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label May 10, 2022

bajajneha27 force-pushed the storage/retry-conformance branch from c8bba11 to c10217e Compare May 12, 2022 07:34

bajajneha27 force-pushed the storage/retry-conformance branch 2 times, most recently from af1f30d to 1262e47 Compare June 9, 2022 08:48

bajajneha27 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 9, 2022

bajajneha27 force-pushed the storage/retry-conformance branch 4 times, most recently from e558204 to 2e60107 Compare June 20, 2022 05:26

bajajneha27 force-pushed the storage/retry-conformance branch from 2e60107 to fe58b5d Compare June 28, 2022 09:11

bajajneha27 force-pushed the storage/retry-conformance branch 3 times, most recently from e7a0dd5 to 5331212 Compare July 7, 2022 11:07

bajajneha27 force-pushed the storage/retry-conformance branch 4 times, most recently from c597c25 to 61c96a4 Compare July 18, 2022 11:32

bajajneha27 force-pushed the storage/retry-conformance branch from 61c96a4 to 361c371 Compare August 22, 2022 09:08

bajajneha27 force-pushed the storage/retry-conformance branch 2 times, most recently from af561fa to 5686331 Compare September 7, 2022 16:10

bajajneha27 force-pushed the storage/retry-conformance branch from 3297f9c to b64fa30 Compare September 12, 2022 11:21

bajajneha27 marked this pull request as ready for review September 15, 2022 05:45

bajajneha27 requested review from a team as code owners September 15, 2022 05:45

bajajneha27 force-pushed the storage/retry-conformance branch from 790e0a3 to b88be1b Compare September 20, 2022 10:54

bajajneha27 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 26, 2022

bajajneha27 added 4 commits September 26, 2022 13:58

feat(storage): Add retry conformance test

3c7d396

cleaning up

36f9ca8

cleaning up

c1acd51

add tests for resumable uploads

e9651be

bajajneha27 added 13 commits September 26, 2022 13:58

add preconditions for patch_bucket

4565db6

non idempotent operations I

7ff170e

non-idempotent operation II and add missing conditionally idempotent …

d23bac6

…operations

add preconditions for conditionally idempotent operations

a9e390a

delete resources after each test case

8372780

chore(storage): remove test cases where etag precondition is not supp…

3c0c5a7

…orted, remove broken-stream and reset-connection tests

chore(storage): github action for retry conformance test

43b9aaf

chore(storage): see the file structure

8e26f39

chore(storage): correct path for conformance test

2309917

chore(storage): new rake task for conformance testing

d798e11

chore(storage): set debug mode on temporarily

09a7ecb

feat(storage): Bump up the storage_v1 version to 0.19.0

5075e85

fix: precondition for object rewrite

25c4c16

bajajneha27 force-pushed the storage/retry-conformance branch from 10a3309 to 25c4c16 Compare September 26, 2022 14:03

shivgautam reviewed Sep 28, 2022

View reviewed changes

shivgautam suggested changes Sep 28, 2022

View reviewed changes

bajajneha27 requested a review from tritone September 28, 2022 04:37

dazuma reviewed Sep 28, 2022

View reviewed changes

bajajneha27 added 3 commits September 28, 2022 13:49

chore(storage): address review comments

e80657f

chore(storage): address review comments

7cef4a2

chore(storage): limit to 80 characters per line

5c7e38f

shivgautam approved these changes Sep 28, 2022

View reviewed changes

dazuma approved these changes Sep 28, 2022

View reviewed changes

tritone approved these changes Sep 29, 2022

View reviewed changes

tritone reviewed Sep 29, 2022

View reviewed changes

chore(storage): address review comments

bec83c4

bajajneha27 merged commit 8a35f3d into googleapis:main Sep 30, 2022

yoshi-code-bot mentioned this pull request Sep 30, 2022

chore(main): release google-cloud-storage 1.43.0 #19241

Merged

		## Running the tests

		The conformance tests are included in the ordinary unit test suite. In the `google-cloud-storage` directory, run `rake test`.

feat(storage): Add retry conformance test #18230

feat(storage): Add retry conformance test #18230

Uh oh!

Conversation

bajajneha27 commented May 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shivgautam left a comment

Choose a reason for hiding this comment

Uh oh!

dazuma left a comment

Choose a reason for hiding this comment

Uh oh!

tritone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants