AAD test coverage#46568
Conversation
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…zure/azure-sdk-for-python into users/dibahl/aad-test-coverage # Conflicts: # sdk/cosmos/aad.tests.yml
|
/azp run python - cosmos - tests |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run python - cosmos - tests |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sdkReviewAgent-2 |
| else: | ||
| print("Waiting for split to complete") | ||
| time.sleep(SLEEP_TIME) | ||
| await asyncio.sleep(SLEEP_TIME) |
There was a problem hiding this comment.
💬 Observation — Good Bug Fix: time.sleep → await asyncio.sleep
await asyncio.sleep(SLEEP_TIME)This fixes a real pre-existing bug. The old time.sleep(SLEEP_TIME) was inside an async def trigger_split_async() method, blocking the entire asyncio event loop for 30 seconds per iteration. This prevented any concurrent async operations from running during the split wait loop. The fix to await asyncio.sleep() correctly yields control back to the event loop.
👍 Nice catch.
|
✅ Review complete (51:20) Posted 15 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| @pytest.mark.semanticReranker | ||
| @pytest.mark.cosmosAADLong | ||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Why would we want to skip ? Shouldn't we fail if the test isn't setup correctly
There was a problem hiding this comment.
either way we are currently skipping all the semanticReranker tests (we don't have that configured for any run) since it relies on a separate service - though with that in mind, do we need this skip flag at all Dikshi?
There was a problem hiding this comment.
Good point — I removed the skipif as its dead weight anwyay since semantic reranker infra isnt there, I’ll keep these tests under the semanticReranker marker for now.
|
✅ Review complete (51:44) Posted 4 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
|
@sdkReviewAgent-2 |
|
✅ Review complete (45:54) Posted 2 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5 participants
This PR attempts to enhance the test coverage to include Data-plane user-data operations. It doesn't not cover ARM control plane or Data-plane resource management operations.
Currently there are 2 known gaps in the AAD workflow-
Known Issue — 403/5302 right after creating a new container under AAD - service side - there is a follow up going on.
403/5300 After Token Expiry - need to follow up with the client to see if they doing any custom work on Token Credential and have them provide more information if it happens next time - there is a separate thread around this.
Please note for now I have annotated the all the AAD tests with skip notation except one to ensure pipeline set up is correct. will enable all tests once this is confirmed.