Recordings for storage #3194

vhvb1989 · 2021-12-17T00:13:45Z

Recordings for storage

Recordings for storage blobs

.vscode/cspell.json

sdk/core/azure-core-test/inc/azure/core/test/test_base.hpp

sdk/core/azure-core-test/src/record_policy.cpp

sdk/core/ci.yml

datalake recordings

…ings-story

…cpp into recordings-story

ci fixes

vhvb1989 · 2021-12-30T23:06:37Z

/azp run cpp - storage

azure-pipelines · 2021-12-30T23:06:53Z

Azure Pipelines successfully started running 1 pipeline(s).

…ings-story

…cpp into recordings-story

file shares recordings

Queues recordings

…ings-story

…cpp into recordings-story

vhvb1989 · 2022-01-06T02:03:47Z

/azp run cpp - storage

azure-pipelines · 2022-01-06T02:04:04Z

Azure Pipelines successfully started running 1 pipeline(s).

Jinming-Hu

Let me confirm one thing first, when running in LIVE mode, are different pipeline (Linux, Windows x64, Windows x86 etc.) running against the same storage account or different storage accounts? Since you've removed randomness, there could be problems if they run against the same account.

Jinming-Hu · 2022-01-06T02:04:40Z

sdk/storage/azure-storage-blobs/test/ut/append_blob_client_test.cpp

-    auto appendBlobClient = Azure::Storage::Blobs::AppendBlobClient::CreateFromConnectionString(
-        StandardStorageConnectionString(), m_containerName, RandomString());
+
+    auto const testName(GetTestName());


It makes it obscure to use auto keyword here. I cannot figure out what the type is at the first glance. How about

const std::string testName = GetTestName();

const std::string testName(GetTestName()); that's how I did other places.

vhvb1989 · 2022-01-06T02:45:49Z

Let me confirm one thing first, when running in LIVE mode, are different pipeline (Linux, Windows x64, Windows x86 etc.) running against the same storage account or different storage accounts? Since you've removed randomness, there could be problems if they run against the same account.

Yes, each pipeline creates an instance of storage, so, it won't be collision 💥

vhvb1989 · 2022-01-06T03:33:40Z

Finally got coverage for cpp - storage - ci:
https://dev.azure.com/azure-sdk/public/_build/results?buildId=1277527&view=codecoverage-tab

Jinming-Hu · 2022-01-06T03:54:19Z

Finally got coverage for cpp - storage - ci: https://dev.azure.com/azure-sdk/public/_build/results?buildId=1277527&view=codecoverage-tab

The coverage looks real good.

vhvb1989 · 2022-01-06T04:17:10Z

Finally got coverage for cpp - storage - ci: https://dev.azure.com/azure-sdk/public/_build/results?buildId=1277527&view=codecoverage-tab

The coverage looks real good.

It's a little bit below the real coverage from cpp - storage ( live test ) because some tests are LIVEONLY like sas and some tests depending on datetimes comparison

Jinming-Hu

Hi @vhvb1989 ,this is a huge PR and it may take quite some time for me to review it. Unfortunately, I have some higher priority task to do recently. So maybe I cannot review it until Feb.

vhvb1989 · 2022-01-07T17:44:28Z

Hi @vhvb1989 ,this is a huge PR and it may take quite some time for me to review it. Unfortunately, I have some higher priority task to do recently. So maybe I cannot review it until Feb.

Can you hold on to any making changes to Storage tests? Otherwise there would be lots of conflicts.

Also? Since all these changes are for tests only. Can we just notice that the Coverage is still the same for approving? And then you can gradually keep reviewing in the future and I can address any detail you see in there.

You know, waiting until Feb will be hard to keep this PR on sync

vhvb1989 · 2022-01-07T17:45:11Z

/azp run cpp - storage

azure-pipelines · 2022-01-07T17:45:27Z

Azure Pipelines successfully started running 1 pipeline(s).

Jinming-Hu · 2022-01-08T04:23:03Z

All right, I'll try to spare some time to review this PR before Feb.

…ings-story

vhvb1989 · 2022-01-11T17:06:35Z

/azp run cpp - storage

azure-pipelines · 2022-01-11T17:06:52Z

Azure Pipelines successfully started running 1 pipeline(s).

Jinming-Hu

I'll keep reviewing the rest of the PR tomorrow

Jinming-Hu · 2022-01-14T11:31:55Z

eng/pipelines/templates/jobs/archetype-sdk-tests.yml

@@ -207,7 +207,7 @@ jobs:
      Location: ${{ parameters.Location }}
      SubscriptionConfiguration: ${{ parameters.SubscriptionConfiguration }}

-  - script: ctest -C Debug --tests-regex ${{ parameters.CtestRegex }} --no-compress-output -T Test
+  - script: ctest -V -C Debug --tests-regex ${{ parameters.CtestRegex }} --no-compress-output -T Test


