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]

Merge is not recommended, at this time, until I finish some workflow modifications and we have a chance to discuss that stuff.

[dodexahedron]

I want to call particular attention to https://github.com/dodexahedron/TerminalGuiDesigner/issues/28 and its associated branch.

ViewFactory didn't have any explicit separate tests defined, so I'm making some for it.

Since it's a pretty important core component, I've started with some tests that are not functional unit tests, but rather change control tests, to enforce very intentional changes to its core functionality. For example, if a new type becomes supported and is removed from the known unsupported types that are excluded, at least one of these tests would fail, unless the same change is made in the test itself. This keeps unintended side-effects of various types of changes from creeping in and creating new bugs or regressions.

I'll of course add normal tests, once I'm done with those, but I've made some initial commits to the branch associated with that issue that result in 24 test cases (per target framework version), for the change control enforcement on ViewFactory.

As part of it, I've also been making some fundamental changes to the way ViewFactory works (primarily the Create method, which is the meat of it - I've created a generic version now). However, the changes result in the same output, which I've been careful to ensure (while also adding minor flexibility that might be handy in the future, such as additional parameters for dimensions). These changes are non-breaking, and I've left the old method there, for the time being, with an [Obsolete] attribute (and it internally delegates to the new method, for types I've implemented in it so far). I have also updated usages of Create(Type) in TGD to use the new Create<T>() method, one at a time, while verifying tests continue to pass in their existing form. Then I also make the tests call the generic method, once that is verified. I also (so far) moved a constant from another class into ViewFactory, to make a cleaner dependency graph.

If you want to peruse the changes, I do make a lot of micro-commits, so it is generally pretty easy to follow individual changes. But I'm more than willing to discuss it all directly (and kinda prefer that anyway).

Apologies for any style differences. That isn't me being passive-aggressive or anything haha. It's mostly just whitespace differences (and also explicit types on declarations, usually). Both are not a big deal to change on my end, though, if I have a spec you'd like me to follow. The stylecop rules were really not compatible with newer language constructs and were extremely noisy, so I disabled them in my local config. StyleCop seems to be sorta-kinda-dead, anyway?\ ReSharper is doing the formatting, on my end, using settings that mostly follow MS style guides (with deviations both to adhere a little closer to this project's original code, as well as some personal preferences that are mostly just for readability over all else). I'm of course more than willing to work out ReSharper formatting settings that will bring it closer to what was previously in the project. Either way, I can also share the resulting team-shared DotSettings files, if you like. Bottom line on that is it isn't my intent to impose style changes, if they are disliked, and I'll go back and work on changing it, if you want. :)

That branch hasn't yet been merged to the branch in this PR, but it will be once I'm done writing the ViewFactory tests.

[dodexahedron]

I have discovered some sort of strange coupling between two different test fixtures which caused one fixture to fail certain tests if a change was made to the other fixture.

The two are ColorSchemeTests and AddViewOperationTests

I'm going to investigate what's going on there.\ Different test fixtures shouldn't be dependent like that.\ I suspect there's some sort of side effect happening that is a consequence of how the application startup and such is handled, since they are all inheriting from a base class. That, I believe, has the effect of ALL fixtures executing in the context of the single startup, rather than the startup happening for each one, independently.

[tznind]

Thanks for your hard work on this. Love the changes! I have left a few comments, let me know if any of them require elaboration.

[dodexahedron]

Love the feedback.

Gave a few quick responses, but I'm on my phone on my way to a Christmas party, so apologies if they're a bit incomplete.

There's a lot of work beyond this that hasn't been pushed yet, because I've discovered some things that spidered out quite a bit that I'm wanting to be sure I address as well and as close to the original intent as possible.

At the moment, I've got branches of branches of branches, to help me keep the work a bit more organized, but they'll get merged as appropriate into my main working branch(es) when they're ready.

[dodexahedron]

I've also got quite a bit of running commentary on the various issues I've filed for things, over on my fork, for the work items related to all of this.

Some of it is just me putting my thoughts in writing, but some of it is also exploring real-world problems, opportunities for improvements, and my thinking regarding changes I make.

I tend to be pretty verbose, even if I'm the only one who ever ends up reading it, because it's saved me a lot of effort plenty of times, in the past. 😅

[dodexahedron]

So...

Just to keep some of the more general, yet still single-topic-ish stuff a bit more contained, I turned on discussions on my fork and opened these two:

