-
Notifications
You must be signed in to change notification settings - Fork 4.9k
First DirectoryServcies tests which runs against real server #24316
First DirectoryServcies tests which runs against real server #24316
Conversation
CC @AlexGhiondea @karelz @danmosemsft |
Looks msbuild complaining
I'll try to fix this one. |
Very cool. cc @hughbe |
return ouCoreDevs; | ||
} | ||
|
||
private DirectoryEntry CreateCN(DirectoryEntry ouEntry, string cn, string description, string phone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CN isn't really a "type" it stands for common name. I would change this name to match the object type that is actually getting created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix that
} | ||
} | ||
|
||
private void SearchCNByName(DirectoryEntry de, string cnName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback on naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix that
DirectoryEntry rootOU = CreateOU(de, "CoreFxRootOU", "CoreFx Root OU"); | ||
try | ||
{ | ||
DirectoryEntry child1OU = CreateOU(rootOU, "CoreFxChild1OU", "CoreFx Child OU 1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need using statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix that
|
||
CreateCN(child1OU, "user.ou1.1", "User 1 is in CoreFx ou 1", "1 111 111 11111"); | ||
CreateCN(child1OU, "user.ou1.2", "User 2 is in CoreFx ou 1", "1 222 222 22222"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we'll add some test logging in the future so it's clear which of these sub steps failed when the test fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XUnit will catch any thrown exception and have it in the logs.
|
||
private void SearchOUByName(DirectoryEntry de, string ouName) | ||
{ | ||
using (DirectorySearcher ds = new DirectorySearcher(de)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set ds.ClientTimeout to some reasonable value. I think the default is infinite wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix that. I'll use 2 minutes timeout which I think can be reasonable.
LdapConfiguration.Configuration.AuthenticationTypes)) | ||
{ | ||
string ouName = "NetCoreDevs"; | ||
CreateOU(de, ouName, ".Net Core Developers Unit"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should try and remove any pre-exisiting OUs with the target name first to take care of tests which leave the environment dirty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix that
DeleteDirectoryEntry(de, child); | ||
} | ||
|
||
parent.Children.Remove(de); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix that
} | ||
} | ||
|
||
private void SearchCNByName(DirectoryEntry de, string cnName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all of these test* methods, should we add a finally block and then assert if any exception is thrown? That way you know which action failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed. XUnit is catching the thrown exception and fail the test with the exception logs. we should be fine here.
internal string Port { get; set; } | ||
internal string Domain { get; set; } | ||
internal AuthenticationTypes AuthenticationTypes { get; set; } | ||
internal string LdapPath => String.IsNullOrEmpty(Port) ? $"LDAP://{ServerName}/{Domain}" : $"LDAP://{ServerName}:{Port}/{Domain}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This format is very specific to the System.DirectoryServices* assembly but doesn't apply to System.DirectoryServices.Protocols We may want to reuse this config class for all the S.DS.* assemblies to maybe this assembly specific logic should be moved to the assembly specific tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding tests to System.DirectoryServices.Protocols, we can look at what configuration is used there and then can update the configuration class to support it. or to use another specific configuration class.
Thanks @tquerec for your review |
…24316) * First DirectoryServcies tests which runs against real server * Fix msbuild failure with None element * Address the feedback * Fix some formatting
…corefx#24316) * First DirectoryServcies tests which runs against real server * Fix msbuild failure with None element * Address the feedback * Fix some formatting Commit migrated from dotnet/corefx@f4c2ba6
This is the first test collection which intended to run against real servers supports LDAP.
Read the comment in the file LDAP.Configuration.xml to know how can enable such tests. by default, these tests will be skipped till providing the real server info in the file LDAP.Configuration.xml.
These are just initial tests which we should add more as we go. It is recommended when adding any more tests to test it against AD servers (i.e. Windows Server) and against OpenDJ too to avoid the behavior differences.
One more note, I have written every test case as self-contained so every test case will exit in a clean state of the server.