Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Key Vault] Update tests which disable soft-delete #15352

Merged
merged 6 commits into from
Nov 18, 2020

Conversation

mccoyp
Copy link
Member

@mccoyp mccoyp commented Nov 17, 2020

Closes #13680.

This updates tests which had previously disabled soft-delete, so they now purge deleted test resources. In samples showing backup/restore functionality we can't poll restore operations, so time.sleep() is used for live tests. This is, as far as I can tell, the best solution we have for the time being since purge operations can't be waited on.

@mccoyp mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Nov 17, 2020
restored = await client.restore_key_backup(key_backup)
self.assertEqual(created_bundle.id, restored.id)
self._assert_key_attributes_equal(created_bundle.properties, restored.properties)
await self._poll_until_no_exception(
Copy link
Member

Choose a reason for hiding this comment

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

It's weird that the async _poll_until_no_exception doesn't return the result, as does the sync equivalent 😇. Maybe this is a good time to change that. It seems like it would be straightforward, just move the loop out of the function to the one test in test_secrets_async that requires it. Then you don't need to get the resource after polling. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely agree, I think it makes sense for them to have the same return behavior. I'll look into implementing that!

await key_client.purge_deleted_key(key_name)

if self.is_live:
time.sleep(15)
Copy link
Member

Choose a reason for hiding this comment

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

time.sleep() blocks the thread and thereby the event loop, preventing other coroutines from progressing. Coroutines should yield the loop when they want to sleep:

Suggested change
time.sleep(15)
await asyncio.sleep(15)

@@ -168,7 +168,13 @@ def test_example_secrets_backup_restore(self, client, **kwargs):
print(secret_backup)

# [END backup_secret]

Copy link
Member

Choose a reason for hiding this comment

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

I've been removing empty lines after/before these markers because they render in the docs and look kind of odd, for example (compare to one without empty lines).

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't noticed that; thanks for pointing it out 👍

chlowell
chlowell previously approved these changes Nov 18, 2020
@chlowell
Copy link
Member

/check-enforcer evaluate

@mccoyp mccoyp merged commit a03f8da into Azure:master Nov 18, 2020
@mccoyp mccoyp deleted the softdelete branch November 18, 2020 19:18
iscai-msft added a commit that referenced this pull request Nov 19, 2020
…into feature/textanalytics5.2

* 'master' of https://github.com/Azure/azure-sdk-for-python: (40 commits)
  Sync eng/common directory with azure-sdk-tools for PR 1203 (#15441)
  [ServiceBus] Graceful noops for methods taking empty lists. (#15286)
  [text analytics] add sample stories and improve documents (#15429)
  [ServiceBus] Enable FQDNs and connection strings to support newlines and protocol prefixing (e.g. sb://) (#15212)
  Fix combined session+auto-auto_lock_renewer+receive-and-delete mode issue where registry would fail during receipt.  Add tests and changelog entry. (#15343)
  Add Update-python-CIConfig (#15379)
  Remove aiodns from our CI (#15424)
  Resolve Broken Portal Link (#15431)
  [Key Vault] Update tests which disable soft-delete (#15352)
  switching order on readme (#15426)
  [text analtyics] add abby as codeowner (#15376)
  test (#15402)
  fix iothub version (#15405)
  [T2] Batch (#14757)
  [T2] Policyinsights (#14794)
  [T2] apimanagement Wave3 (#14804)
  [T2] devtestlabs Wave3 (#14795)
  [T2]eventgrid wave3 (#14805)
  [T2] Search (#14823)
  test,version,CHANGELOG (#14838)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update tests which disable soft-delete
2 participants