-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Updating test-resources for communication #15551
Conversation
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
1 similar comment
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines failed to run 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -147,6 +148,8 @@ public void E2E_ThreadCreateUpdateGetDelete_MemberAddUpdateRemove_MessageGetSend | |||
chatClient.DeleteChatThread(threadId); | |||
#endregion Snippet:Azure_Communication_Chat_Tests_E2E_DeleteChatThread | |||
|
|||
Thread.Sleep(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@minnieliu These are synchronous calls, we should not be adding Thread.Sleep on our tests. Any reason we need this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is flakey in the pipeline, see this: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=559498&view=logs&j=fb683405-d979-52da-6de9-2541dff429a6&t=47b32f90-f4d1-5232-a550-dd1152acf48a
In Java, a 500ms sleep was added to the tests as a temporary solution. There is already a bug created on the server side and they will be looking into the delay https://skype.visualstudio.com/SPOOL/_workitems/edit/2271329/. Once this is fixed on server, the sleep can be removed. Let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread sleep here is not doing much actually. This value was taken before :
var readReceiptsCount = readReceipts.Count();
Probably what we are seeing here is a case where the read receipt is sent, and downstream on MFE it gets stored, but due to distributed storage when we do a get it is probably not there yet (we will confirm that soon). I suggest we just remove the thread sleeps and comment the readreciept assertion.
@@ -253,7 +258,7 @@ public async Task E2E_ThreadCreateUpdateGetDelete_MemberAddUpdateRemove_MessageG | |||
Assert.AreEqual(3, chatThreadMembersAfterOneAddedCount); | |||
Assert.AreEqual(2, chatThreadMembersAfterOneDeletedCount); | |||
Assert.AreEqual((int)HttpStatusCode.OK, typingNotificationResponse.Status); | |||
Assert.AreEqual(1, readReceiptsCount); | |||
//Assert.AreEqual(1, readReceiptsCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of commenting this out, we should look into why is not passing (if that is the case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, thanks for catching this. I commented it out to see if pipeline tests would pass without it (it does) and missed removing this comment. Fixing in next commit
EnvVars: | ||
AZURE_COMMUNICATION_TEST_MODE: Record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we will run in 'Record' mode on CI ? My understanding is that it should run on Playback mode
@JoshLove-msft is that right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is newly introduced and only used in the service.projects file (added in this PR) as a condition to skip SMS tests in the live test automation since we dont have a good solution to automating generation of phone number resources yet. In addition, I believe that tests.yml files are specifically used for the live test automation pipeline and not the build pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a different understanding of Recorded vs Live tests, but I may be wrong.
I will talk to the azure folks today to clarify how this should work.
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -5,6 +5,7 @@ | |||
using System.Collections.Generic; | |||
using System.Linq; | |||
using System.Net; | |||
using System.Threading; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is not needed anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, I will remove :)
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - communication - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
* Updating test-resources.json to automate live tests * Update spacing * Remove discrepency * Run against the public cloud * Skip SMS automation tests for now * Skip SMS automation tests for now * Remove the test csproj * Update service projects to remove sms automated tests * Update service projects to remove sms automated tests * Update service projects to remove sms automated tests * Update service projects to remove sms automated tests * Testing chat fix * Adding condition to remove sms * Updating condition to remove sms * Adding sleep to failing chat tests * Testing chat fix * Comment out flakey assert * Clean up * Adding TODOs for tests Co-authored-by: Minnie Liu <peiliu@microsoft.com>
No description provided.