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

Extend filter functionality: support filtering by category and filtering by text #1162

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

rowo360
Copy link
Contributor

@rowo360 rowo360 commented Dec 5, 2024

This PR contributes to issue #1148 , #1147, #1161 which all dealing with the filtering functionality ot the test tree.

This PR extends the core filtering functionality by supporting filtering by category and supporting filtering by text now.
It doesn't have any impact to the user so far, because there's still no UI available which triggers these filters. The UI elements will be provided by the next PRs.

However there's a bunch of tests included to verify that this filtering works as expected.

The filter class contains three properties to set a filter: OutcomeFilter, CategoryFilter and TextFilter.
If one of these properties is set, the current set of filter is applied to all TestNodes and afterwards an event is triggered (so the UI can update the tree).

The actual filtering is execute in method FilterNodes, which is called recursivly. First of all a node must be visible, if any of his child node is visible. And a leaf node is visible, if all kind of filters (Outcome, category, text) are matching. So overall a combination of filters can be applied (for example: filter all 'Failed' tests from category 'Category1'.)

@rowo360 rowo360 requested a review from CharliePoole December 5, 2024 06:04
@rowo360 rowo360 self-assigned this Dec 5, 2024
Copy link
Contributor

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

I'm approving this and simultaneously suggesting a change in this comment, which is odd but necessary because GitHub assumes a much stricter way of managing merges (and teams) than I tend to use.

The interface you have will work right now. I imagine you will eventually find a need to change the interface and that's OK with me as well, So it's fine with me to merge.

OTOH, I've worked on the business of multiple filter types before, so I'll share a few thoughts about the interface so you can have them in mind.

Generally, I expect a filter object to filter, which, in this case, means to look at a particular tree item and decide whether it should be included or not. Within the framework itself, that's how filters are organized. The abstract class TestFilter has grown more complex over time, but it's fundamental method is bool Pass(ITest test), which examines a test and decides whether it should be run. A large number of individual filter types, e.g. CatFilter derive from this.

I don't suggest you copy the framework design. It's old had contains a lot of history. I do suggest that filter objects will eventually need to filter tree items.

Of course, if the filters only filtered, then some other object will have to apply them, clear the filter, set them up, etc. For the moment, I suppose that could be the strategy, but some kind of filter manager could arise if needed.

Reiterating... please think about this and decide whether to make any changes now, at a later stage or just wait until you see the need.

/// <summary>
/// Init filter after a project is loaded
/// </summary>
void Init();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should improve the interface in this PR, in the next PR or only when the need to improve it arises. Each of those approaches could make sense and there's no right answer. I'll enlarge on what I'm thinking about in my overall review comment.

return str;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Very thorough testing of the implementation. This will make it possible to change things very readily at any point.

@rowo360
Copy link
Contributor Author

rowo360 commented Dec 5, 2024

That's right - good point. I think your proposed design is more object-oriented and more flexible. So I'll prepare a new commit which breaks this (monolithic) filter class into several smaller classes. Each one designed to filter only one dedicated aspect (outcome, text or category). In addition I will introduce a filter interface which eachs on those new classes will implement.
That interface will at least contain one method like: bool IsMatching(TestNode testNode)
And the existing filter class will act like a filter manager class which aggregates all these concrete filter classes.

I think that will be a first step into the right direction and definitely a better design. On the other hand it's not absolutely necessary at the moment, as the filter is not (yet) as powerful as compared with NUnit. Therefore I expect that I will not catch all use cases concerning filtering in general in this first step but it's the first attempt. :-)

@CharliePoole
Copy link
Contributor

@rowo360 That seems reasonable to me. FWIW the old V2 GUI had fairly non-object-oriented implementation but it only had one filter type, Category, so it worked fine. But with three different types of filter I think separate classes will pay off.

…lasses, each one dedicated to one single filter condition
Copy link
Contributor

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

Thanks. This looks like a better framework in which to build up the filtering functionality. Most likely, it will change more in the future, but that's to be expected.

@rowo360 rowo360 merged commit 18b8a35 into main Dec 6, 2024
2 checks passed
@rowo360 rowo360 deleted the issue-1148 branch December 6, 2024 15:16
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.

2 participants