Enable default server address test #1436

JunTaoLuo · 2017-03-03T07:37:24Z

aspnet/Hosting#917 has now been merged. So enabling this test. Will merge to feature/dev-si once approved.

JunTaoLuo · 2017-03-03T07:38:25Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/AddressRegistrationTests.cs

@@ -147,7 +147,8 @@ public class AddressRegistrationTests
            {
                host.Start();

-                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug);
+                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug &&
+                    !log.Message.StartsWith("hosting", StringComparison.OrdinalIgnoreCase));


Need to filter out two hosting logs "Hosting starting" and "Hosting started".

Do a case-sensitive comparison. You want to be as specific as possible here.

cesarblum · 2017-03-03T18:53:41Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/AddressRegistrationTests.cs

@@ -147,7 +147,8 @@ public class AddressRegistrationTests
            {
                host.Start();

-                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug);
+                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug &&
+                    !log.Message.StartsWith("hosting", StringComparison.OrdinalIgnoreCase));


Do a case-sensitive comparison. You want to be as specific as possible here.

cesarblum · 2017-03-03T19:08:01Z

Some thoughts:

If we're verifying log messages, we should actually assert the exact string we're looking for.
We have an extension method for WebHostBuilder called GetPort(). You could assert that in the test too.
Having the test hit localhost makes the test imprecise. What is it resolving to? If the default address is 127.0.0.1:5000, hit that. If [::1]:5000 is also a default, hit that too. Hitting localhost is not testing anything relevant.

I know those things were already there, but since you're at it... 😄

halter73 · 2017-03-03T23:24:56Z

I agree with @CesarBS about testing both 127.0.0.1:5000 and [::1]:5000 though.

JunTaoLuo · 2017-03-03T23:54:14Z

The changes in https://github.com/aspnet/KestrelHttpServer/pull/1312/files seems to only add the ipv4 by default when no address is configured. I'll need to address #1434 and bind to both by default. I'll adapt this PR and actually change the behaviour.

halter73 · 2017-03-04T00:03:14Z

Right, I forgot that the change to hosting was going to break that.

It's slightly trickier to fix than it looks since we need to special case not throwing in KestrelServer.Start if binding to only one of 127.0.0.1:5000 and [::1]:5000 fails (like on Azure Web Apps). We also need to make sure the logs aren't too scary when the binding fails.

@JunTaoLuo Merge this as is.

JunTaoLuo · 2017-03-04T00:30:21Z

I see okay then I'll just merge the current changes.

JunTaoLuo · 2017-03-04T01:02:33Z

🆙📅 @CesarBS

cesarblum · 2017-03-04T01:16:18Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/AddressRegistrationTests.cs

+
+                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug &&
+                    !log.Message.StartsWith("Hosting start", StringComparison.Ordinal)); // Filter out "Hosting starting" and "Hosting started"
+                Assert.True(string.Equals(debugLog.Message, $"No listening endpoints were configured. Binding to {Constants.DefaultIPEndPoint} by default.", StringComparison.Ordinal));


Can make this simpler by doing

Assert.Equal( $"No listening endpoints were configured. Binding to {Constants.DefaultIPEndPoint} by default.", testLogger.Messages.SingleOrDefault(log => log.LogLevel == LogLevel.Debug && !log.Message.StartsWith("Hosting start", StringComparison.Ordinal)));

cesarblum · 2017-03-04T02:38:44Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/AddressRegistrationTests.cs

