Round 6 of test conversions

[dodexahedron]

Round 6!

Same as all the others.

So far, done with TextValidateFieldTests and TextViewTests. Added several more test cases too.

Moving on down the list.

Only 3 more files remain in the root of the UnitTests project, and then I'll move on to the test fixtures in the subfolders, from top to bottom.

This was branched from round 5, and has had this repo's v2 branch as of commit ea3c0c42174f22c7aa0a97b3e6cb7b341986dcfc merged into it (rather than rebasing, which requires a force push and doesn't show the differences).

No merge conflicts (makes sense since we're working on different stuff and I'm trying to avoid non-test changes for the most part).

No build problems on my dev machine.

Tests pass (aside from the ones already marked ignore).

As with previous rounds, this is ready to merge at any point it is pushed, as I push it when I merge individual change branches to it.

[dodexahedron]

Found a bug in ViewExtensions.HitTest

isLowerRight can't come back true, ever.

See: https://github.com/dodexahedron/TerminalGuiDesigner/issues/51

[dodexahedron]

Fix is pushed to this branch, in commits https://github.com/gui-cs/TerminalGuiDesigner/pull/284/commits/75930446c87ca83d0623bbd70b41d6b2e20bcfe4 (initial fix, but off by one and gives 3x3) and https://github.com/gui-cs/TerminalGuiDesigner/pull/284/commits/730aff04cd5d51e009679a1316faef620c05f500 (better fix that actually is 2x2)

[tznind]

Looks like the new resize box calculation is off after these changes. Resize should only happen when you click the very bottom right. Instead as we can see in this gif the Tableview is being resized with clicks well into the centre of the View.

It looks like the error is proportionate to the X/Y offset of the View within its parent - This makes me think that your changes are not factoring in that it is screen coordinates not local coordinates that are being processed (var screenFrame = hit.FrameToScreen();).

Resizing TableView now occurs when clicking well into the middle of the View.

[dodexahedron]

Hm.

When I stepped through in the debugger, they looked like control-relative coordinates. But that might just have been coincidence based on how I interpreted it.

I'll expand the test to use different starting locations for the control, as well, which would have caught this in the first place.

However, the hit testing claims this shouldn't be happening, via the current test. I see an easy way to fix this, but I'm going to do the test first to be sure the end result is as expected.

But the fact that IsLowerRight only comes back true when expected makes me wonder if something isn't using things consistently. 🤔

[dodexahedron]

For a little more context, I had temporarily made all 3 of those hit test methods try coordinates from 0,0 to 19,19, to see what would happen, and they all return true only when expected, so I do suspect something isn't using IsLowerRight when it probably should be.

[dodexahedron]

Ok, I pushed a change to one of the tests that makes the starting position of the view different.

All but 83 of the over 1000 resulting test cases still pass.

So, I started debugging to trace where those are going wrong.

Seems that FindDeepestView is returning null for some cases that should not be returning null.

I renamed that point local in HitTest to be more explicit about what it actually is (the call to ScreenToBounds make that variable a view-relative point).

The case I am inspecting right now is when starting position is 2,2 , dimensions are 4,3 (meaning lower right is at 6,5), and click is at 2,5. This clearly should be a hit, but FindDeepestView (in Terminal.Gui) is returning null. That should be a hit on the lower left corner of the view. All of the new test cases that should have hit the bottom edge with that starting position and dimensions (so only clickX is variable) fail for the same reason.

That's the source of at least some of these failures.

This means either there is a bug in FindDeepestView or that the test itself is making an invalid assumption (maybe impossible coordinates?).

[dodexahedron]

Nope. It's not because of impossible coordinates.

Application.Top is 80x25, so all cases are within the bounds of Application.Top.

I think there may be a bug in FindDeepestView.

[dodexahedron]

Strike that.

I stepped through FindDeepestView and the problem I think is actually that HitTest is calling that on the view under test, but is passing it the global coordinates of the click event, rather than the view-local coordinates. Thus, the hit fails, as would be expected, because the screen coordinates are outside those bounds.

