[Storage][tests] Use "Exists" APIs to improve test reliability

[mikeharder]

• In test code, all calls to Create() or Delete() were replaced by CreateIfNotExists() and DeleteIfExists(), with the exception of calls explicitly testing Create() or Delete(), and calls which require the response (since the "Exists" APIs may return a null response).

• The non-"Exists" APIs may fail at any time if the server failed to respond to the first HTTP request and it was retried

PRs which added "Exists" APIs to product code

• Blob: [Storage] [feature] CreateIfNotExists/DeleteIfExists for Blobs #7881
• Files.Shares: Added Exists(), CreateIfNotExists(), and DeleteIfExists() to ShareCli… #10074
• Files.DataLake: Added Exists, CreateIfNotExists, and DeleteIfExists to Data Lake #9996
• Queues: Added QueueClient.CreateIfNotExists(), .Exists(), add .DeleteIfExists() #10050

[mikeharder]

/azp run net - storage - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mikeharder]

/azp run net - storage - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mikeharder]

@tg-msft, @JoshLove-msft: Looks like this PR was forgotten. I just rebased against master and am running tests. Also happy to merge to a different branch like feature/storage if you want.

[mikeharder]

@tg-msft, @JoshLove-msft: Recorded tests are failing. If I recall, it's because the recordings need to be updated since the Exists APIs add an If-None-Match header. I can update the recordings if you share instructions, or one of you could do it.

[mikeharder]

@amishra-dev, @seanmcc-msft: Would either of you like to review this, and would you prefer I merge this to master or a different branch? These are just fixes to improve test reliability so any branch is fine with me.

[seanmcc-msft]

Hi @mikeharder , could you merge this to master and feature/storage/stg73base?

[mikeharder]

@seanmcc-msft: Sure, I will merge to both those branches once this is ready.

[Petermarcu]

Is this PR still moving forward?

[mikeharder]

@Petermarcu: Yes. @tg-msft has agreed to update the test recordings and then merge this PR.

[seanmcc-msft]

@mikeharder @Petermarcu

I'm not sure if this PR is necessary. The majority of our unit tests use DisposableContainer/Share/FileSystem, which are created at the beginning of the test, and deleted in a finally loop at the end of the test.

Even if the containers were not deleted, new ones are created the next time a test is run. I'm not sure how we would ever run into a conflicting Blob/SMB File/Data Lake File.

[mikeharder]

I'm not sure if this PR is necessary. The majority of our unit tests use DisposableContainer/Share/FileSystem, which are created at the beginning of the test, and deleted in a finally loop at the end of the test.

Even if the containers were not deleted, new ones are created the next time a test is run. I'm not sure how we would ever run into a conflicting Blob/SMB File/Data Lake File.

This PR will improve test reliability. Further, I think we should encourage customers to use the *Exists APIs by default unless they need to change behavior based on whether the resource already exists.

The non-Exists APIs always have a small chance of failing even if the resource was in the correct state before the API call, and we have seen these failures regularly in our test results. Here's the issue as I understand it:

1. App calls API like BlobContainerClient.CreateAsync() with a name that has never existed (say a GUID)
2. SDK calls Create Container REST API
3. Occasionally (say 1 in 5000 requests), the REST API will return an error. Sometimes the container was actually created in the service, but sometimes it was not, and the client cannot disambiguate.
4. SDK retries the Create Container REST API call.
5. If the container was actually created in the service, the REST API will return an error that the container already exists, and the BlobContainerClient.CreateAsync() API will throw an exception.

The only way to avoid these occasional exceptions is to use the *Exists APIs.

[jsquire]

@tg-msft - Are you still planning to update and merge or can this be closed out?

[jsquire]

@mikeharder - Is this still an active work stream or should we close out the PR?

[mikeharder]

I put a fair bit of work into this, and it's important for test reliability, so I would like to see it get merged. I believe the remaining work is for someone to update the recordings and commit them with the test changes. @tg-msft has agreed to do this when he gets time.

[JoshLove-msft]

I put a fair bit of work into this, and it's important for test reliability, so I would like to see it get merged. I believe the remaining work is for someone to update the recordings and commit them with the test changes. @tg-msft has agreed to do this when he gets time.

I was planning on re-recording these in a separate PR.

[jsquire]

@JoshLove-msft - Is there an issue that we can link with this for the remaining work to keep apprised of scheduling?

[mikeharder]

My changes were made a long time ago, so whomever takes over this PR should also see if any new tests were added since my changes, and update them in the same way (always use an "Exists" API, unless you are explicitly testing the non-Exists API).

[tg-msft]

Fixed with #15638