Skip to content
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

Jump to bottom

(NOT FOR SUBMISSION) Proof of concept of Storage conformance tests #2954

Closed
wants to merge 8 commits into from
Closed

(NOT FOR SUBMISSION) Proof of concept of Storage conformance tests #2954

wants to merge 8 commits into from

Conversation

jskeet
Copy link
Collaborator

@jskeet jskeet commented Mar 18, 2019

  • test.proto describes the expected format of all JSON files
  • signing.json is an example of some Signing V4 tests
  • validation is a small amount of code to validate that all the JSON files are valid
  • .travis.yml runs the validation code

This is a quick and dirty way of doing the validation; other languages may well be more suitable.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 18, 2019
@jskeet jskeet force-pushed the storage-conformance branch 2 times, most recently from fc42af5 to 4e9f6a8 Compare March 18, 2019 18:03
frankyn
frankyn suggested changes Mar 20, 2019
View reviewed changes
Copy link
Contributor

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Thanks @jskeet for putting this together.

I think this is a good idea and I agree it should be moved into a conformance-tests repository.

Question: What is the experience for a failure (bad type or bad JSON format) and is it easy to debug based on test logs?

I had one related comment that can be addressed later if not in this PR.

storage/v1/signing.json Outdated
"headers": {
"X-Goog-Encryption-Algorithm": {
"values": [
"ignored"
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly outside of scope comment:
Per discussion in bug:128647687.
The goal is to allow these three headers. However, it's not available right now.
Given it's not available, can you update this conformance test to not ignore X-Goog-Encryption-Algorithm? This will keep v2 and v4 consistent in this case.
Libraries should block until bug ref is fixed though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll update the one that's actually being used in my repo, along with my implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I can update this one as well later on if that becomes useful, of course - but it'll be just as easy to do in the "real thing" when we have that.)

Copy link
Contributor

Choose a reason for hiding this comment

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

When you say "real thing" do you mean the library implementation?

@jskeet
Copy link
Collaborator Author

jskeet commented Mar 21, 2019

Question: What is the experience for a failure (bad type or bad JSON format) and is it easy to debug based on test logs?

I've just added a commit with an invalid piece of JSON. Here's the relevant part of the Travis log

$ dotnet run -p storage/v1/validation -- storage/v1/*.json
Validating storage/v1/signing.json
Error: InvalidProtocolBufferException: Unknown field: desription
The command "dotnet run -p storage/v1/validation -- storage/v1/*.json" exited with 1.

(If there are multiple files, it will validate all of them even if there's an error in the first one - but it'll only show the first error per file.)

@frankyn frankyn mentioned this pull request Mar 21, 2019
Merged
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 25, 2019
frankyn
frankyn suggested changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Small nits based on conversation and two colons are highlighted red.

Overall LGTM and follows our discussion this morning. Thanks @jskeet.

storage/v1/signing.json
"expectedUrl": "https://storage.googleapis.com/test-bucket/test-object?X-Goog-Algorithm=GOOG4-RSA-SHA256\u0026X-Goog-Credential=test-iam-credentials%40dummy-project-id.iam.gserviceaccount.com%2F20190201%2Fauto%2Fstorage%2Fgoog4_request\u0026X-Goog-Date=20190201T090000Z\u0026X-Goog-Expires=10\u0026X-Goog-SignedHeaders=bar%3Bfoo%3Bhost\u0026X-Goog-Signature=68ecd3b008328ed30d91e2fe37444ed7b9b03f28ed4424555b5161980531ef87db1c3a5bc0265aad5640af30f96014c94fb2dba7479c41bfe1c020eb90c0c6d387d4dd09d4a5df8b60ea50eb6b01cdd786a1e37020f5f95eb8f9b6cd3f65a1f8a8a65c9fcb61ea662959efd9cd73b683f8d8804ef4d6d9b2852419b013368842731359d7f9e6d1139032ceca75d5e67cee5fd0192ea2125e5f2955d38d3d50cf116f3a52e6a62de77f6207f5b95aaa1d7d0f8a46de89ea72e7ea30f21286318d7eba0142232b0deb3a1dc9e1e812a981c66b5ffda3c6b01a8a9d113155792309fd53a3acfd054ca7776e8eec28c26480cd1e3c812f67f91d14217f39a606669d",
"headers": {
"BAR": "BAR-value"
"foo": "foo-value"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this red?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I failed to put a comma at the end of line 60. Doh!

storage/v1/test.proto
@@ -0,0 +1,19 @@
syntax = "proto3";
Copy link
Contributor

Choose a reason for hiding this comment

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

Google Copyright.

storage/v1/validation/Program.cs
@@ -0,0 +1,29 @@
using Google.Protobuf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Google Copyright.

storage/v1/signing.json
"expectedUrl": "https://storage.googleapis.com/test-bucket/?X-Goog-Algorithm=GOOG4-RSA-SHA256\u0026X-Goog-Credential=test-iam-credentials%40dummy-project-id.iam.gserviceaccount.com%2F20190201%2Fauto%2Fstorage%2Fgoog4_request\u0026X-Goog-Date=20190201T090000Z\u0026X-Goog-Expires=10\u0026X-Goog-SignedHeaders=host\u0026X-Goog-Signature=2a1d342f11ddf0c90c669b9ba89ab5099f94049a86351cacbc85845fd5a8b31e1f9c8d484926c19fbd6930da6c8d3049ca8ebcfeefb7b02e53137755d36f97baab479414528b2802f10d94541facb888edf886d91ba124e60cb3801464f61aadc575fc921c99cf8c52e281f7bc0d3e740f529201c469c8e52775b6433687e0c0dca1c6b874614c3c3d09599be1e192c40ad6827416e387bf6e88a5f501f1d8225bce498d134599d0dfe30c9c833c244d3f90cf9595b9f8175658b788ee5c4a90b575fde5e83c645772250c7098373ca754b39d0fc1ebca2f50261a015931541c9827920eba67a1c41613853a1bd23299a1f9f5d583c0feb05ea2f792ba390d27"
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a newline.

jskeet added 5 commits March 28, 2019 07:29
@jskeet
Proof of concept of Storage conformance tests
e0fdb5b 
- test.proto describes the expected format of all JSON files
- signing.json is an example of some Signing V4 tests
- validation is a small amount of code to validate that all the JSON files are valid
- .travis.yml runs the validation code

This is a quick and dirty way of doing the validation; other languages may well be more suitable.
@jskeet
Change to make a test deliberately invalid
22b531c
@jskeet
Fix deliberate typo
5cfde4a
@jskeet
Update conformance tests
387df71 
- Use a single value per header
- Remove multi-value-header tests
- Add "Header value with multiple inline values" test
- Update customer-supplied encryption key test to not ignore headers
@jskeet
Amend list objects test
d9ee6a1
@jskeet jskeet force-pushed the storage-conformance branch from a1aafb2 to d9ee6a1 Compare March 28, 2019 07:29
@jskeet jskeet added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Apr 1, 2019
@jskeet jskeet force-pushed the storage-conformance branch from a4c72d7 to 536c0ed Compare April 1, 2019 06:40
@jskeet jskeet force-pushed the storage-conformance branch from fadd59e to b49ea48 Compare April 1, 2019 07:26
@jskeet
Copy link
Collaborator Author

jskeet commented Apr 18, 2019

This is now a PR in its own repo.

@jskeet jskeet closed this Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankyn frankyn frankyn requested changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. needs work This is a pull request that needs a little love.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jskeet @frankyn @googlebot @yoshi-automation