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

Added constants for hierarchical naming #2724

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

Haplois
Copy link
Contributor

@Haplois Haplois commented Feb 1, 2021

  • Added constants for hierarchical naming.
  • Reduced dependency surface of the AdapterUtilities package.

@Haplois
Copy link
Contributor Author

Haplois commented Feb 1, 2021

We're adding a TestProperty to store multiple levels of a name. I am calling it HierarchicalName. Any recommendations about the naming?

HierarchicalName property is an array, which holds these for a TestCase:

  • Namespace
  • Class
  • TestGroup
  • DisplayName
  • Theory / DataRow

@Haplois Haplois force-pushed the AdapterUtilities-refactoring branch from 4404196 to fb4c26e Compare February 1, 2021 15:03
@Haplois Haplois self-assigned this Feb 1, 2021
@peterwald
Copy link
Member

peterwald commented Feb 1, 2021

HierarchicalName property is an array, which holds these for a TestCase:

  • Namespace
  • Class
  • TestGroup
  • DisplayName
  • Theory / DataRow

I'm ok with the name HierarchicalName or HierarchyName. We shouldn't need an entry for Theory/DataRow. That is captured by TestGroup & DisplayName.

In the case of a "Fact" test, you would have a Namespace/Class/(blank)/DisplayName and in the case of a "Theory" test you would have a Namespace/Class/TestGroup/DisplayName where TestGroup captures the "test method" level and DisplayName would be different for each data driven test case. The way we handle this today in the test hierarchy is if there is only one DisplayName in a TestGroup, then the TestGroup level is removed.

@peterwald
Copy link
Member

What about using just the name Hierarchy for the property.

@Haplois
Copy link
Contributor Author

Haplois commented Feb 1, 2021

What about using just the name Hierarchy for the property.

Sounds better.

@Haplois Haplois force-pushed the AdapterUtilities-refactoring branch from fb4c26e to 6572ef1 Compare February 1, 2021 15:14
@Haplois Haplois requested a review from peterwald February 1, 2021 15:14
@Haplois Haplois force-pushed the AdapterUtilities-refactoring branch from 6572ef1 to 31b4729 Compare February 1, 2021 15:22
Copy link
Member

@peterwald peterwald left a comment

Choose a reason for hiding this comment

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

Looks good with a few minor changes.

/// <summary>
/// Meanings of the indices in the Hierarchy array.
/// </summary>
public static class Levels
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the HierarchyName property be a dictionary instead, so both setting/retrieving is easier to understand and uniqueness of values is guaranteed? We could serialize/deserialise this the way we want internally,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean like a <string, string> directory with set keys on the adapter?

Copy link
Contributor

@AbhitejJohn AbhitejJohn Apr 7, 2021

Choose a reason for hiding this comment

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

Gosh, I dont have a good way of responding to notifications from github. Yes, I meant a <string,string> property bag.
/cc : @Haplois

 - Added constants for hierarchical naming.
 - Reduced dependency surface of the AdapterUtilities package.
@Haplois Haplois force-pushed the AdapterUtilities-refactoring branch from 31b4729 to 5091dd2 Compare February 1, 2021 15:42
@Haplois Haplois requested a review from AbhitejJohn February 1, 2021 16:08
@Haplois Haplois merged commit 54850df into microsoft:master Feb 1, 2021
@Haplois Haplois deleted the AdapterUtilities-refactoring branch February 1, 2021 16:57
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.

3 participants