Skip to content

Move ComponentHubReliability tests to unit and E2E tests #32834

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

Merged
merged 13 commits into from
Jun 8, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented May 18, 2021

Fixes #19666

  • Make CircuitFactory.CreateCircuitHostAsync virtual to allow mocking in unit tests. This method does the bulk of the work around initializing a circuit host and setting up the dependent services.
  • Make ComponentHub method unsealed and make certain methods virtual. This is done to make it easier to mock out parts of the API that are responsible for setting up connections.
  • Move some tests into a separate E2E test project and move others to the new ComponentHubTest unit tests.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label May 18, 2021
@captainsafia captainsafia force-pushed the safia/ch-unit-tests branch from 64df102 to e63ee59 Compare May 18, 2021 23:30
@captainsafia
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@captainsafia captainsafia marked this pull request as ready for review May 21, 2021 15:45
@captainsafia captainsafia requested a review from a team as a code owner May 21, 2021 15:45
@captainsafia captainsafia force-pushed the safia/ch-unit-tests branch from db438a6 to 7f6ae32 Compare June 3, 2021 23:42
var serviceScope = new Mock<IServiceScope>();
var circuitHost = TestCircuitHost.Create(
serviceScope: new AsyncServiceScope(serviceScope.Object));
return circuitHost;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not as familiar with the flow of these tests as you are, but is there any value in validating that the circuitKey is correct? For example storing the value supplied on the SetCircuit call and using that to decide whether to return null or not from here.

No big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this particular case, I don't believe it provides a ton of value.

The circuit key is computed on a per-connection basis (so essentially on per circuit) so in real world scenarios it's not likely to be incorrect since there's only ever one value.

That and we don't really fully exercise the per-connection logic in this scenario. If there were issues with this codepath, they'd show up as failing server-side tests.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This is great. Thanks very much for going through the process of making it more amenable to unit testing. I know that was extra steps but the end result tells a much clearer story about itself now.

@captainsafia captainsafia enabled auto-merge (squash) June 7, 2021 23:18
@@ -16,7 +16,7 @@

namespace Microsoft.AspNetCore.Components.Server.Circuits
{
internal class CircuitFactory
internal class CircuitFactory : ICircuitFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
internal class CircuitFactory : ICircuitFactory
internal sealed class CircuitFactory : ICircuitFactory

@@ -69,7 +72,7 @@ public override Task OnDisconnectedAsync(Exception exception)
{
// If the CircuitHost is gone now this isn't an error. This could happen if the disconnect
// if the result of well behaving client hanging up after an unhandled exception.
var circuitHost = GetCircuit();
var circuitHost = _circuitHandleRegistry.GetCircuit(Context.Items, CircuitKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we configure Context.Items instead of introducing the registry?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original intent here was to have an injected type that we could mock in the tests to avoid actually have to created a circuit instance. This originally started out as a singleton service with its own internal dictionary. However, that doesn't work because Context.Items is set up to correctly exist at a connection scope so I set up the registry as a wrapper around Context.Items with the ability to override in the code.

It's not possible for us to configure Context.Items directly in the test since the setting/getting happens entirely within the StartCircuit invocation so it's not possible to set up/tear down ahead of time.

@captainsafia captainsafia merged commit 4eaaaad into main Jun 8, 2021
@captainsafia captainsafia deleted the safia/ch-unit-tests branch June 8, 2021 05:17
@ghost ghost added this to the 6.0-preview6 milestone Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ComponentHubReliabilityTest are unreliable
5 participants