8000 Storage: Update conformance tests by BenWhitehead · Pull Request #5901 · googleapis/google-cloud-java · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@BenWhitehead
8000 Copy link
Contributor
@BenWhitehead BenWhitehead commented Jul 26, 2019
  • Refactor V4SigningTest to use JUnits Parameterized Test runner.
    This allows running each of the tests as an individual test thereby
    providing success/ignore/failure information per test.
  • Refactor V4SigningTest to use the generated Java Protobuf classes
    from the google-cloud-conformance-test module. The format read from is
    still json.
  • Update to use the latest revision of the conformance-tests data.
    • One of the updated tests Headers should be trimmed does not
      currently pass and has been marked as ignore until there is a
      resolution to b/136171758 relating to the specification of how
      whitespace is to be handled when signing URLs.
    • I choose to move forward with the update, to reduce the drift
      between the test definitions in conformance-tests where other
      automation is being introduced to hopefully reduce the maintenance
      burden of conformance tests.

* Refactor V4SigningTest to use JUnits Parameterized Test runner.
  This allows running each of the tests as an individual test thereby
  providing success/ignore/failure information per test.
* Refactor V4SigningTest to use the generated Java Protobuf classes
  from the google-cloud-conformance-test module. The format read from is
  still json.
* Update to use the latest revision of the conformance-tests data.
  * One of the updated tests `Headers should be trimmed` does not
    currently pass and has been marked as ignore until there is a
    resolution to b/136171758 relating to the specification of how
    whitespace is to be handled when signing URLs.
  * I choose to move forward with the update, to reduce the drift
    between the test definitions in conformance-tests where other
    automation is being introduced to hopefully reduce the maintenance
    burden of conformance tests.
@BenWhitehead BenWhitehead added api: storage Issues related to the Cloud Storage API. type: cleanup An internal cleanup or hygiene concern. labels Jul 26, 2019
@BenWhitehead BenWhitehead self-assigned this Jul 26, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2019
@codecov
Copy link
codecov bot commented Jul 27, 2019

Codecov Report

Merging #5901 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5901      +/-   ##
============================================
- Coverage     46.79%   46.79%   -0.01%     
- Complexity    25648    25663      +15     
============================================
  Files          2456     2456              
  Lines        267593   267593              
  Branches      30564    30559       -5     
============================================
- Hits         125229   125222       -7     
- Misses       133105   133112       +7     
  Partials       9259     9259
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/cloud/testing/CommandWrapper.java 87.87% <0%> (-9.1%) 13% <0%> (ø)
.../cloud/datastore/testing/LocalDatastoreHelper.java 80.59% <0%> (-4.48%) 17% <0%> (ø)
...ain/java/com/google/clo 8000 ud/pubsub/v1/Publisher.java 89.54% <0%> (-0.35%) 40% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b212217...004cfcb. Read the comment docs.

3 similar comments
@codecov
Copy link
codecov bot commented Jul 27, 2019

Codecov Report

Merging #5901 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5901      +/-   ##
============================================
- Coverage     46.79%   46.79%   -0.01%     
- Complexity    25648    25663      +15     
============================================
  Files          2456     2456              
  Lines        267593   267593              
  Branches      30564    30559       -5     
============================================
- Hits         125229   125222       -7     
- Misses       133105   133112       +7     
  Partials       9259     9259
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/cloud/testing/CommandWrapper.java 87.87% <0%> (-9.1%) 13% <0%> (ø)
.../cloud/datastore/testing/LocalDatastoreHelper.java 80.59% <0%> (-4.48%) 17% <0%> (ø)
...ain/java/com/google/cloud/pubsub/v1/Publisher.java 89.54% <0%> (-0.35%) 40% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b212217...004cfcb. Read the comment docs.

@codecov
Copy link
codecov bot commented Jul 27, 2019

Codecov Report

Merging #5901 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5901      +/-   ##
============================================
- Coverage     46.79%   46.79%   -0.01%     
- Complexity    25648    25663      +15     
============================================
  Files          2456     2456              
  Lines        267593   267593              
  Branches      30564    30559       -5     