So, at least up to that point, FindDeepestView is behaving properly, and HitTest is just passing the wrong coordinates to it.

Note the pushed change is not a fix, by the way. It's just an interim push for visibility into what I'm investigating.

I'm combing through HitTest, right now, to determine how best to fix it.

[dodexahedron]

I've made some progress and made some potentially interesting discoveries, as I've been working on this.

HitTest definitely did have some subtly odd behavior in certain situations, especially when IsBorderlessContainerView() is false on the tested View (I tested using Button, as well, just to make sure things were working more than just coincidentally for the base View type). The trouble is not obvious, so I've been making the code in HitTest VERY expressive, to help with things.

I now have many thousands of passing test cases with what appear to be correct results, but with caveats in other test fixtures. (I will reduce the parameter ranges before I finish, to reduce the test case volume while still covering a wide range of important possibilities).

However, as I suspected earlier, there are potential issues elsewhere that are revealed by the modified HitTest method, which I now have a much higher degree of confidence in. But, there are now a handful of failures in the MouseManager tests and one in ResizeOperationTests. I do not yet know if those failures are because of the tests themselves or if they point to an actual bug in HandleMouse or its dependencies. I will continue to debug and trace down what's going on.

In the initial debugging of the new failures, I did find an error in a test, where the test is supposedly performing two click events which should result in a drag operation. However, the coordinates of the two mouse events (hard-coded) would not actually correspond to a resize operation, as written. The fact that it used to pass indicates a very suspicious behavior that I'm definitely going to investigate next, because I have a pretty strong suspicion that, while HitTest did have some specific incorrect cases, the odd behavior you demonstrated in your UI testing was probably caused by what I'm about to dig into.

I have not yet pushed the current changes, since I want to be sure things test as expected before I do so. The behaviors here are very time-consuming to track down, especially since I have to run my suite of 8000 tests each time, which is all the new cases (which, again, are partially temporary), as well as the rest of the test project.

I'll be working on it for a bit longer, tonight. I'm going to be at CES in Vegas, tomorrow. So, if I don't have a solution before I go to bed, I likely won't have one tomorrow, either. But it's just a day trip, so I'll likely get back to it Wednesday, if I can't figure it out tonight.

As an FYI, I have had to touch ViewExtensions.cs for this, and likely will have to touch MouseManager.cs, as well.

[tznind]

Cool, enjoy CES Vegas!

One thing to check is that any fix works in the editor itself when you run it.

One thing that may confuse is that when you mount a root view in editor that is a Window then its already got a client area starting at 1,1 (because of the border).

It could be that there are places in tests or implementation that are calling things client coords when they are screen or vice versa. Also to check when you nest multiple views inside one another (e.g. root view, into tab view, into a FrameView) that you can resize and drag that nested view the same as you can the parents.

[dodexahedron]

MAN!

An emergency came up and I had to sit it out.

But the folks I was originally going with said it was pretty disappointing, so I guess small silver lining? 😅

