adobe / aem-testing-clients

Testing tools for Adobe Experience Manager
Apache License 2.0
50 stars 39 forks source link

fix CQAssetsClient failing to retry binary upload (PUT) requests #47

Closed alexkli closed 3 years ago

alexkli commented 3 years ago

Cloud blob store (Azure Blob Storage or S3) relatively frequently return temporary 503 errors that can be solved with a retry. The http client here is already retrying automatically, but retries fail with

Cannot retry request with a non-repeatable request entity

because it uses an InputStreamEntity which is non-repeatable as the input stream is already consumed after the first request.

The fix changes that to an EntityTemplate which will recreate the input stream on every retry.

Related issue: CQ-4311192.

codecov[bot] commented 3 years ago

Codecov Report

Merging #47 (34fab6f) into aem-cloud (89a27d5) will not change coverage. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             aem-cloud     #47   +/-   ##
===========================================
  Coverage         3.40%   3.40%           
  Complexity          47      47           
===========================================
  Files              141     141           
  Lines             5609    5609           
  Branches           566     565    -1     
===========================================
  Hits               191     191           
  Misses            5395    5395           
  Partials            23      23           
Impacted Files Coverage Δ Complexity Δ
...va/com/adobe/cq/testing/client/CQAssetsClient.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)

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 89a27d5...34fab6f. Read the comment docs.

alexkli commented 3 years ago

cc @volteanu @ireasor

alexkli commented 3 years ago

Ping @volteanu @dulvac @ireasor

This fixes a bug that improves test stability.

Not sure what to do with the codecov failure, CQAssetClient does not have any test coverage yet and I didn't have the time to work on that foundation.

dulvac commented 3 years ago

I haven't tested this, but lgtm

ireasor commented 3 years ago

Not sure what to do with the codecov failure, CQAssetClient does not have any test coverage yet and I didn't have the time to work on that foundation.

@dulvac is this a concern? It looks like coverage in this repository is very low - I only see a handful of tests. If so, I can log a ticket for someone to come in and add test coverage of this client.

dulvac commented 3 years ago

@dulvac is this a concern? It looks like coverage in this repository is very low - I only see a handful of tests. If so, I can log a ticket for someone to come in and add test coverage of this client.

@ireasor , It would be nice if you contributed unit tests as well, but it shouldn't block this PR. @volteanu , we're good to merge and release this?

volteanu commented 3 years ago

merged it as it is since I didn't find the time to address the coverage

volteanu commented 3 years ago

this has been included in aem-cloud-testing-clients-0.6.1

alexkli commented 3 years ago

Thanks!