Not related to this PR

Jinming-Hu · 2022-01-14T11:34:21Z

CONTRIBUTING.md

+
+- RECORD
+
+When setting `AZURE_TEST_MODE=RECORD`, test cases will run the as when running `LIVE`. All the AZURE service network responses are recorded in a json file within the /recordings folder from the /test/ut directory. Use this test mode to generate pre-recorded data to be used on `PLAYBACK` mode.


test cases will run the as when running LIVE.

extra the?

Jinming-Hu · 2022-01-14T12:31:47Z

sdk/core/azure-core-test/inc/azure/core/test/network_models.hpp

+   */
+  class WithMemoryBodyStream : public Azure::Core::IO::BodyStream {
+  private:
+    std::vector<uint8_t> m_memory;


We don't need to make a copy here.

Jinming-Hu · 2022-01-14T12:36:08Z

sdk/core/azure-core-test/inc/azure/core/test/network_models.hpp

+    }
+
+  public:
+    CircularBodyStream(size_t size, uint8_t fillWith)


I don't know if this is used in storage tests. But buffer with the same byte or with circular data is far from ideal for storage testing scenario. Consider using runtime-generating random data that only depends on seed and offset. Here is a good example. We always use it for storage tests.

Jinming-Hu · 2022-01-14T13:25:24Z

sdk/core/azure-core-test/inc/azure/core/test/record_network_call_policy.hpp

+    // than 10Kb) and download it later.
+    // The request for upload will contain the payload to upload.
+    // Then the request for download will use the symbol to generate a bodyStream.
+    std::unique_ptr<uint8_t> m_symbol;


Since m_symbol is never null, why do we need std::unique_ptr here? How about just

uint8_t m_symbol;

Jinming-Hu · 2022-01-14T15:26:46Z

sdk/storage/azure-storage-common/test/ut/test_base.hpp

+
+    public:
+      const static Azure::ETag DummyETag;
+      const static Azure::ETag DummyETag2;


Move DummyMd5 and DummyCrc64 here.

Jinming-Hu · 2022-01-14T15:27:29Z

sdk/storage/azure-storage-common/test/ut/test_base.hpp

+        std::string name(m_testContext.GetTestSuiteName() + m_testContext.GetTestName());
+        // Make sure the name is less than 63 characters long
+        auto const nameSize = name.size();
+        size_t const maxContainerNameSize = 63;


IIRC, we have a naming rule for const variables. They should start with uppercase letters.

Jinming-Hu · 2022-01-14T15:28:06Z

sdk/storage/azure-storage-common/test/ut/test_base.hpp