Generics:\ https://github.com/dodexahedron/TerminalGuiDesigner/discussions/32

Code Generation:\ https://github.com/dodexahedron/TerminalGuiDesigner/discussions/33

Nothing there yet, other than starter comments and a link to a pretty nifty code generation resource, but just wanted to get ahead of things before we have a hundred scattered comments we can't keep track of. 😅

[dodexahedron]

So...

Now that I'm restricting the changes to just test conversion and necessary code to support that, I'd say you can play with merging this to whatever working branch you have, as you want, because I'm only merging completed and working units of work to this branch.

I just finished with DimExtensionsTests a little while ago, and that's now been merged to this branch, as well.

With the improvements, expansions, and unrolling of tests, so far, just in the few that have been done, we're now up to 1130 test cases (565 per framework version).

In DimExtensionsTests, I did some additional stuff I didn't have to do, mostly to serve as additional examples of ways to use newer NUnit capabilities/constructs (though they actually reduce total code, vs not doing it that way, anyway, so yay).

Just to keep the PR going, though, I wouldn't suggest using github to merge it. Or, if you do or if github "helpfully" closes it after it notices a merge, we can just open it back up to keep things going. Just don't delete the branch, so that doesn't have to get pushed again and lose the history in the timeline above.

[dodexahedron]

A comment on two test methods, which are very similar to the originals, in DimExtensionsTests:

The last couple that test the ToCode method only tested two DimTypes, in the original.

Was that intentional, or should I add a case to test the remaining type? I would think that I should, even if the test is just to prove a known failure case or something, or else the code isn't actually fully tested.

I know the last one doesn't actually give an offset, but that doesn't mean we shouldn't check that the code doesn't come out as expected, with the same kind of input (plus or minus 2).

[dodexahedron]

Actually... Thinking about that last question...

