Select checkbox of a category tree node and run selected tests => crash

[rowo360]

To Reproduce

1. Open test project which contains some test categories
2. Change tree display format to 'test fixture' and grouping to 'By category'
3. Show checkboxes in tree
4. Select checkbox of one test category
5. Start test run by toolbar item 'Run selected tests' => crash

Here's a screenshot:

Please note: this crash happens not only with category tree nodes, but with all kind of group nodes.

Environment:

• TestCentric Version - latest developer build 2.0.0-dev00451
• OS: Windows 11

[rowo360]

I've the impression that this crash is caused again by some TestNode vs. TestGroup handling. The actual crash is happing in this code location, but I believe it's not the root cause of this bug. (Class TestModel.cs)

I think the critical part is that the TestSelection contains one 'null' item. (BTW: this NullReferenceException is happing in some bog statement - but the scenario does not work if you simply skip this log statement)

[rowo360]

I think that the root cause is instead at the point where the SelectedTests are determined for the model. That's done in the TreeViewPresenter class. There's an EventHandler which is invoked whenever a checkbox in the tree is checked/unchecked.

I think it's obvious right away from the screenshot that the handling for type TestGroup is missing. I expect that this can be solved by adding all the children of a test group to the TestSelection. The children are the actual test cases or test fixtures, but I'm not aware that there's a TestGroup within a TestGroup possible.

Thinking about this code part, I wonder if the items in TestSelection must be distinct? That's actual regardless from this current issue: if I check the checkbox from a TestFixture and one of his test cases, this TestSelection will contain two items. And when starting a test run for this selection the TestRunner will receive the XML Ids of both tests. Do you know if the runner is smart enough to run the tests only once?

Quick update (edit comment): The last question remain, but the example is not appropriate: the implementation in _view.CheckNodes already takes care that not any child nodes are returned in case that the parent is already returned. Nonetheless if we extend the implemenation (for example adding all children of a test group) and we don't care, it might happen that a test case/test fixture is added twice. (TestFixture with two different test categories).

[CharliePoole]

This is a good catch. I need to refresh my memory about this part of the code, so I'll take a closer look to confirm your interpretation of what's happening. I actually forgot that we were enabling checkboxes in the TestList and FixtureList strategies. At one point they were only available in the NUnit Tree and the code you cited seems to confirm that. But enabling them certainly surfaced issues so that 's good!

[CharliePoole]

My gut feel is that this is basically a fault of TestSelection. My intent was that a TestSelection would operate in a set-like manner but I implemented it in a simplistic way, deriving from List and counting on the callers to ensure that a node was only ever added once - and also that no null nodes were ever added.

Can we solve this by first fixing TestSelection to be what it needs to be? We could keep the list as an internal implementation and only add the forwarding methods we want.

BTW, the leading comment to the TestSelection class about .NET 2.0 is no longer true.

[rowo360]

That's a good idea - moving this part to the TestSelection class! I like it and will give it a try! :-)

• TestSelection class will derive from HashSet\<TestNode> instead of List\<TestNode>
• I'll add a method void Add(ITestItem testNode) which internally needs to differ between TestNode and TestSelection added
• I can throw an exception if 'null' is passed in
• I don't need to take care about the uniqueness myself, because the HashSet is doing this already
• (Code cleanup: it seems that the delegate GroupingFunction and the commented-out method GroupBy in this class can be deleted)

[rowo360]

I implemented the idea and it worked pretty well. However while I was reviewing the solution, I wondered if I could go one step further? It might even better if the decision about HashSet or List is just an internal implementation detail of the TestSelection class. So instead of deriving from HashSet/List, it just support the IEnumerable<TestNode> interface. And the HashSet is just a private field.

I made an attempt for this approach and it worked pretty good as well. But obviously I needed to adapt a lot more code locations - all of them are not conceptual changes! Most of them are that I use LINQ to access the IEnumerable (for example Count()).

There's one exception: in class TestNode I deleted some code which performs a Sort on a TestSelection. Sorting is not supported by a HashSet - but luckily this Sort code part was never invoked (controlled by 'comparer' parameter). So it can be safely removed.

Would this approach also be OK for you? (Branch: TestSelectionExtension for a quick review)

[CharliePoole]

Yes. It should be internal. I missed your comment yesterday or I wold have said something sooner. I think the calling call should see a TestSelection with only the methods we choose to expose.

I'll look at the branch code and comment in the morning.

[CharliePoole]

I noticed that your build failed so I peeked at the branch on GitHub. It's three commits behind and one commit ahead of main, so you are not using the latest recipe. Here are the steps I take in git before I start a branch. I use git at the command-line, so if you use a different client, you'll have to translate the steps:

git checkout main
git pull
git checkout -b new-branch

After something is merged and the branch on GitHub is deleted, I do

git checkout main
git pull
git remote prune origin
git branch -d for any branches listed by prune that I still have

Using -d rather than -D protects me from accidentally deleting something I still need.

Of course, if I start a new feature immediately after finishing one, I don't have to pull main twice, but if any time has passed my habit is to do it just to be sure.

When I get in a situation as you are now, with a branch based on an older main, I do rebase the branch on main and fix any conflicts that have arisen...

git checkout mybranch
git rebase main

You can set up your own routine, but make it just that - a routine. Some folks set up aliases or command scripts, but I'm used to typing the commands so I haven't bothered. You can do all or most of this in VS as well but I always have a command prompt open next to my VS window for running the CI builds, so I use that.

[rowo360]

Argh :-( Didn't we talk about branches and merging and best practices a few hours back. And now I'm already using an outdated branch! Thanks for sharing your routines, I'll stick to those. I already made an attempt, but I've the impression that I mixed something up. But nonetheless I need to leave now, but will take care about it myself on Sunday.

[CharliePoole]

One more bit of process.... for the time being (i.e. while we are still using AppVeyor) do a pr when you want a review. Since I disabled the build on PRs, we won't be bothered by spurious build failures and we'll be able to comment inline on the PR.

[rowo360]

Issue is solved with PR #1136 => closed