============================================
- Hits         125229   125222       -7     
- Misses       133105   133112       +7     
  Partials       9259     9259
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/cloud/testing/CommandWrapper.java 87.87% <0%> (-9.1%) 13% <0%> (ø)
.../cloud/datastore/testing/LocalDatastoreHelper.java 80.59% <0%> (-4.48%) 17% <0%> (ø)
...ain/java/com/google/cloud/pubsub/v1/Publisher.java 89.54% <0%> (-0.35%) 40% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b212217...004cfcb. Read the comment docs.

@codecov
Copy link
codecov bot commented Jul 27, 2019

Codecov Report

Merging #5901 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5901      +/-   ##
============================================
- Coverage     46.79%   46.79%   -0.01%     
- Complexity    25648    25663      +15     
============================================
  Files          2456     2456              
  Lines        267593   267593              
  Branches      30564    30559       -5     
============================================
- Hits         125229   125222       -7     
- Misses       133105   133112       +7     
  Partials       9259     9259
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/cloud/testing/CommandWrapper.java 87.87% <0%> (-9.1%) 13% <0%> (ø)
.../cloud/datastore/testing/LocalDatastoreHelper.java 80.59% <0%> (-4.48%) 17% <0%> (ø)
...ain/java/com/google/cloud/pubsub/v1/Publisher.java 89.54% <0%> (-0.35%) 40% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b212217...004cfcb. Read the comment docs.

@chingor13 chingor13 changed the title Update Storage conformance tests Storage: Update conformance tests Jul 30, 2019
public V4SigningTest(
SigningV4Test testData,
ServiceAccountCredentials serviceAccountCredentials,
@SuppressWarnings("unused") String description) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to log out the name of the test being ran. Otherwise remove description given it's Test code and shouldn't impact the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test name is written into the test results for the test suite, and is referenced by the @Parameters(name = "{2}") here: https://github.com/googleapis/google-cloud-java/pull/5901/files#diff-265f00ab3c7dd6cdaf7e8a7392158e52R132

In the JUnit.xml picked up by the build server it is rendered like this:

  <testcase name="test[Simple GET]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.04"/>
  <testcase name="test[Simple PUT]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.015"/>
  <testcase name="test[POST for resumable uploads]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.006"/>
  <testcase name="test[Vary expiration and timestamp]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.005"/>
  <testcase name="test[Vary bucket and object]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.005"/>
  <testcase name="test[Simple headers]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.006"/>
  <testcase name="test[Headers should be trimmed]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0">
    <skipped message="Test skipped until b/136171758 is resolved: got: &quot;test[Headers should be trimmed]&quot;, expected: is not &quot;test[Headers should be trimmed]&quot;"/>
  </testcase>
  <testcase name="test[Header value with multiple inline values]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.004"/>
  <testcase name="test[Customer-supplied encryption key]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.005"/>
  <testcase name="test[List Objects]" classname="com.google.cloud.storage.V4SigningTest(sponge_log)" time="0.004"/>

In IntelliJ it renders like this:
image

Unfortunately, when using the Parameterized runner the number of properties passed to the constructor has to match exactly with the number of values provided. And in the @Parameters only allows a value that will have .toString called on it, it doesn't allow us to do something like @Parameters(name = "{1}.getDescription())").

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying.

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.

LGTM, just have that one nit.

@BenWhitehead
Copy link
Contributor Author

Thanks for the review @frankyn!

@BenWhitehead BenWhitehead merged commit d8d9742 into googleapis:master Aug 1, 2019
@BenWhitehead BenWhitehead deleted the storage-conformance-test-update branch August 1, 2019 18:42
BenWhitehead added a commit that referenced this pull request Aug 26, 2019
* Update BigTable conformance tests (#5901)

* Refactor ReadRowsMergingAcceptanceTest to use the generated Java
  Protobuf classes from the google-cloud-conformance-test module.
  The format read from is still json.
* Update to use the latest revision of the conformance-tests data.
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. cla: yes This human has signed the Contributor License Agreement. type: cleanup An internal cleanup or hygiene concern.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0