Design/Pattern guidance to refector my current design for unit testing

S

sklett

Hello all-

I would like to start unit testing my projects, I'm the sole developer in a
startup with stacks of projects that needed to be done yesterday (nice sob
story, I know) - point is I keep breaking things! Moving too fast to get a
new feature out and I'm breaking stuff left and right. My first step to
remedy this problem is to implement unit tests for my projects. I'm
learning that is easier said than done.

The first project I decided to attempt is an authentication and
authorization module (class library) that is consumed by 5+ different
WinForms apps. When I originally designed the module, ease of
implementation was high on the list considering I had over 5 projects that
needed to use it. The result design is a single class called AuthMgr that
implements an IAuthMgr interface. Here is the interface:

<code>
public interface IAuthMngr
{
LoginResponse Login();
LoginResponse AuthenticateUser(string username, string rawPassword, out
UserAccount account);
LoginResponse AuthenticateUser(string username, string rawPassword,
short requiredRoleID, out UserAccount account);
PasswordChangeResponse ChangePassword();
void ResetPassword(UserAccount account);
UserAccountCollection GetAccounts();
UserRoleCollection GetRoles();
UserRoleCollection GetAccountRoles(int accountID);
void ManageAccounts();
bool ValidateUserPassword(string rawPassword);
bool ValidateUserPassword(UserAccount account, string rawPassword);
bool MonitorSessionTimeout { get; set; }
int SessionTimeoutDuration { get; set; }
IPrincipal Principle { get; }
IIdentity Identity { get; }
}
</code>

I haven't gone through a cleanup pass yet, there is some unused stuff in
there.

I will explain an aspect of the module; Login()

** Login **
- When the application consuming AuthMgr starts up, I instantiate a new
instance of AuthMgr and make a call to Login().
- Login() displays a Form (LoginDlg) to collection Username, password,
location, etc.
- Because I want to present any errors or warnings to the user in the
context of the Login Dialog, I have LoginDlg handle the validation of the
data entered (required fields) and also handle business rules (Password
expired = show ChangePasswordDlg, Account disabled = show message box
alerting user account is disabled, etc)
- LoginDlg takes a reference to the AuthMgr instance in it's constructor.
It uses this reference to make calls to AuthMgr.AuthenticateUser(), etc.

Maybe some code would explain things better...
<code>
// AuthMgr
public LoginResponse Login()
{
// null out the current authenticated user
_authenticatedUser = null;

// Show the login form
Views.LoginDlg loginView = new Views.LoginDlg(this);
loginView.StartPosition = FormStartPosition.CenterScreen;

if(loginView.ShowDialog() == DialogResult.Cancel)
{
_exitHandler();

return LoginResponse.UnspecifiedFailure;
}

_authenticatedUser = loginView.Account;
Guard.ReferenceNotNull(_authenticatedUser,
"successful login should yield valid UserAccount reference");

// Add the login to the audit trail
AddLoginToAuditTrail(_authenticatedUser.Id.Value,
Environment.MachineName);

// Start the activity monitor
if(_monitorSessionActivity)
{
#if (!SKIP_GLOBAL_HOOKS)
// Global hooks cause PAIN when breaking into the debugger,
// unless you REALLY need to debug the hook code (and related)
// functionality I would leave this option off while
// debugging.
StartMonitoringSessionActivity();
#endif
}

return LoginResponse.Success;
}

