Fixes #4274 - v2win vsdebugconsole issue CM related UnitTest failures

[tig]

Fixes

• Fixes Using Windows Host Console v2win is rendering window size badly using VSDebugConsole.exe #4274

This is a replacement for #4275 that @BDisp created and I accidentally merged then had to revert.

[tig]

Note UnitTests now fail in UpdateFromJson_DisableTrue_NewView_Normal_Attribute_Matches_Baseline_TopLevel_Scheme, illustrating where the bug is (sortof).

I'm now diving in deeper to figure this out. I likely has something to do with the way CM sets up its global caches and how DeepCloning works.

[BDisp]

@tig this is a changed unit test of your original. That really prove that the CM isn't resetting to the original configuration at all.

    [Fact]
    public void UpdateFromJson_DisableTrue_NewView_Normal_Attribute_Matches_Baseline_TopLevel_Scheme ()
    {
        Assert.True (IsInitialized ());
        Assert.False (IsEnabled);

        // Capture hardCoded hard-coded TopLevel scheme colors
        Dictionary<string, Scheme> hardCodedSchemes = new (SchemeManager.GetSchemes ());
        Color hardCodedTopLevelNormalFg = new (hardCodedSchemes ["Toplevel"].Normal.Foreground);
        Color hardCodedTopLevelNormalBg = new (hardCodedSchemes ["Toplevel"].Normal.Background);

        Assert.Equal (new Color (StandardColor.CadetBlue), hardCodedTopLevelNormalFg);
        Assert.Equal (new Color (StandardColor.Charcoal), hardCodedTopLevelNormalBg);

        // Mutate via UpdateFromJson (applies runtime theme), then Disable(true)
        var cmt = new ConfigurationManagerTests (output);
        cmt.UpdateFromJson ();


        // Capture hardCoded hard-coded TopLevel scheme colors after UpdateFromJson
        var hardCodedSchemesOriginal = SchemeManager.GetSchemes ();
        var hardCodedTopLevelNormalFgOriginal = hardCodedSchemesOriginal ["TopLevel"].Normal.Foreground;
        var hardCodedTopLevelNormalBgOriginal = hardCodedSchemesOriginal ["TopLevel"].Normal.Background;

        // Use ToString so Assert.Equal shows the actual vs expected values on failure
        Assert.Equal (hardCodedSchemes, hardCodedSchemesOriginal);
        Assert.Equal (hardCodedTopLevelNormalFg.ToString (), hardCodedTopLevelNormalFgOriginal.ToString ());
        Assert.Equal (hardCodedTopLevelNormalBg.ToString (), hardCodedTopLevelNormalBgOriginal.ToString ());
    }

[BDisp]

Note UnitTests now fail in UpdateFromJson_DisableTrue_NewView_Normal_Attribute_Matches_Baseline_TopLevel_Scheme, illustrating where the bug is (sortof).

I'm now diving in deeper to figure this out. I likely has something to do with the way CM sets up its global caches and how DeepCloning works.

Good. I'm glad I've already figured out what's going on.

[tig]

I found the root cause.

It's a nasty design error in how SchemeManager works. The crux of it is calling ConfigurationManager.SourcesManager?.Load to causes the _hardCodedConfigPropertyCache to get corrupted for the Schemes property. Thus any call to get or reset the hard coded defaults returns whatever was just loaded, not the actual hard coded defaults.

It's gonna be a lot of work to fix this correctly, but I've found a workaround that will work ok in the meantime.

I'll open another Issue to fully document this.

[tig]

Found one of the CM tests was leaving CM enabled, which was what threw me off thinking my original fix wasn't working. I've fixed that and now I think it's working.... Will continue to test.

[BDisp]

Bummer. My fix has introduced a horrific performance penalty; hence the unit tests are taking too long to run.

Back to the drawing board...

Yes, I noticed that. Deep copying should only be used when strictly necessary. I think the reason for the immense time spent running unit tests exceeds reasonable time. When the configuration is being changed, each type in the scope must instantiate and change its own configuration. The cache created by default at configuration initialization, stored in the _hardCodedConfigPropertyCache variable, should only be used to retrieve the initial configuration and not for changes made later. What causes the _hardCodedConfigPropertyCache variable to change is the call to the GetHardCodedConfigPropertiesByScope method, which in turn calls the GetHardCodedConfigPropertyCache method by the Scope and ThemeManager classes. Therefore, they read the values ​​of the properties stored in the _hardCodedConfigPropertyCache variable and then copy the new values. Because these objects are references, this change is reflected in the _hardCodedConfigPropertyCache variable.

[tig]

@BDisp did you see my quesiton above? since it was in a CR comment it may have not shown up in notifications.

[tig]

I'm still seeing an unrelated non-deterministic failure in macos and ubuntu regarding some kernel32 issue. This points to the v2 driver stuff, not what we're focused on here. But I think the CM issue has been fixed.

[BDisp]

@BDisp did you see my quesiton above? since it was in a CR comment it may have not shown up in notifications.

I actually didn't see the notification. Maybe you didn't submit it, and it was just saved in your review until you submitted it. But to answer your question, I think it doesn't make sense to use TextStyle in this situation because it will already be controlled in the application by WindowsDriver, as you can verify by running the "Text Styles" scenario. I used this solution only to get around a situation where the colors weren't being reset when exiting the application. At first, I used Console.ResetColor() and it worked, but then I started noticing that it wasn't working as intended, not restoring the original colors. So I started using fields to save the original colors at startup and reset them upon exit. This is only for cases where the Windows Host Console is defined in Windows Terminal and when being debugged by VS 2022. After exiting the app, the console is no longer usable. But if you think it's worthwhile, feel free to make any necessary changes.