Only one is actually necessary, for a definitively tight set of tests - the ToCode without offset (which also doesn't exist for Absolute).

So long as it is proven to return the expected code for that case, testing it in the offset case is redundant, since Terminal.Gui is responsible for that and we just get back a new absolute dim with the value added to it.

[dodexahedron]

Hey question about the Operation class and its relation to the OperationManager that came up as I'm working on the EditorTests fixture:

The Do method on an operation is not equivalent to the OperationManager.Do() method.

Is this intentional?

Seems to me that one should probably delegate to the other, so that they have the same behavior.

The impact of the current model is that calling Do() directly on an operation does not invoke the change tracking behavior that OperationManager.Do() has.

But, that change tracking is also dependent on the undoStack being modified, which means change tracking in theory does not work unless the last action that was executed had SupportsUndo == true, and either its explicitly modifies the undoStack itself or was invoked via the OperationManager, that operation will not be considered a change, for the unsaved change tracking.

I would suggest that unsaved change tracking be modified to use something not dependent on an operation supporting undo.

A change I added during work on a previous text fixture added execution counting to operations. Currently, it only increments, on Do(), but the ultimate intention was to make it also decrement, if undo was ever run on it. It was added to enable a stronger test guarantee, as it only increments if Operation.Do() is about to return true.

I have a really simple event-based solution for it that I can write up to show you (I'll stick it in a separate branch). It's not much work to do, and would solve these problems, so I'm happy to show at least a prototype, whether you want to use it or not.

[dodexahedron]

Bah

Actually, no. That's me letting myself get side-tracked again.

I'll table that for later, and will file an issue on my fork so I don't forget.

[dodexahedron]

Filed https://github.com/dodexahedron/TerminalGuiDesigner/issues/34 to keep track of that for later. Back to working on EditorTests for now.

[dodexahedron]

EditorTests is converted, and ObjectExtensionsTests is converted, with new tests added.

I've got somewhere to be, so I'll continue work on adding more tests to ObjectExtensionsTests later, but I merged current work for visibility.

[tznind]

Hey question about the Operation class and its relation to the OperationManager that came up as I'm working on the EditorTests fixture:

The Do method on an operation is not equivalent to the OperationManager.Do() method.

Is this intentional?

Yes, being able to Do an operation without having to put it on the undo list is useful.

e.g. if you want to have a composite operation that does multiple other sub operations and don't want them ramming themselves onto the undo stack. But you still want to be able to have those sub operations atomically undoable.

The impact of the current model is that calling Do() directly on an operation does not invoke the change tracking behavior that OperationManager.Do() has.

This is intenional

But, that change tracking is also dependent on the undoStack being modified, which means change tracking in theory does not work unless the last action that was executed had SupportsUndo == true, and either its explicitly modifies the undoStack itself or was invoked via the OperationManager, that operation will not be considered a change, for the unsaved change tracking.

I would like to see a unit test demonstrating this. Typically I think things that do not support undo don't get pushed into undo history. Some things like Copy cannot sensibly be undone (or are otherwise unexpected e.g. in word you do not expect Ctrl+Z to edit your clipboard).

I would suggest that unsaved change tracking be modified to use something not dependent on an operation supporting undo.

I'm open to all ideas.

A change I added during work on a previous text fixture added execution counting to operations. Currently, it only increments, on Do(), but the ultimate intention was to make it also decrement, if undo was ever run on it. It was added to enable a stronger test guarantee, as it only increments if Operation.Do() is about to return true.

I have a really simple event-based solution for it that I can write up to show you (I'll stick it in a separate branch). It's not much work to do, and would solve these problems, so I'm happy to show at least a prototype, whether you want to use it or not.

Would love to see this. It may be you have a better design (given the above constraints).

[dodexahedron]

Hey question about the Operation class and its relation to the OperationManager that came up as I'm working on the EditorTests fixture: The Do method on an operation is not equivalent to the OperationManager.Do() method. Is this intentional?

Yes, being able to Do an operation without having to put it on the undo list is useful.

e.g. if you want to have a composite operation that does multiple other sub operations and don't want them ramming themselves onto the undo stack. But you still want to be able to have those sub operations atomically undoable.

The impact of the current model is that calling Do() directly on an operation does not invoke the change tracking behavior that OperationManager.Do() has.

This is intenional

Coolio. I will keep that in mind with whatever I come up with, then, to maintain those capabilities.

But, that change tracking is also dependent on the undoStack being modified, which means change tracking in theory does not work unless the last action that was executed had SupportsUndo == true, and either its explicitly modifies the undoStack itself or was invoked via the OperationManager, that operation will not be considered a change, for the unsaved change tracking.

I would like to see a unit test demonstrating this. Typically I think things that do not support undo don't get pushed into undo history. Some things like Copy cannot sensibly be undone (or are otherwise unexpected e.g. in word you do not expect Ctrl+Z to edit your clipboard).

Yeah. The IsImpossible and SupportsUndo, in the current implementation, handle that just fine. My goal would be to keep the same behavior, while just making testing a bit easier. And potentially moving the intelligence into the application itself, so you can always call Do and just expect it to Do it properly. Which I think is even already possible with what you've already got in there, too, without any major refactoring. But, for now, it is definitely something for me to watch for in tests.

I would suggest that unsaved change tracking be modified to use something not dependent on an operation supporting undo.

I'm open to all ideas.

👌

As above, I'd like to keep true to your original intent. I don't want to step on toes - just provide help where I can, with opportunistic enhancements, mostly, especially right now. 🙂

What follows is somewhat stream of consciousness writing what pops into my head, so you've been warned (also, that means it's likely to have lots of holes) 😅: \ So my main "master plan," if you want to call it that, for such an undertaking, if it is ever done, involves a potential reduction in the size and complexity of the operation class hierarchy, by them mostly being data necessary to undo, redo, or repeat an action, and by separating some common pieces in the interface and base class into, for example, an ISupportsUndo and ISupportsRedo interface, so only the ones that need it get it, and events make all the magic work together. Events would handle a lot of the meat of what they currently do, really, and whichever way the subscription works would be what determines where the code actually lives. They're actually a good candidate to be records or even record structs, depending on a million things between now and then.

It's not really a change in user-facing functionality - it's simply a change in the execution method/programming paradigm, from synchronous imperative function dispatch to synchronous and asynchronous (not necessarily meaning async mind you, though that's cool too) event-based dispatch. Same code, different places, less unneeded code for operations that don't support things in the first place (and that also can tie into unsaved change and undo/redo tracking, as well, and makes implementing things like INotifyPropertyChanged trivial).

Obviously, to have the functionality that exists, the code, or at least what it does, in some form, has to be somewhere, and I imagine a fair amount would end up in the OperationManager, with only the specific functionality related to each one living inside it, as an event handler. I'd have to actually whiteboard it out or prototype some code to figure that all out, since sometimes control is better inverted and sometimes not. One big bonus would be a likely improvement to thread safety, almost for free, since the singleton itself wouldn't be responsible for managing the events - they themselves would, via the events, and the singleton basically just becomes a service. That could even be tossed into the "modern" .net "service" infrastructure (all DI-like), with almost no modification, from that point.

A change I added during work on a previous text fixture added execution counting to operations. Currently, it only increments, on Do(), but the ultimate intention was to make it also decrement, if undo was ever run on it. It was added to enable a stronger test guarantee, as it only increments if Operation.Do() is about to return true. I have a really simple event-based solution for it that I can write up to show you (I'll stick it in a separate branch). It's not much work to do, and would solve these problems, so I'm happy to show at least a prototype, whether you want to use it or not.

Would love to see this. It may be you have a better design (given the above constraints).

Well, it was a preliminary step toward future ideas. Partially just to keep focused on getting the tests done, I didn't go farther, and made only non-impacting changes. Baby steps (plus, that means way less likely to run into merge conflicts as you're working on other stuff). You can see it in the current code in this branch, in the base Operation class (and IOperation, though nothing uses it via the interface at the moment). It's just a private int field, a get-only internal ref int property pointing to that field, and thread-safe incrementing of the field, in the Do method, right before it's about to return true. Its use is limited to just execution counting, such as in tests, at the moment (which was the immediate goal anyway), so tests can prove that the same instance of an Operation hasn't run more than once when it shouldn't have, even though other references, such as to views themselves are maintained (relevant especially for copy/paste).

Later on, decrementing in the Undo method would be the simplest next step. Or, for more robust tracking possibilities, it could just be a separate counter, and then 3 total properties: one exposing Do/Redo count, one exposing Undo count, and one for convenience that just returns the difference of the two.

The rest of the design is quite a bit to try to convey in this form, but nothing is set in stone as far as I'm concerned, and I still have plenty to familiarize myself with, in the application, as I'm doing the test work, which will probably impact every one of these thoughts, before it's time to actually act on them.

Partially related, keeping tighter track of things would even open up an easier possibility of supporting "repeatable" actions, while still keeping reliable undo history, for example.

Anyway, enough of my rambling haha. That's all down the road, since I'm focusing on tests right now, which will take some time, at the pace I can dedicate time to it. I'm sure there will be more thoughts and questions, along the way. 😅

We're up to 573 tests per framework, as of the last code I merged to this branch. Onward!

[dodexahedron]

While I'm at it, is there anything in particular, with the unit test project, that you've been wanting?

[dodexahedron]

KeyMapTests has been converted.

Along with this came a small move toward standardization for configuration, using Microsoft.Extensions.Configuration.

It's still YAML and still uses YamlDotNet under the covers, but now we have the capability to add literally three lines to allow users to use any format they like for configuration of keymaps.

KeyMap and ColorSchemeBlueprint became record classes, as part of this.

They're configuration classes, so rigorous testing of value equality is prudent. Options were to either manually implement (and then have to maintain, in perpetuity) equality methods and operators, and their associated tests, or simply change them to records, which get value equality for free.

I did go a little farther and went ahead and made a primary constructor for them, as well, just for flexibility and for the brevity it allows in the uses of them in the existing code, so I suppose they also gained that.

Tests for KeyMap now do the following:

• Check that the default parameterless constructor does, in fact, give the expected values.
• A change control test, to ensure the test yaml file isn't changed without the test and code also being updated to match.
• Checks that loading a yaml file produces the expected KeyMap.
• Checks that reference and value equality behave as they should.
• Checks that non-destructive mutation (another cool thing gained for free with records) behaves as expected, with respect to value and reference equality of the KeyMap instances, as well as the mutated properties

To ensure someone can't accidentally break the KeyMap tests, there are also updated metadata files for git and VS that enforce LF line endings, for test purposes, and in the IDE, in the context of the KeyMapTests.cs file and the stock Keys.yaml file. The configuration test also ensures that hasn't been changed accidentally. This does not affect the actual behavior or requirements of the TerminalGuiDesigner application, itself. It just removes the need to do mutations on the string data in the tests, as was done before, just to ensure line endings don't break them.

We are now up to 629 tests per framework - all passing.

[dodexahedron]

oops. Didn't mean to close that. 🤦‍♂️

[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: https://github.com/gui-cs/Terminal.Gui/issues/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: https://github.com/gui-cs/TerminalGuiDesigner/issues/288