// LoginDlg
private void button1_Click(object sender, EventArgs e)
{
if(string.IsNullOrEmpty(textBox_Username.Text))
{
// Show error message
base.ShowMissingDataMessage("Username");
}

if(string.IsNullOrEmpty(textBox_Password.Text))
{
// Show error message
base.ShowMissingDataMessage("Password");
}

LoginResponse authenticationResponse = _authMgr.AuthenticateUser(
textBox_Username.Text,
textBox_Password.Text,
out _account);

// Check if the account password has expired
if(_account != null && _account.PasswordExpirationDate < DateTime.Now)
{
base.ShowGeneralErrorMessage("Your password has expired, you will be
prompted to create a new one");
PasswordChangeResponse changeResponse =
_authMgr.ChangePassword(_account);
return;
}

switch(authenticationResponse)
{
case LoginResponse.InvalidPassword:

textBox_Password.Clear();
textBox_Password.Focus();

// If this user hasn't been added to the failed attempts
// dictionary, add them now
if(_failedAttempts.ContainsKey(textBox_Username.Text) == false)
{
_failedAttempts.Add(textBox_Username.Text, 0);
}

// increment the failed attempts count for this username
_failedAttempts[textBox_Username.Text]++;

// Show the invalid password message
base.ShowInvalidPasswordMessage(_failedAttempts[textBox_Username.Text]);

// If the user has exceeded the number of failed attempts,
// lock their account and show the message
if (_failedAttempts[textBox_Username.Text] >=
Settings.Default.MaxFailedLoginAttempts)
{
_authMgr.LockoutAccount(_account);
base.ShowAccountLockedOutMessage(_account);
}
break;
case LoginResponse.PasswordExpired:
{
base.ShowGeneralErrorMessage("Your password has expired, you
will be prompted to create a new one");

PasswordChangeResponse changeResponse =
_authMgr.ChangePassword(_account);
if (changeResponse == PasswordChangeResponse.Success)
{
this.DialogResult = DialogResult.OK;
}
break;
}
case LoginResponse.Success:
this.DialogResult = DialogResult.OK;
break;

// Additional cases removed to keep things reasonably small...
}
}
</code>


Hopefully that will give you a general idea of how I've structured things.
I'm starting to think that I might have a bad design on my hands, I will let
you all judge that ;0)

Now, finally to my problem with Unit Testing. As you can imagine when I'm
showing these dialogs it will break my unit tests because the modal dialogs
will block indefinitly. After speaking with a friend that has experience
with unit testing (I gave him a much simpler example) he suggested that I
separate my concerns. For example, I would have something like this:
<pseudo code>
interface ICredentialsCollector
{
string Username {get;}
string Password {get;}
bool GetCredentials();
}

LoginDialog : ICredentialsCollector
{
// IGetCredentials members
}

class Authenticate
{
private ICredentialsCollector _collector = null;

public Authenticate(ICredentialsCollector collector){ _collector =
collector);

bool AuthenticateUser()
{
if(_collector.GetCredentials())
{
// do stuff with the username and password
return true;
}

return false;
}
}

// In my application I would do this
ICredentialsCollector collector = new LoginDialog();
Authenticate auth = new Authenticate(collector);
auth.AuthenticateUser();

// This would show the dialog, etc, etc

// in my unit test I would do something like this
class MockLoginDialog : ICredentialsCollector
{
public bool GetCredentials()
{
_username = "username to test with";
_password = "password to test with";
return true;
}
}

ICredentialsCollector collector = new MockLoginDialog();
Authenticate auth = new Authenticate(collector);
auth.AuthenticateUser();
<pseudo code>

What the above example achieves is the ability to either have my
Authenticate class show a dialog or use a mock object for testing (or some
other concrete class to obtain user credentials)

It also is completely incompatible with my current design. Furthermore, if
I were to extrapolate the above "mini-design" to my entire module I would
need to instantiate all of my dialogs (or mocks) up front, supply the
AuthMgr class with a reference to each, etc. This would result in my
original 2 lines of code being needed on the consuming application to many
lines of code and it wouldn't make sense to anyone. The comment would be
something like:
// This stuff is all here so I can unit test the authentication system

His suggestion makes sense to me, I can already imagine many other areas
where this pattern would make a lot of my code cleaner. I should have given
him the FULL requirement like I did here, I just didn't anticipate all the
details.

I'm not exactly proud of my original design either, so I am very open to
finding some middle ground.

What I want is a module with a very limited public interface that will
implement and manage complex operations involving multiple Forms,
MessageBoxs, etc that can also be unit tested following the execution path
of the public interface as closely as possible.

I can imagine a design where AuthMgr is really just a container for many
separate classes, each responsible for different operations and each taking
dialog interfaces in their constructors. AuthMgr would instantiate these
components and execute them in the correct order.. sort of a "macro". My
unit test would test these individual components on there own, building mock
dialog objects, etc. So while this design would not allow me to unit test
AuthMgr.Login(), it would allow me to test all the pieces that
AuthMgr.Login() is composed of. This is still not very good though,
AuthMgr.Login() would require logic to handle error conditions and business
rules, this logic would not be under test with this design.

Wow, this is a long thread. Apologies. I really hope someone out there
understands what I'm after and picks this thread up, I would really
appreciate the help.

Thanks for reading,
Steve
 
G

Guest

Hello steve,

I read all the article / question you wrote and i think you are right,
When is started to read the quiz I encounter with the first problem, the
interface
IAuthMngr is not bad, contains more then few method (rule of thumb 5~ of
method), in case the interface is long then it should divide to subset
interfaces.
So, think on it.

Second.
MVC, model view controller pattern, implement it.
In case you have n dialog, Each will have its own controller, in case you
need a combination between the dialog, a controller will manage/handle this,
the data (working with security login DB - windows security - NTLM), this
will be handled by the model object(s).
Using the mvc, each will handled the black box of its own, other will
communicate between the black box, so, the unit text (i.e. nunit) will handle
each black box separately and together.
Design and order of the unit test (check each element and segment) will
improve the throughout and the code.

Good luck

Sincerely
Yaron Karni
http://dotnetbible.blogspot.com/


sklett said:
Hello all-

I would like to start unit testing my projects, I'm the sole developer in a
startup with stacks of projects that needed to be done yesterday (nice sob
story, I know) - point is I keep breaking things! Moving too fast to get a
new feature out and I'm breaking stuff left and right. My first step to
remedy this problem is to implement unit tests for my projects. I'm
learning that is easier said than done.

The first project I decided to attempt is an authentication and
authorization module (class library) that is consumed by 5+ different
WinForms apps. When I originally designed the module, ease of
implementation was high on the list considering I had over 5 projects that
needed to use it. The result design is a single class called AuthMgr that
implements an IAuthMgr interface. Here is the interface:

<code>
public interface IAuthMngr
{
LoginResponse Login();
LoginResponse AuthenticateUser(string username, string rawPassword, out
UserAccount account);
LoginResponse AuthenticateUser(string username, string rawPassword,
short requiredRoleID, out UserAccount account);
PasswordChangeResponse ChangePassword();
void ResetPassword(UserAccount account);
UserAccountCollection GetAccounts();
UserRoleCollection GetRoles();
UserRoleCollection GetAccountRoles(int accountID);
void ManageAccounts();
bool ValidateUserPassword(string rawPassword);
bool ValidateUserPassword(UserAccount account, string rawPassword);
bool MonitorSessionTimeout { get; set; }
int SessionTimeoutDuration { get; set; }
IPrincipal Principle { get; }
IIdentity Identity { get; }
}
</code>

I haven't gone through a cleanup pass yet, there is some unused stuff in
there.

I will explain an aspect of the module; Login()

** Login **
- When the application consuming AuthMgr starts up, I instantiate a new
instance of AuthMgr and make a call to Login().
- Login() displays a Form (LoginDlg) to collection Username, password,
location, etc.
- Because I want to present any errors or warnings to the user in the
context of the Login Dialog, I have LoginDlg handle the validation of the
data entered (required fields) and also handle business rules (Password
expired = show ChangePasswordDlg, Account disabled = show message box
alerting user account is disabled, etc)
- LoginDlg takes a reference to the AuthMgr instance in it's constructor.
It uses this reference to make calls to AuthMgr.AuthenticateUser(), etc.

Maybe some code would explain things better...
<code>
// AuthMgr
public LoginResponse Login()
{
// null out the current authenticated user
_authenticatedUser = null;

// Show the login form
Views.LoginDlg loginView = new Views.LoginDlg(this);
loginView.StartPosition = FormStartPosition.CenterScreen;

if(loginView.ShowDialog() == DialogResult.Cancel)
{
_exitHandler();

return LoginResponse.UnspecifiedFailure;
}

_authenticatedUser = loginView.Account;
Guard.ReferenceNotNull(_authenticatedUser,
"successful login should yield valid UserAccount reference");

// Add the login to the audit trail
AddLoginToAuditTrail(_authenticatedUser.Id.Value,
Environment.MachineName);

// Start the activity monitor
if(_monitorSessionActivity)
{
#if (!SKIP_GLOBAL_HOOKS)
// Global hooks cause PAIN when breaking into the debugger,
// unless you REALLY need to debug the hook code (and related)
// functionality I would leave this option off while
// debugging.
StartMonitoringSessionActivity();
#endif
}

return LoginResponse.Success;
}

// LoginDlg
private void button1_Click(object sender, EventArgs e)
{
if(string.IsNullOrEmpty(textBox_Username.Text))
{
// Show error message
base.ShowMissingDataMessage("Username");
}

if(string.IsNullOrEmpty(textBox_Password.Text))
{
// Show error message
base.ShowMissingDataMessage("Password");
}

LoginResponse authenticationResponse = _authMgr.AuthenticateUser(
textBox_Username.Text,
textBox_Password.Text,
out _account);

// Check if the account password has expired
if(_account != null && _account.PasswordExpirationDate < DateTime.Now)
{
base.ShowGeneralErrorMessage("Your password has expired, you will be
prompted to create a new one");
PasswordChangeResponse changeResponse =
_authMgr.ChangePassword(_account);
return;
}

switch(authenticationResponse)
{
case LoginResponse.InvalidPassword:

textBox_Password.Clear();
textBox_Password.Focus();

// If this user hasn't been added to the failed attempts
// dictionary, add them now
if(_failedAttempts.ContainsKey(textBox_Username.Text) == false)
{
_failedAttempts.Add(textBox_Username.Text, 0);
}

// increment the failed attempts count for this username
_failedAttempts[textBox_Username.Text]++;

// Show the invalid password message
base.ShowInvalidPasswordMessage(_failedAttempts[textBox_Username.Text]);

// If the user has exceeded the number of failed attempts,
// lock their account and show the message
if (_failedAttempts[textBox_Username.Text] >=
Settings.Default.MaxFailedLoginAttempts)
{
_authMgr.LockoutAccount(_account);
base.ShowAccountLockedOutMessage(_account);
}
break;
case LoginResponse.PasswordExpired:
{
base.ShowGeneralErrorMessage("Your password has expired, you
will be prompted to create a new one");

PasswordChangeResponse changeResponse =
_authMgr.ChangePassword(_account);
if (changeResponse == PasswordChangeResponse.Success)
{
this.DialogResult = DialogResult.OK;
}
break;
}
case LoginResponse.Success:
this.DialogResult = DialogResult.OK;
break;

// Additional cases removed to keep things reasonably small...
}
}
</code>


Hopefully that will give you a general idea of how I've structured things.
I'm starting to think that I might have a bad design on my hands, I will let
you all judge that ;0)

Now, finally to my problem with Unit Testing. As you can imagine when I'm
showing these dialogs it will break my unit tests because the modal dialogs
will block indefinitly. After speaking with a friend that has experience
with unit testing (I gave him a much simpler example) he suggested that I
separate my concerns. For example, I would have something like this:
<pseudo code>
interface ICredentialsCollector
{
string Username {get;}
string Password {get;}
bool GetCredentials();
}

LoginDialog : ICredentialsCollector
{
// IGetCredentials members
}

class Authenticate
{
private ICredentialsCollector _collector = null;

public Authenticate(ICredentialsCollector collector){ _collector =
collector);

bool AuthenticateUser()
{
if(_collector.GetCredentials())
{
// do stuff with the username and password
return true;
}

return false;
}
}

// In my application I would do this
ICredentialsCollector collector = new LoginDialog();
Authenticate auth = new Authenticate(collector);
auth.AuthenticateUser();

// This would show the dialog, etc, etc

// in my unit test I would do something like this
class MockLoginDialog : ICredentialsCollector
{
public bool GetCredentials()
{
_username = "username to test with";
_password = "password to test with";
return true;
}
}

ICredentialsCollector collector = new MockLoginDialog();
Authenticate auth = new Authenticate(collector);
auth.AuthenticateUser();
<pseudo code>

What the above example achieves is the ability to either have my
Authenticate class show a dialog or use a mock object for testing (or some
other concrete class to obtain user credentials)

It also is completely incompatible with my current design. Furthermore, if
I were to extrapolate the above "mini-design" to my entire module I would
need to instantiate all of my dialogs (or mocks) up front, supply the
AuthMgr class with a reference to each, etc. This would result in my
original 2 lines of code being needed on the consuming application to many
lines of code and it wouldn't make sense to anyone. The comment would be
something like:
// This stuff is all here so I can unit test the authentication system

His suggestion makes sense to me, I can already imagine many other areas
where this pattern would make a lot of my code cleaner. I should have given
him the FULL requirement like I did here, I just didn't anticipate all the
details.

I'm not exactly proud of my original design either, so I am very open to
finding some middle ground.

What I want is a module with a very limited public interface that will
implement and manage complex operations involving multiple Forms,
MessageBoxs, etc that can also be unit tested following the execution path
of the public interface as closely as possible.

I can imagine a design where AuthMgr is really just a container for many
separate classes, each responsible for different operations and each taking
dialog interfaces in their constructors. AuthMgr would instantiate these
components and execute them in the correct order.. sort of a "macro". My
unit test would test these individual components on there own, building mock
dialog objects, etc. So while this design would not allow me to unit test
AuthMgr.Login(), it would allow me to test all the pieces that
AuthMgr.Login() is composed of. This is still not very good though,
AuthMgr.Login() would require logic to handle error conditions and business
rules, this logic would not be under test with this design.

Wow, this is a long thread. Apologies. I really hope someone out there
understands what I'm after and picks this thread up, I would really
appreciate the help.

Thanks for reading,
Steve
 
Top