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

Checking in windows desktop automated test wrapper and example projects #11

Merged
merged 3 commits into from Dec 21, 2016

Conversation

PandaMagnus
Copy link
Collaborator

No description provided.

Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, let me know if you have any questions on things.

{
public class CalculatorHelpers
{
readonly CalculatorWindow _CalculatorWindow = new CalculatorWindow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set protection modifier.

readonly CalculatorWindow _CalculatorWindow = new CalculatorWindow();
public bool VerifyResults(string expectedValue)
{
return expectedValue == _CalculatorWindow.FindResultsControl().Name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is possible for FindResultsControl() to return null, add null propagation operator before .Name

public class NotepadWindowHelpers
{
readonly NotepadWindow _NotepadWindow = new NotepadWindow();
readonly SaveAsWindow _SaveAsWindow = new SaveAsWindow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add protection modifiers

public void SaveDocument(string location, string filename)
{
//Check if doc with same name already exists in location before saving
string formattedPath = location + "\\" + filename;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better way to do this is

using System.IO;
...
string formattedPath = Path.Combine(location, filename);

Typically you want to avoid specifying the folder separator (and use Path.DirectorySeparator where you have to use it)

string formattedPath = location + "\\" + filename;
if (File.Exists(formattedPath))
{
filename += "1";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will functionally change everything after the file extension. Is this intentional?
Is something like this what you were hoping to achieve?

public static string GetUniqueFileName(string filePath)
{
    string uniqueFileName = filePath;
    for (int i = 0; File.Exists(uniqueFileName); uniqueFileName = AppendFileName(filePath, $" ({++i})"))
    { }
    return uniqueFileName;
}

public static string AppendFileName(string filePath, string toAppend)
{
    var dir = Path.GetDirectoryName(filePath);
    var fileName = Path.GetFileNameWithoutExtension(filePath);
    var extension = Path.GetExtension(filePath);
    return Path.Combine(dir ?? "", string.Concat(fileName, toAppend, extension));
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not intentional. It was an oversight on me trying to come up with something quick and not double checking. However, your comment has got me thinking that there is a better way in general to handling saving files for a test. I will do some poking around with this...

<ListBox ItemsSource="{Binding Path=MyList}" AutomationProperties.AutomationId="autoListBoxMyList">
<ListBox.ItemTemplate>
<DataTemplate>
<TextBlock>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you only have one pieces of text you can simplify this to be:

<TextBlock Text="{Binding Path=MyListItem}" />

return foundControl;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bunch of code duplication between these two cases. I think you want something like this:

protected UITestControl BuildControlHeirarchy(SearchTypes searchType, string baseControl, params string[] controlList)        
{ 
    if (searchType == null) 
        throw new ArgumentNullException(nameof(searchType)); 
    if (baseControl == null) 
        throw new ArgumentNullException(nameof(baseControl)); 
    UITestControl foundControl; 
    switch(searchType)
    {
        case SearchTypes.AutomationId:
            foundControl = FindControlImpl((controlName, rootControl) => FindWpfControlByAutomationId(controlName, c => new WpfControl(c), rootControl)); 
        case SearchTypes.ControlName:
            foundControl = FindControlImpl((controlName, rootControl) => FindControlByName(controlName, c => new UITestControl(c), rootControl));
        default:
            foundControl = new UITestControl();
    }
    return foundControl; 
}

private UITestControl FindControlImpl(Func<string, UITestControl, UITestControl> findMethod, string baseControl, string[] childControls)
{
    UITestControl foundControl = findMethod(baseControl, FindWinWindowUnderTest()); 
    if ( childControls?.Any() == true) 
    { 
        foreach (string control in controlList) 
        { 
            foundControl = findMethod(control, foundControl);
        } 
    } 
    return foundControl;
}

Few things to note:

  • foundControl starts as null and we only give it the default value in the unlikely case where we have not set it to anything else
  • Use a switch rather than a bunch of if statements when acting on an enum.
  • The common logic is abstracted into its own method
  • Added null propogation operator when checking childControls. Typically you don't need to check values that come from params as the compiler will put in an empty array unless the caller specifically passes null.
  • Zero testing of this code on my part, so there may be errors.

//Tip from Stack Overflow to remove and then re-add error handling to get it to initialize properly
//Don't know why this works, but it did when Mike Curn checked using VS2010.
Playback.PlaybackError -= PlaybackErrorHandler;
Playback.PlaybackError += PlaybackErrorHandler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a common pattern to keep from accidently duplicating event handlers. I suspect that was likely the cause.

public string ParseTextFile(string path)
{
string text = null;
int seconds = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to name this something like retryAttempts since it is not actually a measurement of seconds.

{
try
{
File.Delete(location + "\\" + filename);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Path.Combine here and you won't need to worry about if the location ends with a slash or not.

@Keboo
Copy link
Member

Keboo commented Dec 21, 2016

:shipit:

@PandaMagnus PandaMagnus merged commit 62ff08f into master Dec 21, 2016
@PandaMagnus PandaMagnus deleted the addDesktop branch December 21, 2016 20:04
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.

None yet

2 participants