[BDisp]

I'm still seeing an unrelated non-deterministic failure in macos and ubuntu regarding some kernel32 issue. This points to the v2 driver stuff, not what we're focused on here. But I think the CM issue has been fixed.

Seems that only the All_Scenarios_Quit_And_Init_Shutdown_Properly unit tests sometimes fails in ubuntu and macOS.

[tig]

@BDisp did you see my quesiton above? since it was in a CR comment it may have not shown up in notifications.

I actually didn't see the notification. Maybe you didn't submit it, and it was just saved in your review until you submitted it. But to answer your question, I think it doesn't make sense to use TextStyle in this situation because it will already be controlled in the application by WindowsDriver, as you can verify by running the "Text Styles" scenario. I used this solution only to get around a situation where the colors weren't being reset when exiting the application. At first, I used Console.ResetColor() and it worked, but then I started noticing that it wasn't working as intended, not restoring the original colors. So I started using fields to save the original colors at startup and reset them upon exit. This is only for cases where the Windows Host Console is defined in Windows Terminal and when being debugged by VS 2022. After exiting the app, the console is no longer usable. But if you think it's worthwhile, feel free to make any necessary changes.

Cool. Thanks for the explanation.

[tig]

I'm still seeing an unrelated non-deterministic failure in macos and ubuntu regarding some kernel32 issue. This points to the v2 driver stuff, not what we're focused on here. But I think the CM issue has been fixed.

Seems that only the All_Scenarios_Quit_And_Init_Shutdown_Properly unit tests sometimes fails in ubuntu and macOS.

I wish I could get Test Explorer to reliably run the tests on Ubuntu locally. This is such a pita to diagnose.

Did you see it fail locally?

When I looked at the logs of the last Action failure I saw, I couldn't find any direct evidence of that test failing. All I saw was that the test run was aborted for some reason. When I looked at all the log, I noted some spew about kernel32.dll but it didnt seem correlated with any failing test.

[BDisp]

Yes I run Ubuntu locally in VS 2022 Test Explorer with the Ubuntu profile. But you can change to what distro you have. Then click on Run until fail on the tests you want.

[tig]

Yes I run Ubuntu locally in VS 2022 Test Explorer with the Ubuntu profile. But you can change to what distro you have. Then click on Run until fail on the tests you want.

It only works for me when the moon is in a certain phase. Other times it fails with a cryptic error when trying to discover the tests.

[BDisp]

First you have to check if the distro in the profile is the same you have installed in your machine. The name is important. For test being discovered when you switch from Windows local to Ubuntu I recommend to clean and rebuild the solution. But you can have some other issue that I don't know.

[tig]

I'm doing code cleanup as a way of repeatedly pushing new commits to this PR to cause the Actions to run so I can figure out wtf is causing the intermittent failures.

[tig]

Got another failure referencing kernel32.dll on macos.

CoPilot says:

The failing job was caused by a crash during test execution. The logs show this key error:

Error in Peek: Unable to load shared library 'kernel32.dll' or one of its dependencies.
The active Test Run was aborted because the host process exited unexpectedly.
The test running when the crash occurred:
Terminal.Gui.ViewsTests.ViewDisposalTest.TestViewsDisposeCorrectly

Root Cause

• The attempt to load 'kernel32.dll' failed. This DLL is only available on Windows, but the runner is using macOS (based on file paths like /Users/runner/).
• The crash appears during test execution, likely when code attempts to use Windows-specific functionality or P/Invoke to 'kernel32.dll'.

Solution

1. Fix Platform-specific Code Usage

• Locate any code (production or test) that calls Windows APIs (e.g., kernel32.dll) and ensure it is only executed on Windows.
• Use runtime checks to avoid calling Windows-specific libraries on non-Windows platforms.

Example fix:

// Example: Prevent Windows-specific code on non-Windows
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
    // Call Windows-specific P/Invoke or APIs
}

Review any P/Invoke or platform-specific logic for correct usage.

2. Update/Test Guards in Unit Tests

• If the failing test (TestViewsDisposeCorrectly) or any setup code is using Windows APIs, add platform guards or skip those tests on non-Windows platforms.

Example using xUnit:

[Fact]
public void MyTest()
{
    if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
        return; // Or use Assert.Skip in newer xUnit
    // Windows-specific test logic
}

Or with xUnit's Skip property:

[Fact(Skip = "Windows-specific test")]

3. Review and Fix Any Library References

• Ensure no explicit references to 'kernel32.dll' or other Windows-only libraries in your cross-platform codebase.

4. Validate CI Runner Configuration

• Make sure your CI workflow does not attempt to run Windows-only code/tests on macOS or Linux runners.

---

Code References

• The crash occurred during: Terminal.Gui.ViewsTests.ViewDisposalTest.TestViewsDisposeCorrectly
• You should check the implementation of this test and any setup/disposal code for platform-specific calls.

---

Summary:
Guard all Windows-specific code with runtime OS checks, and skip or modify tests that require Windows APIs so they do not run on macOS/Linux. This prevents attempts to load 'kernel32.dll' and will resolve CI crashes on non-Windows runners.

If you want to see the referenced test code or suspect another file, let me know which source file you want to inspect.

[tig]

Ok, I'm convinced CM is no longer causing the failures. There still are intermittent failures (on MacOS), but they are not related to CM.

I'm going to merge this and I've opened #4290