Re-enable hostname resolution test on Linux, but skip under certain conditions by rmkerr · Pull Request #28324 · dotnet/corefx

rmkerr · 2018-03-21T18:14:58Z

This test assumed that the host name can always be resolved, which isn't guaranteed on Unix.

In the interest of "testing as much as we can" I'm going to enable the test, but pass it if we get the specific error that indicates we haven't been able to resolve the localhost on Unix. That way we'll hopefully still see failures if things go wrong somewhere else.

The logic to skip the test is taken from another test in the same file:

corefx/src/System.Net.NameResolution/tests/PalTests/NameResolutionPalTests.cs

Lines 89 to 104 in 643bccc

    
           [Fact] 
        
           public void TryGetAddrInfo_HostName_TryGetNameInfo() 
        
           { 
        
               string hostName = NameResolutionPal.GetHostName(); 
        
               Assert.NotNull(hostName); 
        
               IPHostEntry hostEntry; 
        
               int nativeErrorCode; 
        
               SocketError error = NameResolutionPal.TryGetAddrInfo(hostName, out hostEntry, out nativeErrorCode); 
        
               if (error == SocketError.HostNotFound) 
        
               { 
        
                   // On Unix, getaddrinfo returns host not found, if all the machine discovery settings on the local network 
        
                   // is turned off. Hence dns lookup for it's own hostname fails. 
        
                   Assert.True(RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)); 
        
                   return; 
        
               }

Fixes: #20245

caesar-chen

LGTM.

karelz · 2018-03-21T18:22:43Z

src/System.Net.NameResolution/tests/PalTests/NameResolutionPalTests.cs

+            {
+                // On Unix, we are not guaranteed to be able to resove the local host. The ability to do so depends on the 
+                // machine configurations, which varies by distro and is often inconsistent.
+                Assert.True(RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX));


Not: I wonder if it would be "better" to move OS check into the if-condition. If we ever get HostNotFound on Windows, it should ideally fail on the Assert.Equal(SocketError.Succes, error); line.
If this is established pattern in other tests, I'm fine leaving it as is.

I would rather see the OS checks in the if-condition. This is usually how we do OS platform-specific tests using 'ifs'. It makes the code easier to read if we see things like this:

if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { // Test setup and assertions specific to Linux } else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { // Test setup and assertions specific to OSX } // ...

This is really not specific to OS but given environment. Linux can resolve the names if configured as such. Add it will in most cases.

Do you think it's better to do things this way, or to set up a capability as mentioned by @wfurt below?

Capabilities are not used for this kind of thing. This is basically a platform difference in behavior that you want to capture in a test.

Made this change. I agree that it's an improvement over the check I initially added -- it should make failures a lot more clear.

wfurt · 2018-03-21T18:23:31Z

Can we turn this into test Capability?
I think there were more tests making similar assumptions.
That would also allow us to skip and track that instead of instant pass.

karelz · 2018-03-21T19:08:34Z

Can we turn this into test Capability?

Long-term, why not if it is possible. Short-term, let's just disable the test on Linux / skip the testing as @rmkerr proposes.

rmkerr · 2018-03-22T16:38:09Z

I can't see why the Linux build failed, so I'm rerunning it. The Windows failures are unrelated, but I'm rerunning them anyways since I have the chance.

@dotnet-bot test Linux x64 Release Build
@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build
@dotnet-bot test Windows x64 Release Build

…onditions (dotnet/corefx#28324) This test assumed that the host name can always be resolved, which isn't guaranteed on Unix. In the interest of "testing as much as we can" this change enables the test, but passes it if we get the specific error that indicates we haven't been able to resolve the localhost on Unix. That way we'll hopefully still see failures if things go wrong somewhere else. Fixes: dotnet/corefx#20245 Commit migrated from dotnet/corefx@69ba7ef

Max Kerr added 4 commits March 21, 2018 17:34

Pass test on Linux if specific failure mode is detected.

3c2abe1

Add second failing test.

19f5332

Revert misplaced check.

bfc412c

Whitespace.

fad6537

rmkerr added test bug Problem in test source code (most likely) area-System.Net labels Mar 21, 2018

rmkerr self-assigned this Mar 21, 2018

rmkerr requested review from a team and danmoseley March 21, 2018 18:14

caesar-chen approved these changes Mar 21, 2018

View reviewed changes

karelz approved these changes Mar 21, 2018

View reviewed changes

Move platform check.

03eb9e7

karelz approved these changes Mar 21, 2018

View reviewed changes

Add bracket I missed when copying check...

9aee411

davidsh approved these changes Mar 22, 2018

View reviewed changes

rmkerr merged commit 69ba7ef into dotnet:master Mar 22, 2018

karelz added this to the 2.1.0 milestone Mar 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable hostname resolution test on Linux, but skip under certain conditions#28324

Re-enable hostname resolution test on Linux, but skip under certain conditions#28324
rmkerr merged 6 commits intodotnet:masterfrom
rmkerr:NameResolution_UnixHostname_DoesNotResolve

rmkerr commented Mar 21, 2018 •

edited

Loading

Uh oh!

caesar-chen left a comment

Uh oh!

karelz Mar 21, 2018

Uh oh!

davidsh Mar 21, 2018 •

edited

Loading

Uh oh!

wfurt Mar 21, 2018

Uh oh!

rmkerr Mar 21, 2018

Uh oh!

davidsh Mar 21, 2018

Uh oh!

rmkerr Mar 21, 2018

Uh oh!

wfurt commented Mar 21, 2018

Uh oh!

karelz commented Mar 21, 2018

Uh oh!

rmkerr commented Mar 22, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	[Fact]
	public void TryGetAddrInfo_HostName_TryGetNameInfo()
	{
	string hostName = NameResolutionPal.GetHostName();
	Assert.NotNull(hostName);

	IPHostEntry hostEntry;
	int nativeErrorCode;
	SocketError error = NameResolutionPal.TryGetAddrInfo(hostName, out hostEntry, out nativeErrorCode);
	if (error == SocketError.HostNotFound)
	{
	// On Unix, getaddrinfo returns host not found, if all the machine discovery settings on the local network
	// is turned off. Hence dns lookup for it's own hostname fails.
	Assert.True(RuntimeInformation.IsOSPlatform(OSPlatform.Linux) \|\| RuntimeInformation.IsOSPlatform(OSPlatform.OSX));
	return;
	}

Conversation

rmkerr commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caesar-chen left a comment

Choose a reason for hiding this comment

Uh oh!

karelz Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

davidsh Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

rmkerr Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

davidsh Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

rmkerr Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

wfurt commented Mar 21, 2018

Uh oh!

karelz commented Mar 21, 2018

Uh oh!

rmkerr commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rmkerr commented Mar 21, 2018 •

edited

Loading

davidsh Mar 21, 2018 •

edited

Loading

rmkerr commented Mar 22, 2018 •

edited

Loading