Skip to content

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.

}

[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

{
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 👍

<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.

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