+      return x * 1024 * 1024 * 1024 * 1024;
+    }
+
+    class CryptFunctionsTest : public StorageTest {


Can we just use StorageTest if the definition is empty?

Jinming-Hu · 2022-01-14T15:32:39Z

sdk/core/azure-core-test/inc/azure/core/test/test_base.hpp

+    template <class T> T InitClientOptions()
+    {
+      // Run instrumentation before creating the client
+      T options;
+      PrepareOptions(options);
+      return options;
+    }
+


Do we even need template here?

How about just

void InitClientOptions(Azure::Core::_internal::ClientOptions& options) { PrepareOptions(options); }

Or mayber we should make PrepareOptions protected.

The idea is to enable InitClientOptions() to be called without creating the options first. Like:

auto client = BlobClient(url, credential, InitClientOptions());

otherwise, the pattern would be
BlobClientOptions options;
InitClientOptions(options); // The name would change to PreparaClientOptions
auto client = BlobClient(url, credential, options);

Jinming-Hu · 2022-01-14T15:40:25Z

sdk/storage/azure-storage-common/test/ut/test_base.hpp

+        Azure::Storage::Blobs::BlobClientOptions options;
+
+        m_client = InitTestClient<
+            Azure::Storage::Blobs::BlobContainerClient,
+            Azure::Storage::Blobs::BlobClientOptions>(
+            Azure::Storage::Blobs::BlobContainerClient::CreateFromConnectionString(
+                StandardStorageConnectionString(), m_containerName)
+                .GetUrl(),
+            &m_credential,
+            options);


I don't think we need InitTestClient at all in the first place. What's the value of this function, with all the complexity of template stuff?

Azure::Storage::Blobs::BlobClientOptions options; InitClientOptions(options); m_client = std::make_unique<Azure::Storage::Blobs::BlobContainerClient>( Azure::Storage::Blobs::BlobContainerClient::CreateFromConnectionString( StandardStorageConnectionString(), m_containerName).GetUrl(), &m_credential, options);

InitTestClient<ClientClass, OptionsClass>() make it possible to re-use this call from any service. It is useful for any test case that creates a client with a tokenCredential.

The method is not a must-call method, anyway. From the test case, we can call InitClientOptions and InitCredential as well.

At the end, what we need is to let the test-framework to take care of updating the client options and credentials to support playback.

Jinming-Hu

I quit. This PR is too complicated. Let's just merge it. What's the worst thing that can happen anyway.

…ings-story

vhvb1989 · 2022-01-18T18:58:33Z

/azp run cpp - storage

azure-pipelines · 2022-01-18T18:58:50Z

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989 · 2022-01-18T19:38:47Z

/azp run cpp - storage

azure-pipelines · 2022-01-18T19:39:05Z

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989 · 2022-01-18T21:31:02Z

/azp run cpp - storage

azure-pipelines · 2022-01-18T21:31:22Z

Azure Pipelines successfully started running 1 pipeline(s).

Recordings for storage (#4)

9b958fe

Recordings for storage blobs

gearama reviewed Dec 21, 2021

View reviewed changes

.vscode/cspell.json Outdated Show resolved Hide resolved

sdk/core/azure-core-test/inc/azure/core/test/test_base.hpp Outdated Show resolved Hide resolved

sdk/core/azure-core-test/src/record_policy.cpp Show resolved Hide resolved

sdk/core/ci.yml Show resolved Hide resolved

vhvb1989 and others added 3 commits December 30, 2021 11:55

Files datalake recordings (#5)

46c0c60

datalake recordings

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into record…

ca80dde

…ings-story

Merge branch 'recordings-story' of github.com:vhvb1989/azure-sdk-for-…

bd02af0

…cpp into recordings-story

vhvb1989 self-assigned this Dec 30, 2021

Ci fixes recording (#6)

7e48d69

ci fixes

vhvb1989 and others added 6 commits January 5, 2022 23:25

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into record…

7928144

…ings-story

Merge branch 'recordings-story' of github.com:vhvb1989/azure-sdk-for-…

e68e840

…cpp into recordings-story

Files shares recordings (#7)

c5dc419

file shares recordings

Queues recordings (#8)

3da9136

Queues recordings

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into record…

1881a74

…ings-story

Merge branch 'recordings-story' of github.com:vhvb1989/azure-sdk-for-…

2a1e405

…cpp into recordings-story

vhvb1989 marked this pull request as ready for review January 6, 2022 00:11

vhvb1989 requested review from ahsonkhan, antkmsft, benbp, danieljurek, Jinming-Hu, katmsft, RickWinter, vinjiang and weshaggard as code owners January 6, 2022 00:11

vhvb1989 requested a review from gearama January 6, 2022 00:11

vhvb1989 added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) test-enhancement test-reliability Issue that causes tests to be unreliable labels Jan 6, 2022

remove

92e2c7c

Jinming-Hu reviewed Jan 6, 2022

View reviewed changes

update coverage for storage

1669598

Jinming-Hu requested changes Jan 7, 2022

View reviewed changes

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into record…

7503d28

…ings-story

Jinming-Hu reviewed Jan 14, 2022

View reviewed changes

Jinming-Hu approved these changes Jan 15, 2022

View reviewed changes

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into record…

630d6d7

…ings-story

use delete if exists

1864d12

vhvb1989 merged commit 20bb1ec into Azure:main Jan 18, 2022

vhvb1989 deleted the recordings-story branch January 18, 2022 22:50

Jinming-Hu mentioned this pull request Jan 20, 2022

Update the testing approach in storage to follow core by not having a single exe containing all the tests for all the storage services. #1200

Closed


		- RECORD

		When setting `AZURE_TEST_MODE=RECORD`, test cases will run the as when running `LIVE`. All the AZURE service network responses are recorded in a json file within the /recordings folder from the /test/ut directory. Use this test mode to generate pre-recorded data to be used on `PLAYBACK` mode.

Recordings for storage #3194

Recordings for storage #3194

Conversation

vhvb1989 commented Dec 17, 2021 • edited Loading

vhvb1989 commented Dec 30, 2021

azure-pipelines bot commented Dec 30, 2021

vhvb1989 commented Jan 6, 2022

azure-pipelines bot commented Jan 6, 2022

Jinming-Hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhvb1989 commented Jan 6, 2022

vhvb1989 commented Jan 6, 2022

Jinming-Hu commented Jan 6, 2022

vhvb1989 commented Jan 6, 2022

Jinming-Hu left a comment

Choose a reason for hiding this comment

vhvb1989 commented Jan 7, 2022

vhvb1989 commented Jan 7, 2022

azure-pipelines bot commented Jan 7, 2022

Jinming-Hu commented Jan 8, 2022

vhvb1989 commented Jan 11, 2022

azure-pipelines bot commented Jan 11, 2022

Jinming-Hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jinming-Hu Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jinming-Hu left a comment

Choose a reason for hiding this comment

vhvb1989 commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

vhvb1989 commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

vhvb1989 commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

vhvb1989 commented Dec 17, 2021 •

edited

Loading

Jinming-Hu Jan 14, 2022 •

edited

Loading