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

Dynamic data display name #373

Merged
merged 8 commits into from
Apr 12, 2018

Conversation

bstoney
Copy link
Contributor

@bstoney bstoney commented Feb 21, 2018

Fixes #351
Added dynamic display name to DynamicDataAttribute

@msftclas
Copy link

msftclas commented Feb 21, 2018

CLA assistant check
All CLA requirements met.

@@ -81,5 +81,20 @@ public void GetDisplayNameShouldReturnAppropriateName()
displayName = dataRowAttribute.GetDisplayName(this.testMethodInfo, data2);
Assert.AreEqual("DataRowTestMethod (First,,Second)", displayName);
}

[TestMethod]
public void GetDisplayNameShouldReturnAppropriateNameWithDataRowDisplayName()
Copy link
Member

Choose a reason for hiding this comment

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

Please consider changing testmethod name to GetDisplayNameShouldReturnProvidedDisplayName() or GetDisplayNameShouldReturnSpecifiedDisplayName()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// <summary>
/// Gets or sets the name of method used to customize the display name in test results.
/// </summary>
public string DynamicDisplayName { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming this to "DynamicDataDisplayName"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// <summary>
/// Gets or sets the declaring type used to customize the display name in test results.
/// </summary>
public Type DynamicDisplayNameDeclaringType { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Same here. "DynamicDataDisplayNameDeclaringType"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -60,7 +60,7 @@ public void GetDataShouldReadDataFromProperty()
public void GetDataShouldReadDataFromPropertyInDifferntClass()
{
var methodInfo = this.dummyTestClass.GetType().GetTypeInfo().GetDeclaredMethod("TestMethod1");
this.dynamicDataAttribute = new DynamicDataAttribute("ReusableTestDataProperty", typeof(DummyTestClass));
this.dynamicDataAttribute = new DynamicDataAttribute("ReusableTestDataProperty2", typeof(DummyTestClass2));
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers 😄

}

[TestFrameworkV1.TestMethod]
public void GetDisplayNameShouldThrowExceptionIfMethodDoesNotMatchExpectedSignature()
Copy link
Member

Choose a reason for hiding this comment

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

Please add Tests for cases like :

  1. Method not existing i.e InvalidCustomDynamicDataDisplayName is not present in class
  2. Method has wrong signature i.e. when first parameter doesn't match, second parameter doesn't match, return type doesn't match. Write tests for each of them separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also picked up a bug where I wasn't checking that the method was static.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome 👍

@@ -283,4 +283,7 @@ Stack Trace: {4}</value>
<data name="DynamicDataValueNull" xml:space="preserve">
<value>Value returned by property or method {0} shouldn't be null.</value>
</data>
<data name="DynamicDataDisplayName" xml:space="preserve">
Copy link
Member

@jayaranigarg jayaranigarg Mar 29, 2018

Choose a reason for hiding this comment

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

Also please run "build.cmd -uxlf" so the string addition is localizable. You would have to commit the xlf files changes post running the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran this and have committed the xlf files. I'm assuming someone or some magic will do the translation?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. We just needed modified xlf files. We will take care of the translation 😄

@jayaranigarg
Copy link
Member

jayaranigarg commented Mar 29, 2018

@bstoney : Apologies for the delay.
Thank you for your contribution..!! PR looks good overall. Kindly do the needful changes so that we can push this in.

@jayaranigarg
Copy link
Member

@bstoney : Can you make the necessary changes here please?

throw new ArgumentNullException(
string.Format(
FrameworkMessages.DynamicDataDisplayName,
this.DynamicDataDisplayName,
Copy link
Member

@jayaranigarg jayaranigarg Apr 9, 2018

Choose a reason for hiding this comment

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

@bstoney : Method signature should also have "public" access modifier specified. Can you please add that as well??

Copy link
Member

Choose a reason for hiding this comment

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

You will have to update xlf files as well if you modify FrameworkMessages.DynamicDataDisplayName string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Interestingly it did work with a private static method even though the documentation for GetDeclaredMethod states that "Returns an object that represents the specified public method declared by the current type." I have updated the exception message and added a test for public accessibility.

Copy link
Member

Choose a reason for hiding this comment

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

Super 👍

@smadala
Copy link
Contributor

smadala commented Apr 10, 2018

@bstoney Thanks for contribution 👍 . Can you please update documentation as well here ? then we can merge this PR.

@jayaranigarg jayaranigarg merged commit bad3e20 into microsoft:master Apr 12, 2018
@bstoney bstoney deleted the dynamic-display-name branch April 12, 2018 21:47
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.

4 participants