-                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug &&
-                    !log.Message.StartsWith("Hosting start", StringComparison.Ordinal)); // Filter out "Hosting starting" and "Hosting started"
-                Assert.True(string.Equals(debugLog.Message, $"No listening endpoints were configured. Binding to {Constants.DefaultIPEndPoint} by default.", StringComparison.Ordinal));
+                Assert.Equal($"No listening endpoints were configured. Binding to {Constants.DefaultIPEndPoint} by default.",


Even simpler (sorry, should have realized this earlier):

Assert.NotNull(testLogger.Messages.SingleOrDefault(log => log.LogLevel == LogLevel.Debug && log.Message.Equals($"No listening endpoints were configured. Binding to {Constants.DefaultIPEndPoint} by default.", StringComparison.Ordinal)));

You get your StringComparison.Ordinal back 😝

That depends on the original intention of the test. I read it as, there is only one message from Kestrel and it's the binding to default address test. With this change, it becomes, there's is only one binding to default address message but I don't care if there are other messages from Kestrel. Functionally, there's no difference since right now there is only one message on startup.

All we care about is checking that the message is logged. Doesn't matter if there are other messages in there.

BTW, xUnit has Assert.Single which makes this even shorter 😄

cesarblum · 2017-03-06T18:07:09Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/AddressRegistrationTests.cs


-                foreach (var testUrl in new[] { "http://127.0.0.1:5000", "http://localhost:5000" })
+                foreach (var testUrl in new[] { "http://127.0.0.1:5000", /* "http://[::1]:5000" */})


Don't leave IPv6 commented out if it's currently not part of the default binding. We can add it if/when it becomes the default.

We need to add it back soon. Similar to 4533383.

When is soon?

I'm working on it now

cesarblum · 2017-03-06T18:07:33Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/AddressRegistrationTests.cs

-                var debugLog = testLogger.Messages.Single(log => log.LogLevel == LogLevel.Debug);
-                Assert.True(debugLog.Message.Contains("default"));
+                Assert.Equal(5000, host.GetPort());
+                Assert.Single(testLogger.Messages.Where(log => log.LogLevel == LogLevel.Debug &&


Use the overload that takes a collection and a predicate, then you don't need the Where.

cesarblum

🚢 when green.

JunTaoLuo requested review from halter73 and cesarblum March 3, 2017 07:37

dnfclas added the cla-already-signed label Mar 3, 2017

JunTaoLuo commented Mar 3, 2017

View reviewed changes

cesarblum suggested changes Mar 3, 2017

View reviewed changes

halter73 approved these changes Mar 3, 2017

View reviewed changes

JunTaoLuo changed the title ~~Enable default port test~~ Bind to both ipv4 and ipv6 loopback by default when no address is explicitly configured Mar 3, 2017

JunTaoLuo changed the title ~~Bind to both ipv4 and ipv6 loopback by default when no address is explicitly configured~~ Enable default server address test Mar 4, 2017

JunTaoLuo force-pushed the johluo/enable-default-portal-test branch from 934c469 to a0ee9b7 Compare March 4, 2017 01:12

cesarblum reviewed Mar 4, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/enable-default-portal-test branch 2 times, most recently from fe78f02 to c959c5b Compare March 5, 2017 06:04

cesarblum reviewed Mar 6, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/enable-default-portal-test branch from c959c5b to 335b7c2 Compare March 6, 2017 18:22

cesarblum approved these changes Mar 6, 2017

View reviewed changes

Enable default server address test

7d94abd

JunTaoLuo force-pushed the johluo/enable-default-portal-test branch from 335b7c2 to 7d94abd Compare March 6, 2017 18:26

JunTaoLuo merged commit 7d94abd into dev Mar 6, 2017

JunTaoLuo deleted the johluo/enable-default-portal-test branch March 6, 2017 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable default server address test #1436

Enable default server address test #1436

JunTaoLuo commented Mar 3, 2017 •

edited

Loading

JunTaoLuo Mar 3, 2017

cesarblum Mar 3, 2017

cesarblum Mar 3, 2017

cesarblum commented Mar 3, 2017

halter73 commented Mar 3, 2017

JunTaoLuo commented Mar 3, 2017

halter73 commented Mar 4, 2017

JunTaoLuo commented Mar 4, 2017

JunTaoLuo commented Mar 4, 2017

cesarblum Mar 4, 2017

cesarblum Mar 4, 2017 •

edited

Loading

JunTaoLuo Mar 4, 2017

cesarblum Mar 4, 2017

cesarblum Mar 6, 2017

JunTaoLuo Mar 6, 2017

cesarblum Mar 6, 2017

JunTaoLuo Mar 6, 2017

cesarblum Mar 6, 2017

cesarblum left a comment


		foreach (var testUrl in new[] { "http://127.0.0.1:5000", "http://localhost:5000" })
		foreach (var testUrl in new[] { "http://127.0.0.1:5000", /* "http://[::1]:5000" */})

Enable default server address test #1436

Enable default server address test #1436

Conversation

JunTaoLuo commented Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum commented Mar 3, 2017

halter73 commented Mar 3, 2017

JunTaoLuo commented Mar 3, 2017

halter73 commented Mar 4, 2017

JunTaoLuo commented Mar 4, 2017

JunTaoLuo commented Mar 4, 2017

Choose a reason for hiding this comment

cesarblum Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum left a comment

Choose a reason for hiding this comment

JunTaoLuo commented Mar 3, 2017 •

edited

Loading

cesarblum Mar 4, 2017 •

edited

Loading