Extensible Filters + AggregateFilter #4200

thomhurst · 2024-11-29T20:17:16Z

Changes:

Remove ITestExecutionFilterFactory - Filters can just be registered directly on the test application builder via a delegate. They can access the service provider if they like.
Test filters expose an IsEnabled property to tell the framework whether it has been requested as a filter for this test run or whether it has the information necessary to perform its filtering.
Filters are then selected only if IsEnabled is true
The filter implementation is built just before being passed to the framework with the following logic:
- If there are test nodes passed to the execute request, then we build a test node uid filter
- If there is a single IsAvailable filter, we pass that to the framework
- If there is no available filter, we pass a NopFilter to the framework
- If there are two or more available filters, we construct an AggregateFilter, and the filters are passed in as inner filters which can be accessed via a ITestExecutionFilter[] array inside AggregateFilter
- Filters define their own IsMatch logic - So framework authors don't have to build the logic themselves. This allows filters to be built generically for MTP, and installed on different frameworks, and they should just work. And the behaviour will be consistent.
- The composite extension object can now also work with filters. That means that a filter could also be a data consumer. This would allow a filter to consume things like discovered test nodes, and have context available to perform filtering where the entire test Suite is required e.g. batching, or explicit/focus filters

AggregateFilter

The aggregate filter will be built if two or more valid filters are combined when executing the test suite.
The resulting filtering with be a combination of all the combined filtering.

e.g.

Test suite has:

Test1
Test2,
Test3
Test4
Test5
Test6

Filter 1 allows:

Test1
Test2
Test3
Test4

Filter 2 allows:

Test3
Test4
Test5
Test6

The result would be executing:

Test3
Test4

As they're the only ones that pass all filters.

Custom Filters

Framework or extension authors can now create their own filters and register them.

They create a filter that implements ITestExecutionFilter.
They then call TestApplicationBuilder.TestHost.RegisterTestExecutionFilter(...)
If IsAvailable is true for that filter, it'll be passed into the framework

thomhurst · 2024-11-30T02:50:38Z

@Evangelink Hope you don't mind me having a go at tackling a couple of issues that have been sitting around for months?

Microsoft.Testing.Extensions.Retry isn't open-source/in this repo? Got failing tests because I've changed code, but that's in a package it seems so I can't make changes to it?

thomhurst · 2024-11-30T17:26:07Z

Same happening with integration tests

Unhandled exception. System.TypeLoadException: Could not load type 'Microsoft.Testing.Platform.Requests.ITestExecutionFilterFactory' from assembly 'Microsoft.Testing.Platform, Version=42.42.42.42,

Why does it reference packages instead of the local project's code?

thomhurst · 2024-12-02T16:06:35Z

@Evangelink @nohwnd @MarcoRossignoli Providing you were okay with implementing this, how would I go about being unblocked due to private/internal code that I can't edit?

Evangelink · 2024-12-02T19:00:27Z

@thomhurst I wasn't able to play a little with the PR but although I like the concept of the aggregate filter, I don't think that the extensibility as you are doing it is the right way forward as it would allow users to register any kind of filter including filters that are not handled by the framework. As of now, the filters have to be specifically supported by the platform or the framework.

thomhurst · 2024-12-02T19:21:23Z

Hmm that's a good point. The registration code is going to have to be surfaced publicly though in order for frameworks to register it.

Might be that filters work in conjunction with capabilities? So filters are only considered active/available if the framework has a capability for it? Kinda like the trx extension. That way we know a framework can handle that filter.

thomhurst · 2024-12-02T22:28:55Z

Another alternative is the filter knows how to perform the logic itself, so that users could actually add filters on the fly without specific support from the framework. This actually makes it freely extensible by the end-user rather than the framework author.

E.g.

public interface ITestExecutionFilter
{
    ...
    bool CanRun(TestNode testNode)
}

public class MyFilter : ITestExecutionFilter
{
    ...
    public bool CanRun(TestNode testNode) => testNode.Id == "1"; 
}

public class TreeNodeFilter : ITestExecutionFilter
{
    ...
    public bool CanRun(TestNode testNode) => MatchesFilter($"{testNode.Assembly.Name}/{testNode.Namespace}/{testNode.Class}/{testNode.Name}", testNode.Properties); 
}

thomhurst · 2024-12-07T11:36:24Z

@Evangelink did you have any opinions? I think the filter defining its own logic means users could make their own filters. Way more extensible then

thomhurst · 2024-12-12T18:28:25Z

@Evangelink @nohwnd @MarcoRossignoli @Youssef1313

Here's my latest idea on filtering. Each implementation implements its own MatchesFilter(TestNode) method which defines its own logic on how it should filter.

