Unit test conversion and TGD modernization (WIP)

[dodexahedron]

This is a work-in-progress for conversion of unit tests for TGD to NUnit 4.0, as well as expansion, improvement, and modernization of various tests.
This also includes some preliminary incidental modernization work on TGD itself, primarily aimed at eliminating some reflection, and some redundant operations.

See https://github.com/dodexahedron/TerminalGuiDesigner for current activity

[dodexahedron]

I just did a bunch of work on the PosTests fixture.

Coverage was increased significantly, and it revealed several bugs, most of which are noted in PosExtensions.cs or in output of tests marked Ignore (visible in test results, while allowing the fixture to "pass" for now).

The current state of that is merged to this branch.

I have not yet fixed them, as I wanted to call attention in case that raises any alarms/thoughts on your end.

Some of them are the fault of Terminal.Gui itself, but I'm pretty sure we can work around them without much work.

[tznind]

I just did a bunch of work on the PosTests fixture.

Coverage was increased significantly, and it revealed several bugs, most of which are noted in PosExtensions.cs or in output of tests marked Ignore (visible in test results, while allowing the fixture to "pass" for now).

The current state of that is merged to this branch.

I have not yet fixed them, as I wanted to call attention in case that raises any alarms/thoughts on your end.

Some of them are the fault of Terminal.Gui itself, but I'm pretty sure we can work around them without much work.

Thanks. This is awesome work. I have done a bit of manual testing and everything seems working in the editor and all the tests pass so I have merged. Feel free to continue working on this branch or create a new one. The diff was getting a bit unwieldy so I thought it was best to merge in.

I looked into the Pos issue and I have created a new bug in Terminal.Gui itself. I have had to do some of this 'unpacking' of PosCombine 0 before but really it shouldn't be a thing for so simple a method:
gui-cs/Terminal.Gui#3064

[dodexahedron]

Yeah, I'm cool with treating this branch as safe to merge.

I do individual work items on separate branches (one per issue, for the most part), and then merge them into this one, when done, if you ever want to look at more bite-sized chunks (or you can at least still see where they merged in, even if I've deleted the branch spec by the time you see it.

Those of course show up in the history here, basically looking like a squashed copy of all the commits on each branch into this one.

I'll keep noting in the commits and annotating in code, when I find bugs and such, unless they're simple and non-impacting, in which case I'm likely to just go ahead and take care of it in the moment, like how I went ahead and put the nullable context ?s on those Pos Extension methods.

However, there were a couple of changes over the past few merges that are a bit bigger than nullability context:

• KeyMap loading now uses the standard Microsoft.Extensions.Configuration API. It still uses YAMLDotNet under the hood, but now we have the ability to support any kind of configuration input supported by that, for almost no effort, in the future.
• KeyMap and ColorSchemeBlueprint got turned into records, for a bunch of reasons, mostly centered around simpler testing and that configuration settings classes are quite well-suited for being records.
• I have been making some internal changes mostly to private methods, largely centered around just some cleanup activities like turning big sets of if-return if-return if-return blocks into switches (preference toward expressions, for brevity, when possible) with guard clauses to cut down like 60% of the code volume for those, while also gaining an unimportant (for this kind of application) performance boost, as a side effect. Mostly, it's for readability, since many more cases can fit on the screen, making it easier for meatbags like us to deal with visually. 😅
• Probably a few other similar things. I always make sure everything passes on existing tests before committing changes, and ensure I've got a test covering it, before merge.
• The ViewFactory changes, which have yet to be merged, but which I want to go over with a fine-toothed comb before I do merge them, just in case. Shouldn't be a problem, but I'm taking a slow approach since there's no rush to get there, since the current implementation does work.

I think we should do a workaround and fix for that, for a couple of reasons, regardless of if it gets fixed in Terminal.Gui. Most importantly, if it's a generic Pos, which is the only kind Terminal.Gui actually exposes publicly, any one of the IsWhateverTypeOfPos() methods will return true, if it's null. But, also, TGD has a bug in that it treats Pos types created from a literal value as absolute and zero, but then has inconsistent behavior because those methods all just return true, which breaks that assumption.

Also, I think it would be good for Terminal.Gui to actually expose the sub-classes of Pos, because they're useful to be able to identify by their type, rather than having to do what TGD already has to do, which is that string comparison of the type name. If that's "fixed" in Terminal.Gui, we wouldn't have to do that any more, which would be great. I really don't see or understand the reason for those being internal classes, over there. It creates a potential problem with inheritance, anyway, if someone were to inherit from Pos. Also, if TG exposed those types publicly, many of those extension methods would actually just plain no longer be necessary, since they're essentially a workaround for that design detail.

The only reason I didn't go ahead and do the fix to the part we have control over, on the TGD end (it would be suuuuper simple), is because that could have subtle effects in the UI, if any code paths actually do reach that stuff, at runtime, and I just wanted to make you aware of it before I touched anything, in case some report were to come in related to it. I have a pretty strong feeling it does hit them, sometimes, because I've had cases when using TGD where a position mysteriously turns into another type of position, if something didn't work out quite right. Now that I've seen where that most likely comes from, I'm pretty sure fixing the null handling will, at minimum, reduce the instance of that, or at least bubble the underlying problem up to something more obvious that can be addressed, if that's not the root of it.

Alrighty. Enough rambling for now. 😅
I'm going to go back and make a little extra coverage pass over some fixtures I've already touched, before I move on to the next fixture, just because I'm kinda in the mood for that over the somewhat more rote conversion of existing tests. In fact, I had one more test method already in the works for PosExtensions that visual studio didn't actually save til I exited, so it didn't get included in the last commit before I merged. :| It's got the same issue as several others in there do, around how null Pos instances get handled.

[tznind]

I think this is the PR that nuked the code coverage

When I run the code coverage locally I still get 75%

Maybe codeql.yml is generating additional coverage output files too somehow? Cant see why it is dropping to 12%

[tznind]

Any ideas @dodexahedron? I can't see anything that looks like it would change the CI except .github/workflows/codeql.yml

Well that also is not responsible because removing it did nothing (see on test branch) https://github.com/gui-cs/TerminalGuiDesigner/actions/runs/7857029886/job/21440440937

[tznind]

[tznind]

Moving this to an issue: #288