Round 3 of unit test conversions and modernization work #276
Conversation
C# 12 feature
Just set the object.
Potentially fewer allocations, as well
Bump nuget packages. Tests confirmed still passing.
|
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. |
Should probably be split up into multiple tests or at least have tests added for the individual actions it takes to prove them in isolation.
|
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 Still a few more test methods to go before MenuBarTests is converted. |
Assert end is identical to start Also use the OperationManager so the state of the stacks can be tested
Collection expressions to the rescue!
This was already a reference equality check. If the SameAs constraint is true, the other three MUST be true
As with the previous commit, the value checks are redundant to the reference check
A lot more reference checking, since it is known to be a destructive operation, even through Undo.
Yay! A small one!
Too bad it isn't aware of the test methods that guaranteed not null already
Enables compiler optimizations not possible with the old style.
|
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. |
|
Whoops. Didn't push my last commit. OK, NOW it's going to stay as-is. |
|
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. |
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.