This means all frameworks need to do is call: ITestExecutionFilter.MatchesFilter(testNode) - They don't need to do any casting, or type checking, or implementing any logic, because it's defined by the filter itself.

Of course, they still could override the behaviour by doing what they do currently - Casting to the concrete type and implementing the filtering themselves, but I don't envision a lot of scenarios where you'd want to change the logic.

This also makes filtering more consistent between frameworks as they can all use centralised logic instead of recreating it themselves, with the risk of introducing bugs.

This also means users could create their own filters and register them, and they'd automatically flow through as the ITestExecutionFilter, or inside the AggregateFilter, and the filtering will work with their newly introduced logic without the framework even having to know about the specific type/logic.

Keen to know your thoughts. I think this works nicely with the Open/Closed principle - Closed for modification but highly open to extensibility 😄

thomhurst · 2024-12-20T11:21:11Z

Bump 😇

Evangelink · 2024-12-21T07:23:39Z

Hey @thomhurst !

Sorry for the delay, the colleagues are in holidays and I was trying to make sure the new version gets released before I also leave but we had many infra issues.

I'll try to have a look asap and write my feedback

thomhurst · 2025-01-07T20:43:59Z

Did you ever manage to take a look @Evangelink ?

Evangelink · 2025-01-07T20:45:25Z

Not yet sadly. I am finishing up fixes for 3.7.1 and will be on it.

Let me auto-assign the PR.

Evangelink · 2025-01-28T11:00:17Z

@thomhurst I am really sorry but I am not able to get some focus on this for the time being.

thomhurst · 2025-03-28T13:57:18Z

Anyone going to have any capacity at any point? I'd really like to write my own batching filter.

Youssef1313 · 2025-03-28T14:19:26Z

Thanks for the ping @thomhurst

It looks like there are merge conflicts here now, if you can fix them please.

thomhurst · 2025-03-28T15:29:37Z

Thanks for the ping @thomhurst

It looks like there are merge conflicts here now, if you can fix them please.

Thanks! Pushed a newer version 😄

Youssef1313 · 2025-04-01T14:14:49Z

Yes @thomhurst, I think this is a very good improvement.

I'd like to see some tests that use and demonstrate IReceivesAllTestNodesExtension though, and there will be few things to discuss with the team to agree on the final API shape we want.

And very sorry for the delays in reviewing this.

…_Itself

thomhurst · 2025-04-01T20:51:31Z

Added a test for AggregateFilter calling inner IReceivesAllTestNodesExtension when it's called itself.

But apart from that there's no logic to test - It's just an interface that serves the purpose of passing data via the parameter. The logic is up to whoever is implementing it.

… feature/extensible-filters

thomhurst · 2025-04-01T21:10:49Z

test/UnitTests/Microsoft.Testing.Platform.UnitTests/Filters/FiltersTests.cs

+    }
+
+    [TestMethod]
+    public async Task Batch_ReceivesAllTestNodes_Filter_Example()


@Youssef1313 Here's an example of how I'd like to to use IReceivesAllTestNodesExtension within a filter.

I assume in real-world, you will do something like if ([run|discover]TestExecutionRequest.Filter is IReceivesAllTestNodesExtension receivesTestNodeFilter) receivesTestNodeFilter.ReceiveTestNodes()?

Isn't that achievable if you define the interface on your side, and have the special handling for AggregateFilter? Something like:

if (request.Filter is AggregateFilter aggregateFilter) { foreach (var inner in aggregateFilter.InnerFilters.OfType<IReceivesAllTestNodes>()) { inner.ReceiveTestNodes(); } } (request.Filter as IReceivesAllTestNodes)?.ReceiveTestNodes();

Yeah you're right actually. I can just define this my side. I'll remove it from this PR!

drognanar · 2025-04-03T12:16:52Z

src/Platform/Microsoft.Testing.Extensions.VSTestBridge/Requests/VSTestTestExecutionFilter.cs

@@ -24,4 +25,10 @@ private VSTestTestExecutionFilter()
    public ImmutableArray<TestCase>? TestCases => null;

    internal static VSTestTestExecutionFilter Instance { get; } = new();
+
+    /// <inheritdoc />
+    public bool IsEnabled => false;


why is this filter disabled?

It's an obsolete filter and always returns false for a match

drognanar · 2025-04-03T12:18:09Z

src/Platform/Microsoft.Testing.Platform/Requests/AggregateFilter.cs

+    public IReadOnlyList<ITestExecutionFilter> InnerFilters { get; } = innerFilters;
+
+    /// <inheritdoc />
+    public bool IsEnabled => true;


we should check whether any of the subfilters are enabled

drognanar · 2025-04-03T12:36:04Z

src/Platform/Microsoft.Testing.Platform/TestHost/TestHostManager.cs

        }

-        return ActionResult.Fail<ITestExecutionFilterFactory>();
+        ITestExecutionFilter[] requestedFilters = list
+            .Where(x => x.IsEnabled)


I think we should pass all filters rather than filter them out. this allows us to keep the async nature of the factories and build them at the time that the request is invoked.

especially in case of the server mode, where the request is invoked once JSON-RPC method is invoked and that might not happen immediately. and might impact the filter generated.

for instance, this affects how the RetryFilter operates. with a delayed, async factory, the RetryFilterFactory can check with the retry orchestrator if there's tests to be retried or not and disable the filter on construction. once built, the filter should be immediate, i.e. not contain any async code.

with the non-async approach, either each filter match needs to become async, or all of the filter creation needs to be done at process startup, which the RetryFilter does not have the luxury of doing.

Each match being async also gives more flexibility to filters. And for most filters, we can just return Task.FromResult(true/false) so it won't be heavy on allocations or create a state machine.

I've changed the API to async so you can see what it'd be like

drognanar · 2025-04-03T13:18:50Z

src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt

+Microsoft.Testing.Platform.Requests.AggregateFilter.IsEnabled.get -> bool
+Microsoft.Testing.Platform.Requests.AggregateFilter.MatchesFilter(Microsoft.Testing.Platform.Extensions.Messages.TestNode! testNode) -> bool
+Microsoft.Testing.Platform.Requests.ITestExecutionFilter.IsEnabled.get -> bool
+Microsoft.Testing.Platform.Requests.ITestExecutionFilter.MatchesFilter(Microsoft.Testing.Platform.Extensions.Messages.TestNode! testNode) -> bool


if we want to open up the filtering, we should have a discussion on whether this is the right abstraction.

will it handle the tree filter correctly?

some test frameworks have a notion of focus tests, where if any test has a focus label only that test is run. is this an example where the filter factory would need to query for all of the test nodes in the dll?

longer term, can we optimize discovery via the filter? the current MatchesFilter() requires the framework to discover all test nodes. would we be able to optimize the discovery (to only discover tests in a class) if the test execution filters can be added externally as a delegate?

will it handle the tree filter correctly?

Why wouldn't it?

the current MatchesFilter() requires the framework to discover all test nodes

What do you mean? Currently it just accepts a single node, so it doesn't care about the rest of them.

some test frameworks have a notion of focus tests, where if any test has a focus label only that test is run. is this an example where the filter factory would need to query for all of the test nodes in the dll?

I had an idea for a BatchFilter too. This would need a way of passing it all the test nodes so it has the information it needs.

My initial idea was an interface like IReceivesAllTestNodes with a method that takes testnodes as a parameter, allowing the filter to then store them within a field or something.

This would then be up to the framework to call. Or alternatively, could a DataConsumer of DiscoveredTestNodes exist that aggregates them all and could pass them to the filter if it has that interface? Can a DataConsumer see a filter? 🤷‍♂️

Actually - Could a filter itself just also be registered as a data consumer itself?

@drognanar Can you follow-up here please?

I've added the ability for test filters to also be data consumers. See here: #4200 (comment)

…ers can also be data consumers

Youssef1313 · 2025-04-09T10:42:09Z

src/Platform/Microsoft.Testing.Platform/Extensions/CompositeExtensionsFactory.cs

-public class CompositeExtensionFactory<TExtension> : ICompositeExtensionFactory, ICloneable
-    where TExtension : class, IExtension
+public class CompositeExtensionFactory<TExtension> : ICompositeExtensionFactory
+    where TExtension : class


Why drop the IExtension constraint?

Each TestHost method able to take a CompositeExtensionFactory can keep its constraints, so we know we're still registering the appropriate types.

public void AddDataConsumer<T>(CompositeExtensionFactory<T> compositeServiceFactory) where T : class, IDataConsumer

public void AddTestSessionLifetimeHandle<T>(CompositeExtensionFactory<T> compositeServiceFactory) where T : class, ITestSessionLifetimeHandler

And the new one:

public void AddTestExecutionFilter<T>(CompositeExtensionFactory<T> compositeServiceFactory) where T : class, ITestExecutionFilter

Now an ITestExecutionFilter can be registered using a CompositeExtensionFactory - Meaning a filter could also be a DataConsumer.

My thinking for this was TestFilters that need the context of all test nodes (Explicit Filter, or Batch Filter) could be both a filter and a data consumer of test nodes. They could collect and aggregate discovered test nodes, and then use those within their filtering logic, and don't then require being passed anything by the test framework, it'll just happen automatically.