Thanks for the tip about Window. That was what I thought, but wasn't 100% sure of, so nice to have confirmation. That's probably a good Assume.That thing to test (since things that are TG's responsibility should at least mostly be assumptions, in our tests).

There's definitely an issue with one pair of old tests (single test method with true/false parameter) that I converted in a previous round without looking into the meaning of, that are a combination of invalid and, as it turns out, redundant with other tests, as far as I can see, though I'll still probably replace them with something else, after all the conversions, unless I am able to definitively determine they are 100% redundant.

But I also see a case that we need another TG type turned into internal for, at minimum, to be able to properly handle not only for tests, but at run-time, after compilation of user code, with certain cases involving hit testing on TabViews. There's a type check done in HitTest which is done by name, but which is only actually possible to hit on base TG code. User types compiled and loaded will have a different name, even if they directly inherit from the type, and we can't do a reliable IsAssignableTo check, without having to reflect on the TabRowView type, which is really unpalatable and potentially fragile for that use case, especially for something that can be solved by a simple change from private to internal, so long as InternalsVisibleTo includes TerminalGuiDesigner.

Oh, that also brought up a mostly unrelated thought...

We should change the namespace in the UnitTests project to be under TerminalGuiDesigner, since UnitTests being the top namespace is collision-prone and could lead to consumers' unit test projects accidentally being able to see internals of Terminal.Gui.

I can stick that in round 7.

[dodexahedron]

Oh and also yes - there are almost certainly test cases that either relied on old behavior or which aren't quite mathematically tight against the actual implementation. That's the sort of stuff that all the expansion of test cases via parameterization is helping to smoke out, for relatively little initial effort - at least for the test itself haha - on my part.

On that topic (the increase in number of test cases)....

I think it might be worth considering creating a couple of different configurations for the tests (whether using NUnit's Explicit attribute or the native dotnet runconfigurations or whatever - that's not important right now), after the major test work is done.

Why?

Well, there are a lot of test cases that exist to be exhaustive, which are excessive to always have to run, when doing work on parts of the application not related to those tests. The HitTest method is a good example of that. While it's an important core component, it's also not necessary to have to run the thousands of test cases every single time you run tests while working on something not related to the hit test code, but it is still good to have it run for automated builds, since nobody really cares how long it takes for the github action to run, so long as it does run.

It's probably sufficient to either skip those tests by default for non-explicit runs or else at least run a significantly reduced set of cases for those runs (such as only actually testing the border coordinates and maybe the coordinates exactly 1 to either side of them, for the isBorder case, which would cut it by a couple thousand, as one example).

The naive and simplest way to achieve that would be to provide the tests via static TestCaseData providers, and then have two TestCaseSource attributes on the relevant test methods, with one specifying a different category that isn't part of the standard test configuration.

There are plenty of other ways to do it, but that one I think results in the least code duplication and zero indirection.

The only very minor edge case that I can think of and will call out just for fairness is that someone working on TerminalGuiDesigner who is using an environment that doesn't understand NUnit or at least all of its functionality (VSCode, I'm looking at you, mostly, and even that would require an extreme novice skill set to be hampered by) might still end up running all test cases every time, but I think that's a more than reasonable "cost," since using VS Community, which is free, or just running tests at the command line, which is also free and is cross-platform, eliminates that problem anyway.

[dodexahedron]

I was super busy this week and didn't really get a chance to continue with this, but I've resumed as of today.

I also got distracted for a couple hours down a minor rabbit hole that spawned from this that I filed as an issue for TG, when I initially came back to this today. But that led to a couple fixes and a potential real performance benefit for ListViews, so it wasn't a waste. 😅

[dodexahedron]

Important note:

Several issues at TG directly impact this, and are quite likely to result in this method needing to be re-worked. At minimum, that means updating symbol names. But there are also some behavior changes incoming which may/probably will require actual code changes beyond that.

Thus, I'm going to stop work on fixing that method, drop the commits that modified the original, and also drop the commits that modify the tests targeting HitTest, for this PR, and then continue work skipping that test fixture for now.

Once the various changes are merged in TG, I'll come back to this and update the tests and whatnot.

[tznind]

Cool that sounds like the best approach. It is clear that the current dragging is working in the editor so most likely if there are bugs its a 'double negative' kind of problem where the calling code and the implementation are both bugged but compensate for one another because of it.

I am back at work from last week so don't have as much time to work on the backlog but will still stay on top of new bugs and review work :)

[dodexahedron]

Cool beans.

I'm working on a couple of things in TG while the new layout stuff is being worked on, but I'll come back to the unit tests here once I'm done with what I'm working on.

[tznind]

Closing because of click and drag issues

[dodexahedron]

Oh wow I didn't even realize I had some left open over here since I had tabled what I was doing here to help move TGv2 along first. 😅