Fix create if exists live tests #28606
Conversation
rickle-msft
requested review from
alzimmermsft,
amishra-dev,
jaschrep-msft and
kasobol-msft
as code owners
ghost
added
the
|
API change check for API changes are not detected in this pull request for |
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| def "Delete if exists service container min"() { | ||
| setup: | ||
| def containerName = generateContainerName() | ||
| primaryBlobServiceClient.createBlobContainer(containerName) | ||
|
|
||
| when: | ||
| def result = premiumBlobServiceClient.deleteBlobContainerIfExists(containerName) | ||
|
|
||
| then: | ||
| !premiumBlobServiceClient.getBlobContainerClient(containerName).exists() | ||
| result | ||
| } | ||
|
|
||
| def "Delete if exists service container that does not exist"() { | ||
| when: | ||
| def response = premiumBlobServiceClient.deleteBlobContainerIfExistsWithResponse(generateContainerName(), null) | ||
|
|
||
| then: | ||
| response.getStatusCode() == 404 | ||
| } | ||
|
|
There was a problem hiding this comment.
did these overlap with other tests?
There was a problem hiding this comment.
No I just didn't like them cus they were failing.
Kidding ;). Yes. There are identical tests in ServiceApiTests, which is where they belong as they are methods on the service client
| response.getStatusCode() == 404 | ||
| } | ||
|
|
||
| // We can't guarantee that the requests will always happen before the container is garbage collected |
There was a problem hiding this comment.
the other option is to refactor this test (and other similar tests) to poll until container gets GC'ed for some period of time before executing the api under test.
There was a problem hiding this comment.
Btw. do we return true when we get 409 with ContainerBeingDeleted ? I would think we should return false in that case.
There was a problem hiding this comment.
It's the opposite problem for this test. The test is checking the behavior of issuing the request while the container is still being GC'ed. It looks like the live pipelines are sometimes waiting too long and not getting the expected response as a result. I don't think we can guarantee the second request comes within a particular time frame, so the most consistent thing to me seems to be to make it playback only.
Let me run the pipeline before that commit, though, and double check what the behavior is.
There was a problem hiding this comment.
do other APIs still work while container is GC'ed? if not then I'd return false from the API and consider the container as "deemed to be gone".
Also looking from another angle - if customer issues multiple calls to this API then only first should return true because it's the one that actually triggers deletion.
There was a problem hiding this comment.
Looks like we may actually have different behavior between sync and async.
Async is statusCode != 404
Sync is statusCode == 202
That said, the Rest docs seem to state that 409 only comes with creating a container not yet GC'ed and that deleting twice should always 404, so in effect these may be the same.
There was a problem hiding this comment.
sounds like a bug then. I think Sync is doing right thing.
There was a problem hiding this comment.
I'll update async
There was a problem hiding this comment.
@kasobol-msft Doing some experimenting, it looks like the service actually returns 202 during the GC phase for the record. We can still keep the logic to check that the status is 202, but FYI.
I also think it's best to still keep those tests as playback only because we can't guarantee that the second request comes within the GC period, and I believe that's what's tripping up the live tests on this PR
|
Overriding the check enforcer that is blocked on failing live tests. The tests relevant to this PR are passing and fixes for the other failing live tests will come in another PR. |
|
/check-enforcer override |
3 participants
Fix some tests that were consistently failing in the nightly builds.
These tests were mixing up the BlobServiceClient instance they were using. Changing the tests to be consistent in which client they use fixes the issue