Skip to content

[Fixes #19666] [Components] Improve reliability of component quarantined tests (take 2) #21499

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 6, 2020
Merged

[Fixes #19666] [Components] Improve reliability of component quarantined tests (take 2) #21499

merged 1 commit into from
May 6, 2020

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented May 5, 2020

  • Tries to increase the reliability of the tests by:
    • Trying to ensure that the server is up and running before connecting.
    • Retrying a connection attempt multiple times.

@javiercn
[Fixes #19666] [Components] Improve reliability of component quaranti…
a5fd7c8 
…ned tests (take 2)

* Tries to increase the reliability of the tests by:
  * Trying to ensure that the server is up and running before connecting.
  * Retrying a connection attempt multiple times.
@javiercn javiercn requested a review from SteveSandersonMS as a code owner May 5, 2020 14:17
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label May 5, 2020
javiercn
javiercn commented May 5, 2020
View reviewed changes
src/Components/Ignitor/src/BlazorClient.cs
{
try
{
await HubConnection.StartAsync(CancellationToken);
Copy link
Member Author

Choose a reason for hiding this comment

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

@BrennanConroy will this leave the hubconnection in an invalid state if the call fails?

Copy link
Member

Choose a reason for hiding this comment

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

It will leave it in a disconnected state

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'm explictly asking if re-using the same hubconnection for the retries is OK

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Although I'd love to know why you think this is needed. SignalR has 100's of tests that use the HubConnection to connect to a server and doesn't need to use retries.

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'm just being cautious here. Based on the error I have not much idea of what can be going wrong. Other than the server is taking longer to start?

@javiercn javiercn requested review from a team, pranavkm and captainsafia May 5, 2020 14:19
@Pilchie
Copy link
Member

Pilchie commented May 5, 2020

👀

@BrennanConroy
Copy link
Member

Do you have any failures I can take a quick look at?

@javiercn
Copy link
Member Author

javiercn commented May 5, 2020 •
edited
Loading

Do you have any failures I can take a quick look at?

#19666

Microsoft.AspNetCore.SignalR.HubException : Unable to complete handshake with the server due to an error: Handshake was canceled.
  at Microsoft.AspNetCore.SignalR.Client.HubConnection.HandshakeAsync(ConnectionState startingConnectionState, CancellationToken cancellationToken) in /_/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs:line 1057
   at Microsoft.AspNetCore.SignalR.Client.HubConnection.StartAsyncCore(CancellationToken cancellationToken) in /_/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs:line 433
   at Microsoft.AspNetCore.SignalR.Client.HubConnection.StartAsyncCore(CancellationToken cancellationToken) in /_/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs:line 441
   at Microsoft.AspNetCore.SignalR.Client.HubConnection.StartAsyncInner(CancellationToken cancellationToken) in /_/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs:line 255
   at System.Threading.Tasks.ForceAsyncAwaiter.GetResult() in /_/src/SignalR/common/Shared/ForceAsyncAwaiter.cs:line 40
   at Microsoft.AspNetCore.SignalR.Client.HubConnection.StartAsync(CancellationToken cancellationToken) in /_/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs:line 232
   at Ignitor.BlazorClient.ConnectAsync(Uri uri, Boolean connectAutomatically) in /_/src/Components/Ignitor/src/BlazorClient.cs:line 359
   at Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests.ComponentHubReliabilityTest.ComponentLifecycleMethodThrowsExceptionTerminatesTheCircuit(String id) in /_/src/Components/test/E2ETest/ServerExecutionTests/ComponentHubReliabilityTest.cs:line 295
--- End of stack trace from previous location -

@javiercn
Copy link
Member Author

javiercn commented May 5, 2020

I believe the other potential cause is the server taking longer to start on the CI, I ran the tests locally on my box 200+ times in a loop and wasn't able to repro it.

@BrennanConroy
Copy link
Member

One interesting thing to note is that when these tests are quarantined, they don't seem to fail anymore. But when you accidentally un-quarantined them last week they failed pretty quickly.

Also, it would be nice if you could enable timestamps in the server logs. Only the client logs have timestamps currently.

@javiercn
Copy link
Member Author

javiercn commented May 5, 2020

One interesting thing to note is that when these tests are quarantined, they don't seem to fail anymore. But when you accidentally un-quarantined them last week they failed pretty quickly.

Bad luck? Karma? Destiny? Murphy's law?

Also, it would be nice if you could enable timestamps in the server logs. Only the client logs have timestamps currently.

No idea how to enable that, do you have any thoughts? If it's simple I'm happy to do it.

@BrennanConroy
Copy link
Member

Bad luck? Karma? Destiny? Murphy's law?

I'm thinking more like machine resource starvation from the parallel test runs.

No idea how to enable that, do you have any thoughts?

Hmm, none right now, might need to be added to the test logger. But something for future improvement.

@javiercn
Copy link
Member Author

javiercn commented May 5, 2020

I'm thinking more like machine resource starvation from the parallel test runs.

I didn't know we ran works in parallel. Are you thinking about thread starvation here? I believe we run the server on a separate thread (at the very least)

@BrennanConroy
Copy link
Member

We run everything in parallel I believe, so other test projects could be running, even builds and packs could be running at the same time I think.

I think I told Pranav once that you might see improved test reliability once Components moved to Helix because then you'll have a dedicated machine for just Components tests.

@javiercn
Copy link
Member Author

javiercn commented May 5, 2020

We run everything in parallel I believe, so other test projects could be running, even builds and packs could be running at the same time I think.

I think I told Pranav once that you might see improved test reliability once Components moved to Helix because then you'll have a dedicated machine for just Components tests.

I thought we had a separate test group for components. That used to be the case

@javiercn javiercn merged commit a9d7026 into master May 6, 2020
@javiercn javiercn deleted the javiercn/blazor-component-reliability-test-take2 branch May 6, 2020 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrennanConroy BrennanConroy BrennanConroy left review comments

@pranavkm pranavkm pranavkm approved these changes

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

@captainsafia captainsafia Awaiting requested review from captainsafia

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@javiercn @Pilchie @BrennanConroy @pranavkm