-
Notifications
You must be signed in to change notification settings - Fork 723
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
WIP: Issue548: Exposing properties in the TestContext #4583
base: master
Are you sure you want to change the base?
Conversation
/// <summary> | ||
/// Return all categories in the hierarchy flattened | ||
/// </summary> | ||
public IEnumerable<string> AllCategories() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding and fixing some of these older related issues requesting usability improvements.
nit: Initially, looking just at the changes within the PR, the "All" here felt out of place as the other methods omit it. But I can also see it as helpful to identify when we traverse up the hierarchy - though we use "Hierarchy" in the other methods to indicate that.
I don't have a better suggestion, so I'm fine with the current naming too, but a passing thought I wanted to share in case it spurred ideas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps changing PropertyValues
to AllPropertyValues
could also work. Still no strong opinions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few questions about the usage especially of properties overriding or augmenting on different levels.
Suggested some optimizations.
/// </summary> | ||
public class PropertyHierachyItem | ||
{ | ||
public string Name { get; set; } = string.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want this to be changeable. I would prefer a constructor and read-only properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense
list.Add(o); | ||
} | ||
|
||
return list.Distinct(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how the properties are used in NUnit, but logically to me a lower hierarchy property would have preference over the same property defined in a parent.
If a fixture defines properties: A=1,2 and B=3,4
If a test defines properties: B=5.
Then I would PropertyValues on the test level to contain: A=1,2 and B=5. Not B=3,4,5.
But that could be because of my lack of understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me too!
/// Returns all properties in the hierarchy | ||
/// </summary> | ||
/// <returns></returns> | ||
public IDictionary<PropertyHierachyItem, IList> PropertyHierarchy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use case for this hierarchy other than the internal usages below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not even sure :-) It could perhaps be useful to someone, but it could also just be a private method.
var props = PropertyHierarchy(); | ||
foreach (var item in props.Keys.Where(o => o.Name == property)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expensive, first creating a dictionary of all properties, then filtering.
Depending on the outcome of my previous question of what the result should be.
It can be cheaper:
- Return value of test.Properties[property] if defined, otherwise return value of
test.Parent.Properties[property]
or null if no parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll have a look at that. I am not that concerned about efficiency here, as I expect this to be called rarely. But your suggestion makes sense, so perhaps the way to go.
var all = PropertyHierarchy(); | ||
foreach (var property in all.Where(o => o.Key.Name == "Category")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful to pass the name to PropertyHierarchy to only include the properties wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Co-authored-by: Manfred Brands <manfred-brands@users.noreply.github.com>
Ref #548 #796 #1358
Exposing properties, but trying to avoid changing existing names, so have added new names that need a discussion.
The hierarchies are implemented as methods to make clear they do processing.
The Parent property can be useful in more scenarios.
Will hold this as WIP until the names and approach are clarified.
[] Need to add tests