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

WIP: Issue548: Exposing properties in the TestContext #4583

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
100 changes: 98 additions & 2 deletions src/NUnitFramework/framework/TestContext.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt

using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading;
using NUnit.Framework.Constraints;
Expand Down Expand Up @@ -476,16 +478,110 @@
/// <summary>
/// The expected result if there is one for the test
/// </summary>
public object? ExpectedResult
public object? ExpectedResult => (_test as TestMethod)?.ExpectedResult;

/// <summary>
/// The parent of this test or suite
/// </summary>
public ITest? Parent => _test.Parent;

/// <summary>
/// Returns all properties in the hierarchy
/// </summary>
/// <returns></returns>
public IDictionary<PropertyHierachyItem, IList> PropertyHierarchy()
Copy link
Member

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?

Copy link
Member Author

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 dict = new Dictionary<PropertyHierachyItem, IList>();
ITest? test = _test;
do
{
foreach (var property in test.Properties.Keys)
{
var values = test.Properties[property];
dict.Add(
new PropertyHierachyItem { Name = property, Level = test.Name ?? string.Empty }, values);
}
test = test.Parent;
}
while (test is object);
OsirisTerje marked this conversation as resolved.
Show resolved Hide resolved

return dict;
}

/// <summary>
/// Returns all values of a given property
/// </summary>
/// <param name="property">Name of property</param>
public IEnumerable<object> PropertyValues(string property)
{
var list = new List<object>();
var props = PropertyHierarchy();
foreach (var item in props.Keys.Where(o => o.Name == property))
Comment on lines +518 to +519
Copy link
Member

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.

Copy link
Member Author

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 values = props[item];
foreach (var o in values)
list.Add(o);
}

return list.Distinct();
Copy link
Member

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.

Copy link
Member Author

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!

}

Check failure on line 528 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Check failure on line 528 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Check failure on line 528 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Check failure on line 528 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Check failure on line 528 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Check failure on line 528 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build



/// <summary>
/// Returns all categories in the hierarchy
/// </summary>
/// <returns></returns>
public IDictionary<string, IList<string>> CategoryHierarchy()
{
get { return (_test as TestMethod)?.ExpectedResult; }
var dict = new Dictionary<string, IList<string>>();
var all = PropertyHierarchy();
foreach (var property in all.Where(o => o.Key.Name == "Category"))
Comment on lines +538 to +539
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

{
var values = new List<string>();
foreach (var item in property.Value)
{
string s = item?.ToString() ?? string.Empty;
values.Add(s);
}
dict.Add(property.Key.Level, values);
}
return dict;
}
/// <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

{
var cats = new List<string>();
var lists = CategoryHierarchy().Values;
foreach (var list in lists)
{
cats.AddRange(list);
}

return cats.Distinct();
}

#endregion
}

#endregion

#region PropertyHierachyItem
/// <summary>
/// Represents properties at different test levels
/// </summary>
public class PropertyHierachyItem
{
public string Name { get; set; } = string.Empty;

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'

Check failure on line 577 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Name'
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense

public string Level { get; set; } = string.Empty;

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'

Check failure on line 578 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Missing XML comment for publicly visible type or member 'TestContext.PropertyHierachyItem.Level'
}

#endregion

Check failure on line 582 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Check failure on line 582 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / MacOS Build

Check failure on line 582 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Check failure on line 582 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Linux Build

Check failure on line 582 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build

Check failure on line 582 in src/NUnitFramework/framework/TestContext.cs

View workflow job for this annotation

GitHub Actions / Windows Build



#region Nested ResultAdapter Class

/// <summary>
Expand Down