Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit test conversion and TGD modernization round 2 #264

Merged
merged 81 commits into from
Dec 30, 2023

Conversation

dodexahedron
Copy link
Contributor

Just continuing from #260

Feel free to merge whenever you like and I'll open another.

There's a bug in the resharper test runner right now that makes it hang when running coverage, if there's more than one runtime targeted. This session limits it to one runtime, if imported.
Also move last bit of reflection to the ReflectionHelpers class
@dodexahedron
Copy link
Contributor Author

dodexahedron commented Dec 21, 2023

A commit including the re-factor of the non-generic ViewFactory.Create method is going to be included in a merge to this branch, pretty soon. It condenses the whole thing down to a single 22-line return with a switch expression that delegates each type to the generic method, using the same type-matching rules and precedence as before, so that it is a non-breaking change.

There is no longer any duplicated code between the two methods.

I also put an obsolete attribute on it, just for ease of helping the migration to the generic version.

The generic version also has optional parameters for convenience in setting initial dimensions or default text, when relevant. The default values for all of them are identical to the old method's defaults.

I also moved the two instances of reflection that remained in the ViewFactory out into a separate ReflectionHelpers static class, since they're not really ViewFactory-specific, and potentially helpful for consolidation of other code, as the work to migrate to more ubiquitous generics goes on.

One instance of reflection also got eliminated entirely, as Terminal.Gui fixed the reason it was necessary in the first place (by putting a setter on a property we had to set via reflection, as it had no setter before), so that was a happy finding.

* coverage-pass-1:
  Apply offsets in-line and adjust bug note to be more precise (non-change)
  Use a switch expression to condense this (no behavior change)
  Don't need this any more after the previous commit
  The ReplaceLineEndings function makes all this a lot easier!
  Improve handling of the generic Create
  Turns out that's not allowed in .net7 :(
  Re-wording to be more explicit about what it does (can return internal, private, protected, etc)
  Terminal.Gui now has a setter for this, so we can get rid of this
  Move these to a different class and refactor a bit
Just an interim merge to make the Create refactor visible
@dodexahedron
Copy link
Contributor Author

Did a quick merge of the current state of the branch I'm working on, at the moment, primarily so you can finally see the fully refactored non-generic ViewFactory.Create method.

* coverage-pass-1:
  Just a note
  Condense these to a switch
  Allow this to proceed with warning
  Cover un-covered method

This is about all I can do, at the moment, without converting more tests. Will be going back to that work, now.
@dodexahedron
Copy link
Contributor Author

Alrighty. For the tests that have been converted, coverage is as high as it can really get, for the most part, aside from some unreachable code that can be ignored for coverage purposes, since it's mostly just guard code against changes in TG affecting TGD in unexpected ways.

I'll be going back to converting more test fixtures.

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

@dodexahedron dodexahedron marked this pull request as ready for review December 21, 2023 02:26
This isn't really a test of labels. It's an OperationManager test, performed on a label.
…-unit-tests-to-constraint-model-2

* 12-convert-labeltests-to-constraint-model:
  This really only tests OperationManager and Property, so renamed the fixture
  This is essentially the same test.
  Convert this test and add pre-conditions
  Annotate the fixture
  Might as well make these global too.
Can specify view types, properties, and expected types of the properties
Easy to add, either in-line as at the bottom, or via delegating to another IEnumerable, as just above that, for organizational purposes.
Some of the DimExtensions code can probably actually go away, now
Less reflection
Parallelized
More cases
Skip earlier on known pointless cases in the big test
The NotNullWhen attributes will help
Revealed unnecessary checks - removed them
@tznind
Copy link
Collaborator

tznind commented Dec 30, 2023

Great stuff! Thanks for all this work <3

@tznind tznind merged commit 08e7981 into gui-cs:v2 Dec 30, 2023
@dodexahedron
Copy link
Contributor Author

My pleasure.

I fast-forwarded my v2 fork to current and am now working on the convert-unit-tests-to-constraint-model-3 branch, from there.

I'll open a new PR. Rinse and repeat. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants