gui-cs / TerminalGuiDesigner

Forms Designer for Terminal.Gui (aka gui.cs)
MIT License
423 stars 28 forks source link

Round 3 of unit test conversions and modernization work #276

Open dodexahedron opened 8 months ago

dodexahedron commented 8 months ago

Merge whenever you like, but I'd suggest waiting til I've at least converted another fixture or two plus whatever related changes happen with those.

I'm going to finish the MenuBarTests as if the IDisposable stuff hasn't been addressed, and we can come back to that stuff later.

This branch does already include some performance improvements in MenuTracker, and a few more may come if I get the urge to bother. It ain't broke, so it doesn't need fixing, so those are just opportunistic if/when I have to inspect a method in it.

dodexahedron commented 8 months ago

Oh also.

Since we're in .net 8 land, I've started using c# 12 features in some places, for both syntactic prettiness and for some which have actual performance implications. In particular, collection expressions can help reduce allocations at run-time, so I have used that. I have also opted to make such usages a little more verbose than is absolutely necessary, just for the sake of readability, such as putting spreads and ranges on separate lines. It reduces the savings in lines of code, for those cases, but doesn't affect their compilation, so I felt the benefits to maintainability, at least while the feature is a new addition to the codebase, are worth the verbosity.

I may come back to certain collections we have, such as the arrays of types in some classes, to turn them into inline arrays, as well, since that also has a nice performance advantage, but that's not something I'm terribly worried about at the moment, so it may or may not happen. It'll be in an atomic commit, if/when it does happen anywhere.

dodexahedron commented 8 months ago

The commits I just pushed convert and improve on a big test, but also add an unregister method to MenuTracker and also makes use of Dispose on the MenuBar (by way of a using declaration).

Still a few more test methods to go before MenuBarTests is converted.

dodexahedron commented 8 months ago

Alrighty.

I finished with MenuBarTests.

It got quite a working over, though I didn't actually add any additional tests in this pass outside of two that ensure the helper methods do what they're expected to do, so that a bunch of redundant checks in tests using them could be eliminated.

Disposal of MenuBars is respected, now, and I did drop a couple of notes about things that should ideally get fixed whenever possible and practical.

I'm sure plenty more tests can and should be added to this fixture, but I'm more interested in getting the main goal of conversion finished and that fixture is going to take more time and thought than others to properly re-work and expand.

The existing tests all got various improvements, anyway, in the process, like the others I've done - just no new parameterized/unrolled tests at this time.

This might be a reasonable merge point, if you like.

I wouldn't recommend looking at the PR as one big diff, though, because the changes make it effectively a complete replacement of the original file. But, if you want to take a look at the actual deltas, as you can see, the commits are pretty darn granular. 😅

I'm going to branch from here for the next chunk of work on the next fixtures, so this branch will remain as-is.

Oh, and I went ahead and bumped the Nuget packages in this, since Dependabot has been getting all uppity about them. 😅

No impact to TGD from those.

dodexahedron commented 8 months ago

Whoops. Didn't push my last commit.

OK, NOW it's going to stay as-is.

dodexahedron commented 8 months ago

The only merge conflicts were just packagereference values in the unit test project file.

Just kept base instead of my branch, so you won't get the same merge conflict.

dodexahedron commented 8 months ago

This is obsoleted by #281, which is obsoleted by #283, but feel free to handle them however you please.