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

Reduce Allocation when choosing the Destination #195

Merged
merged 9 commits into from
Jun 2, 2020

Conversation

Kahbazi
Copy link
Collaborator

@Kahbazi Kahbazi commented May 22, 2020

@Tratcher
Copy link
Member

Tests are failing with a stack overflow?

   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon, System.Collections.Generic.IEqualityComparer`1<System.__Canon>)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon)
   at Microsoft.ReverseProxy.Service.Proxy.Tests.LoadBalancerTests.PickDestination_SingleDestinations_ShortCircuit()

var result = loadBalancer.PickDestination(destinations, in options);
Assert.Equal(result, destinations[0]);

XUnit can't seem to figure out if it should compare these as objects or lists. I don't think there's anything wrong with your implementation, this might just be that the complier is defaulting var result to IReadOnlyList for lack of a more specific restraint. Try updating the test?

@halter73
Copy link
Member

Now DynamicState_WithHealthChecks_HonorsHealthState is failing with a stack overflow.

It seems XUnit's Assert.Equal really doesn't play nice with lists that return themselves as a list item. It just keeps recursively enumerating what it sees as an infinitely nested list. Anything that tries to call Assert.Equal on a DestinationInfo or anything containing a DestinationInfo will fail this way.

Stack overflow.
   at System.Linq.Enumerable.Where[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Collections.Generic.IEnumerable`1<System.__Canon>, System.Func`2<System.__Canon,Boolean>)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].IsSet(System.Reflection.TypeInfo)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfSetsAreEqual(System.__Canon, System.__Canon, System.Reflection.TypeInfo)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
...
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon, System.Collections.Generic.IEqualityComparer`1<System.__Canon>)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon)
   at Microsoft.ReverseProxy.RuntimeModel.Tests.BackendInfoTests.DynamicState_WithHealthChecks_HonorsHealthState()

@Kahbazi If you click the "View more details on Azure Pipelines" link at the bottom of this PR's "Checks" tab, you should be able to see the test failure details by downloading the "2 published" artifacts. We still need to fix things so test failures are properly reported. @Tratcher Is there already an issue tracking improving the test failure output in PR checks?

image

It is nice that stack overflows finally show stack traces though! dotnet/runtime#31956

@Tratcher
Copy link
Member

Is there already an issue tracking improving the test failure output in PR checks?

I had assumed the test reporting was bad in this case because of the StackOverflow. I can try it with a more typical error in another PR.

@Tratcher
Copy link
Member

Yeah, normal test failures work. StackOveflow is special.
https://dev.azure.com/dnceng/public/_build/results?buildId=664478&view=ms.vss-test-web.build-test-results-tab&runId=20601422&resultId=100187&paneView=debug

@Kahbazi Kahbazi requested a review from Tratcher May 31, 2020 17:24
@Tratcher Tratcher requested a review from alnikola May 31, 2020 19:16
@Tratcher Tratcher added this to the 1.0.0-preview2 milestone Jun 2, 2020
@Tratcher Tratcher self-assigned this Jun 2, 2020
@Tratcher Tratcher merged commit 10a355d into microsoft:master Jun 2, 2020
@Tratcher
Copy link
Member

Tratcher commented Jun 2, 2020

Thanks

@Kahbazi Kahbazi deleted the kahbazi/DestinationArray branch June 3, 2020 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants