Add option to merge tree nodes with only one child

[GeorchW]

I didn't like how namespaces containing only one element (e.g. Company.Project.Test) spawned a new tree node for each level. I've added a behaviour where the namespaces behave like folders in VSCode: If there's only one subfolder, it will be shown in the same line.

The option useTreeView has consequently been replaced by a similar option treeMode which can be set to "flat" (the old behaviour for false), "full" (the old behaviour for true) and "merged" (the new behaviour), which is the default.

Also refactored a little in the process.

[stefanforsberg]

Hey @GeorchW and thanks for the PR. Would you mind sharing a screenshot of how the different treeModes will look?

[GeorchW]

Heh, I've catched some bugs while trying to demonstrate :-D - Additionally, I've modified the settings update mechanism such that the view is refreshed when changing settings.

This is the old false behaviour, corresponding to the new "flat". It's really compact vertically, but quite verbose horizontally and doesn't allow collapsing any group of tests.

This is the old true behaviour, corresponding to the new "full". It's less verbose horizontally and allows to collapse tests, but it inserts many redundant nodes - if we were to remove the single test at the top, NUnitTests.NEsted.Space.TestClass1.Pass, four nodes would be removed from the tree.

And this is the added behaviour, which can be selected through "merged". I think it's a good compromise: It takes no more space than needed, but still allows to collapse any distinct group of tests. Note how the NUnitTests node at the root was removed as well: Since everything was contained in it anyway, everything was dumped directly into the tree instead. This behaviour might be something up for discussion.

[GeorchW]

I've also replaced the parsing logic for test names. The old system relied on splitting on regular expressions, which does not allow for any context-free grammar occurring in the name and is generally less readable/debuggable.

The new parsing code should also handle cases like TestClass(typeof(MyType)) and TestClass('\"')correctly. I'm not sure whether arbitrary bracket nesting can be provoked with those constant attribute parameters, but if it is, we're covered for that as well :-)

[stefanforsberg]

Hey @GeorchW! I really like the merged look, like you say it seems like a good compromise with the best of both worlds.

However I have some concern regarding the rework of the parsing logic. While you have a valid point regarding reg exp in general I'm not sure that the code that replaces it in this case is more readable and easier to understand.

Is it possible to not include that change in this PR? Either way I think it's helpful to have a separate PR with changing of the parsing logic and keep this one focused on the code needed for the merged behavior.

Makes sense?

[GeorchW]

About the parsing logic... I didn't touch it for no reason. More concretely:

• The old code did not parse nested classes - i.e. Namespace.MyClass+Nested.SomeTest("myParameter").
• When adding nested classes to the Regex, it's not clear any more how the individual parts should be glued together: if we have ["Namespace", "MyClass", "Nested"], should that be Namespace.MyClass.Nested or Namespace.MyClass+Nested?
  • We need to be able to glue the parts back together to be able to run individual classes of tests: the command line to run all tests from a subclass requires the full name of that subclass.
• The new code basically separates these concerns: Since a non-split-based parser can include whatever information is needed in its output, we don't have to glue the parts back together - the parser already provides the full name of each node.
  • This also allows for some cool new future features: For example, we could "split" at the brackets, allowing to merge all parametrized versions of a fixture under a separate node like this:

    MyParametrizedClass
    | - MyParametrizedClass("anArgument")
    |   | ... (some tests) ...
    | - MyParametrizedClass("anotherArgument")
    |   | ... (some tests) ...
    

• Finally, the old code had some bugs. For example, the string MyTest(".", typeof(MyClass)) would be split at the dot. It is not possible to eliminate such problems by rewriting the regex, since we're trying to parse a context-free grammar with a regular expression.

I was first under the impression that I had regressed from the old behaviour since nested classes did not work while nested classes were included in the test projects, but I've just looked it up and it turns out that nested classes were not supported all along; they were displayed in nodes like MyClass+Nested.

I see that this code might be more of a fit for a separate PR though. In particular, I can see that I should probably make the parsing code more clean, for example through:

• putting it into a separate file instead of in the middle of dotnetTestExplorer.ts
• making a proper recursive descent parser instead of the slightly random "counting how many brackets are open"
• putting the code for parsing different bits into different methods, i.e. having separate methods for brackets and strings

I do think that the additional features justify some additional complexity. If you think the same way, I'll make a separate PR for that. (If you don't, then I'll just throw this code out.)

[stefanforsberg]

Ah yea, moving the functionality to a separate file would improve things. Ideally I'd like to add some focused test on just this functionality, somewhat similar to what we have in https://github.com/formulahendry/vscode-dotnet-test-explorer/blob/master/test/problems.test.ts.

This would also server as a great place for you to demonstrate the difference cases of discovered test names and how they should be split up in their parts. That would make the whole thing much easier as well from a maintainence perspective.

If you would be OK with doing that work I don't think the need to split this into two different PR is all that necessary so you can do whatever is most convenient for you.

Sound ok?

[GeorchW]

Sounds great!

Tbh I thought about adding some tests, too, but was a little too lazy. It's good that you are strict on that part :-) - Since I'll already be doing that, I think I'll also write some tests for the tree generation (at least something like "no test is swallowed in either mode"). I think I'll get something done on the weekend.

EDIT: TODO:

• Make parsing code separate/rework a little
• Test parsing code
• Separate tree generation code
• Integrate it all back together
• Test tree generation code
• Test tree merging code

[GeorchW]

Okay, this should be it! @stefanforsberg

The tests might be a little verbose, but that's the price you pay for strict unit-tests (without dependencies) I guess.

[stefanforsberg]

Amazing work @GeorchW, thanks a lot! =)

I'll take a closer look later today and/or over the weekend!