TL;DR: A ITestExecutionFilter isn't an IExtension so this constraint was preventing me from building a filter with this object type.

Youssef1313 · 2025-04-09T10:43:23Z

src/Platform/Microsoft.Testing.Platform/Requests/AggregateFilter.cs

+    }
+
+    private async Task<List<ITestExecutionFilter>> GetEnabledFiltersAsync()
+        => _enabledFilters ??= await Task.Run(async () =>


Why Task.Run instead of just having the logic directly?

Removed Task.Run

thomhurst requested review from MarcoRossignoli and Evangelink as code owners November 29, 2024 20:17

microsoft-github-policy-service bot added Area: MTP Belongs to the Microsoft.Testing.Platform core library Type: Feature labels Nov 29, 2024

thomhurst marked this pull request as draft November 29, 2024 20:20

Evangelink removed the Type: Feature label Dec 6, 2024

Evangelink self-assigned this Jan 7, 2025

Re-do custom filters

551d8e0

thomhurst force-pushed the feature/extensible-filters branch from e1164e8 to 551d8e0 Compare March 28, 2025 15:22

thomhurst added 5 commits March 28, 2025 15:36

Fix warnings

9937ee5

Merge branch 'main' into feature/extensible-filters

621aa43

Update Public API

39e7508

Xml docs

939bae2

Tidy usings

c736c8c

Youssef1313 assigned nohwnd and Youssef1313 Apr 1, 2025

AggregateFilter_CallsInner_IReceivesAllTestNodesExtension_When_Called…

c8ffe1f

…_Itself

thomhurst added 2 commits April 1, 2025 21:52

Merge remote-tracking branch 'origin/feature/extensible-filters' into…

e978ccb

… feature/extensible-filters

Batch_ReceivesAllTestNodes_Filter_Example

7e355b8

thomhurst commented Apr 1, 2025

View reviewed changes

thomhurst added 4 commits April 3, 2025 12:34

Merge branch 'main' into feature/extensible-filters

22ca79e

Run UpdateXlf Target

3619178

Remove IReceivesAllTestNodesExtension

b87979e

Tidy using filter

a7bd87f

drognanar reviewed Apr 3, 2025

View reviewed changes

thomhurst added 12 commits April 3, 2025 15:05

Async filtering

ff01160

Fix RetryExecutionFilter

d6356cc

Make CompositeExtensionFactory work with ITestExecutionFilter so filt…

5294dae

…ers can also be data consumers

Public API

468a5eb

Fix tests

20bea7a

Fix test

651cb61

Fix test

8293483

NopExpression

c44917d

Merge branch 'main' into feature/extensible-filters

ef2a0fa

true

49c1baf

Don't call IsEnabledAsync on every match filter call

49b866d

Merge branch 'main' into feature/extensible-filters

8d03360

Youssef1313 reviewed Apr 9, 2025

View reviewed changes

thomhurst added 3 commits April 9, 2025 12:01

Remove Task.Run

609851d

Merge branch 'main' into feature/extensible-filters

016e02b

Assign

d9e4c97

Extensible Filters + AggregateFilter #4200

Are you sure you want to change the base?

Extensible Filters + AggregateFilter #4200

Uh oh!

Conversation

thomhurst commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AggregateFilter

Custom Filters

Uh oh!

thomhurst commented Nov 30, 2024

Uh oh!

thomhurst commented Nov 30, 2024

Uh oh!

thomhurst commented Dec 2, 2024

Uh oh!

Evangelink commented Dec 2, 2024

Uh oh!

thomhurst commented Dec 2, 2024

Uh oh!

thomhurst commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomhurst commented Dec 7, 2024

Uh oh!

thomhurst commented Dec 12, 2024

Uh oh!

thomhurst commented Dec 20, 2024

Uh oh!

Evangelink commented Dec 21, 2024

Uh oh!

thomhurst commented Jan 7, 2025

Uh oh!

Evangelink commented Jan 7, 2025

Uh oh!

Evangelink commented Jan 28, 2025

Uh oh!

thomhurst commented Mar 28, 2025

Uh oh!

Youssef1313 commented Mar 28, 2025

Uh oh!

thomhurst commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented Apr 1, 2025

Uh oh!

thomhurst commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomhurst Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomhurst commented Nov 29, 2024 •

edited

Loading

thomhurst commented Dec 2, 2024 •

edited

Loading

thomhurst commented Mar 28, 2025 •

edited

Loading

thomhurst Apr 3, 2025 •

edited

Loading

thomhurst Apr 9, 2025 •

edited

Loading