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

Issue548: Exposing properties in the TestContext #4583

Merged
merged 10 commits into from
Jul 26, 2024
Merged

Issue548: Exposing properties in the TestContext #4583

merged 10 commits into from
Jul 26, 2024

Conversation

OsirisTerje
Copy link
Member

@OsirisTerje OsirisTerje commented Dec 7, 2023

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.

/// <summary>
/// Return all categories in the hierarchy flattened
/// </summary>
public IEnumerable<string> AllCategories()
Copy link
Member

@stevenaw stevenaw Dec 8, 2023

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

Copy link
Member

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

Copy link
Member

@manfred-brands manfred-brands left a 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.

src/NUnitFramework/framework/TestContext.cs Outdated Show resolved Hide resolved
src/NUnitFramework/framework/TestContext.cs Outdated Show resolved Hide resolved
src/NUnitFramework/framework/TestContext.cs Outdated Show resolved Hide resolved
src/NUnitFramework/framework/TestContext.cs Show resolved Hide resolved
src/NUnitFramework/framework/TestContext.cs Outdated Show resolved Hide resolved
src/NUnitFramework/framework/TestContext.cs Outdated Show resolved Hide resolved
@OsirisTerje OsirisTerje changed the title WIP: Issue548: Exposing properties in the TestContext Issue548: Exposing properties in the TestContext Jul 18, 2024
@OsirisTerje
Copy link
Member Author

OsirisTerje commented Jul 21, 2024

As @stevenaw pointed out above, the naming (or Grammar) here seems a bit off. And it bothers me.

The current Properties represents a PropertyBag for only the current Test Level. It will not show anything above this level.

The Test level is also different based on what type of test we have. For a simple test, it is the test method itself, but if we use a TestCase it is that particular testcase instance, and the test method is one level up. Since you can't directly add properties to a TestCase attribute, any property you add belongs to the test method, which is one level up.

Adding the Parent property allows it to move upwards and pick up those properties. This is the fundamental mechanism. As @manfred-brands pointed out in his comments, this can be used to work upwards until there is no more parents.

Earlier in comments for #548 it was discussed adding these properties to the same property bag. Since NUnit doesn't use these custom properties itself, this seems like a waste. So I went for adding extra methods to just walk this hierarchy and collect the properties in a new collection, of PropertyHierarchyItems, showing both which level they were found and their key/value(s).

I made one method PropertyHierchy - to just expose this hierarchy, for anyone who might like to look into this, or for debugging or whatever purpose they might have.

Then comes the challenges with naming for how to easily extract a property value set for a given key, over the whole hierarchy.

The grammar for the PropertyBag have a Get(string key) method for extracting a single item (first if multiple values), and a TryGet which extracts a list of values for a given key. First, this naming seems also odd to me. TryGet doesn't indicate to me that a list could be expected, more like it may fail (as other TryGet methods do). So this grammer seems old, but again, we should not change this.

The new methods for getting out the hierarchy values, one for properties and one for the special property named "Category", I just named them AllPropertyValues(string key) and AllCategories(). But I am not happy about these names.

  1. Should AllPropertyValues stay, or instead be:
    1.1 AllProperties
    1.2 PropertyHierarchy (thus causing the current (but new) PropertyHierarchy to change name to e.g. PropertyHierarchyItems)
    or any other good idea for these names .....

And similar for AllCategories, should it instead be CategoryHierachy ? Or....

@OsirisTerje OsirisTerje merged commit c4952ce into main Jul 26, 2024
5 checks passed
@OsirisTerje OsirisTerje deleted the Issue548 branch July 26, 2024 07:58
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