A bunch of test fixes (#9192)

A bunch of our local tests regressed recently. I'm unsure as to when
this happened. Clearly, we all do a super good job of running these
tests 😄.
* I had to make sure the call to `AppLogic::CurrentAppSettings` was
  try/caught, because that doesn't work in the tests
* I had to make the `Pointer*` events take a weak pointer to the
  `TerminalPage` because for whatever reason, they'd be called at a
  weird point in the test init, causing the tests to fail. It was weird.
  Almost as if the TerminalPage had been released, but the test logs
  showed it hadn't barely been set up yet? Whatever, this fixes it.
* The `VerifyCommandPaletteTabSwitcherOrder` test needed to take a time
  out, for reasons that are not totally clear to me. That one was flakey
  and I hate it.

### Checklist:
* [x] Doesn't close anything, this is just something I noticed.
* [x] Doesn't require docs to be updated, it's test fixes
* [x] Yea, I ran the tests 

/cc @Don-Vito: The `FilteredCommandTests` all crashed immediately for
me. I'm not sure what's causing that - I _think_ everything we need for
those tests is set up right? The generated `AppxManifest.xml` had all
the right classes listed in it, I really can't be sure what was wrong
there. These tests aren't run in CI so it's not a super big deal, but I
thought I'd let you know.

(cherry picked from commit ccda434f69)

src/cascadia/LocalTests_SettingsModel/CommandTests.cpp

@@ -275,14 +275,6 @@ namespace SettingsModelLocalTests
 
     void CommandTests::TestAutogeneratedName()
     {
-        // Tests run in Helix can't report Skipped until GH#7286 is resolved.
-        // Set ignore flag to make Helix run completely overlook it.
-        BEGIN_TEST_METHOD_PROPERTIES()
-            TEST_METHOD_PROPERTY(L"Ignore", L"True")
-        END_TEST_METHOD_PROPERTIES()
-
-        // This test to be corrected as a part of GH#7281
-
         // This test ensures that we'll correctly create commands for actions
         // that don't have given names, pursuant to the spec in GH#6532.
 

src/cascadia/LocalTests_SettingsModel/pch.h

@@ -34,6 +34,7 @@ Author(s):
 #include <WexTestClass.h>
 #include <json.h>
 #include "consoletaeftemplates.hpp"
+#include "winrtTaefTemplates.hpp"
 
 #include <winrt/Windows.ApplicationModel.Resources.Core.h>
 #include "winrt/Windows.UI.Xaml.Markup.h"

src/cascadia/LocalTests_TerminalApp/TabTests.cpp

@@ -868,13 +868,33 @@ namespace TerminalAppLocalTests
         VERIFY_ARE_EQUAL(4u, page->_mruTabs.Size());
 
         Log::Comment(L"give alphabetical names to all switch tab actions");
-        RunOnUIThread([&page]() {
+        TestOnUIThread([&page]() {
             page->_GetTerminalTabImpl(page->_tabs.GetAt(0))->Title(L"a");
+        });
+        TestOnUIThread([&page]() {
             page->_GetTerminalTabImpl(page->_tabs.GetAt(1))->Title(L"b");
+        });
+        TestOnUIThread([&page]() {
             page->_GetTerminalTabImpl(page->_tabs.GetAt(2))->Title(L"c");
+        });
+        TestOnUIThread([&page]() {
             page->_GetTerminalTabImpl(page->_tabs.GetAt(3))->Title(L"d");
         });
 
+        TestOnUIThread([&page]() {
+            Log::Comment(L"Sanity check the titles of our tabs are what we set them to.");
+
+            VERIFY_ARE_EQUAL(L"a", page->_tabs.GetAt(0).Title());
+            VERIFY_ARE_EQUAL(L"b", page->_tabs.GetAt(1).Title());
+            VERIFY_ARE_EQUAL(L"c", page->_tabs.GetAt(2).Title());
+            VERIFY_ARE_EQUAL(L"d", page->_tabs.GetAt(3).Title());
+
+            VERIFY_ARE_EQUAL(L"d", page->_mruTabs.GetAt(0).Title());
+            VERIFY_ARE_EQUAL(L"c", page->_mruTabs.GetAt(1).Title());
+            VERIFY_ARE_EQUAL(L"b", page->_mruTabs.GetAt(2).Title());
+            VERIFY_ARE_EQUAL(L"a", page->_mruTabs.GetAt(3).Title());
+        });
+
         Log::Comment(L"Change the tab switch order to MRU switching");
         TestOnUIThread([&page]() {
             page->_settings.GlobalSettings().TabSwitcherMode(TabSwitcherMode::MostRecentlyUsed);
@@ -897,18 +917,30 @@ namespace TerminalAppLocalTests
         Log::Comment(L"Switch to the next MRU tab, which is the third tab");
         RunOnUIThread([&page]() {
             page->_SelectNextTab(true);
+            // In the course of a single tick, the Command Palette will:
+            // * open
+            // * select the proper tab from the mru's list
+            // * raise an event for _filteredActionsView().SelectionChanged to
+            //   immediately preview the new tab
+            // * raise a _SwitchToTabRequestedHandlers event
+            // * then dismiss itself, because we can't fake holing down an
+            //   anchor key in the tests
+        });
+
+        TestOnUIThread([&page]() {
+            VERIFY_ARE_EQUAL(L"c", page->_mruTabs.GetAt(0).Title());
+            VERIFY_ARE_EQUAL(L"d", page->_mruTabs.GetAt(1).Title());
+            VERIFY_ARE_EQUAL(L"b", page->_mruTabs.GetAt(2).Title());
+            VERIFY_ARE_EQUAL(L"a", page->_mruTabs.GetAt(3).Title());
         });
 
         const auto palette = winrt::get_self<winrt::TerminalApp::implementation::CommandPalette>(page->CommandPalette());
 
-        VERIFY_ARE_EQUAL(1u, palette->_switcherStartIdx, L"Verify the index is 1 as we went right");
         VERIFY_ARE_EQUAL(winrt::TerminalApp::implementation::CommandPaletteMode::TabSwitchMode, palette->_currentMode, L"Verify we are in the tab switcher mode");
-
-        Log::Comment(L"Verify command palette preserves MRU order of tabs");
-        VERIFY_ARE_EQUAL(4u, palette->_filteredActions.Size());
-        VERIFY_ARE_EQUAL(L"d", palette->_filteredActions.GetAt(0).Item().Name());
-        VERIFY_ARE_EQUAL(L"c", palette->_filteredActions.GetAt(1).Item().Name());
-        VERIFY_ARE_EQUAL(L"b", palette->_filteredActions.GetAt(2).Item().Name());
-        VERIFY_ARE_EQUAL(L"a", palette->_filteredActions.GetAt(3).Item().Name());
+        // At this point, the contents of the command palette's _mruTabs list is
+        // still the _old_ ordering (d, c, b, a). The ordering is only updated
+        // in TerminalPage::_SelectNextTab, but as we saw before, the palette
+        // will also dismiss itself immediately when that's called. So we can't
+        // really inspect the contents of the list in this test, unfortunately.
     }
 }

src/cascadia/LocalTests_TerminalApp/pch.h

@@ -34,6 +34,7 @@ Author(s):
 #include <WexTestClass.h>
 #include <json.h>
 #include "consoletaeftemplates.hpp"
+#include "winrtTaefTemplates.hpp"
 
 #include <winrt/Windows.ApplicationModel.Resources.Core.h>
 #include "winrt/Windows.UI.Xaml.Markup.h"

src/cascadia/TerminalApp/AppLogic.cpp

@@ -172,6 +172,9 @@ namespace winrt::TerminalApp::implementation
 
     // Method Description:
     // - Returns the settings currently in use by the entire Terminal application.
+    // - IMPORTANT! This can throw! Make sure to try/catch this, so that the
+    //   LocalTests don't crash (because their Application::Current() won't be a
+    //   AppLogic)
     // Throws:
     // - HR E_INVALIDARG if the app isn't up and running.
     const CascadiaSettings AppLogic::CurrentAppSettings()

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -135,8 +135,7 @@ namespace winrt::TerminalApp::implementation
         }
         CATCH_LOG();
 
-        _tabRow.PointerMoved({ this, &TerminalPage::_RestorePointerCursorHandler });
-
+        _tabRow.PointerMoved({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });
         _tabView.CanReorderTabs(!isElevated);
         _tabView.CanDragTabs(!isElevated);
 
@@ -261,7 +260,11 @@ namespace winrt::TerminalApp::implementation
 
         // Store cursor, so we can restore it, e.g., after mouse vanishing
         // (we'll need to adapt this logic once we make cursor context aware)
-        _defaultPointerCursor = CoreWindow::GetForCurrentThread().PointerCursor();
+        try
+        {
+            _defaultPointerCursor = CoreWindow::GetForCurrentThread().PointerCursor();
+        }
+        CATCH_LOG();
     }
 
     // Method Description:
@@ -1387,8 +1390,8 @@ namespace winrt::TerminalApp::implementation
         // Add an event handler for when the terminal wants to set a progress indicator on the taskbar
         term.SetTaskbarProgress({ this, &TerminalPage::_SetTaskbarProgressHandler });
 
-        term.HidePointerCursor({ this, &TerminalPage::_HidePointerCursorHandler });
-        term.RestorePointerCursor({ this, &TerminalPage::_RestorePointerCursorHandler });
+        term.HidePointerCursor({ get_weak(), &TerminalPage::_HidePointerCursorHandler });
+        term.RestorePointerCursor({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });
 
         // Bind Tab events to the TermControl and the Tab's Pane
         hostingTab.Initialize(term);
@@ -3196,8 +3199,15 @@ namespace winrt::TerminalApp::implementation
     {
         if (_shouldMouseVanish && !_isMouseHidden)
         {
-            CoreWindow::GetForCurrentThread().PointerCursor(nullptr);
-            _isMouseHidden = true;
+            if (auto window{ CoreWindow::GetForCurrentThread() })
+            {
+                try
+                {
+                    window.PointerCursor(nullptr);
+                    _isMouseHidden = true;
+                }
+                CATCH_LOG();
+            }
         }
     }
 
@@ -3209,8 +3219,15 @@ namespace winrt::TerminalApp::implementation
     {
         if (_isMouseHidden)
         {
-            CoreWindow::GetForCurrentThread().PointerCursor(_defaultPointerCursor);
-            _isMouseHidden = false;
+            if (auto window{ CoreWindow::GetForCurrentThread() })
+            {
+                try
+                {
+                    window.PointerCursor(_defaultPointerCursor);
+                    _isMouseHidden = false;
+                }
+                CATCH_LOG();
+            }
         }
     }
 

src/cascadia/TerminalApp/TerminalTab.cpp

@@ -104,15 +104,21 @@ namespace winrt::TerminalApp::implementation
 
         if (auto tab{ weakThis.get() })
         {
-            const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
-            if (settings.GlobalSettings().TabWidthMode() == winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode::SizeToContent)
+            try
             {
-                tab->_headerControl.RenamerMaxWidth(HeaderRenameBoxWidthTitleLength);
-            }
-            else
-            {
-                tab->_headerControl.RenamerMaxWidth(HeaderRenameBoxWidthDefault);
+                // Make sure to try/catch this, because the LocalTests won't be
+                // able to use this helper.
+                const auto settings{ winrt::TerminalApp::implementation::AppLogic::CurrentAppSettings() };
+                if (settings.GlobalSettings().TabWidthMode() == winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode::SizeToContent)
+                {
+                    tab->_headerControl.RenamerMaxWidth(HeaderRenameBoxWidthTitleLength);
+                }
+                else
+                {
+                    tab->_headerControl.RenamerMaxWidth(HeaderRenameBoxWidthDefault);
+                }
             }
+            CATCH_LOG()
         }
     }
 

src/cascadia/UnitTests_TerminalCore/pch.h

@@ -34,6 +34,7 @@ Author(s):
 
 #include <WexTestClass.h>
 #include "consoletaeftemplates.hpp"
+#include "winrtTaefTemplates.hpp"
 
 #include <winrt/Windows.system.h>
 #include <winrt/Windows.Foundation.h>

src/cascadia/ut_app/precomp.h

@@ -45,6 +45,8 @@ Author(s):
 #include <winrt/Windows.UI.Core.h>
 #include <winrt/Windows.UI.Text.h>
 
+#include "winrtTaefTemplates.hpp"
+
 // Manually include til after we include Windows.Foundation to give it winrt superpowers
 #include "til.h"
 

src/inc/consoletaeftemplates.hpp

@@ -22,6 +22,25 @@ Revision History:
     type identifer;                                      \
     VERIFY_SUCCEEDED(TestData::TryGetValue(L#identifer, identifer), description);
 
+// Thinking of adding a new VerifyOutputTraits for a new type? MAKE SURE that
+// you include this header (or at least the relevant definition) before _every_
+// Verify for that type.
+//
+// From a thread on the matter in 2018:
+// > my best guess is that one of your cpp files uses a COORD in a Verify macro
+// > without including consoletaeftemplates.hpp. This caused the
+// > VerifyOutputTraits<COORD> symbol to be used with the default
+// > implementation. In other of your cpp files, you did include
+// > consoletaeftemplates.hpp properly and they would have compiled the actual
+// > specialization from consoletaeftemplates.hpp into the corresponding obj
+// > file for that cpp file. When the test DLL was linked, the linker picks one
+// > of the multiple definitions available from the different obj files for
+// > VerifyOutputTraits<COORD>. The linker happened to pick the one from the cpp
+// > file where consoletaeftemplates.hpp was not included properly. I’ve
+// > encountered a similar situation before and it was baffling because the
+// > compiled code was obviously doing different behavior than what the source
+// > code said. This can happen when you violate the one-definition rule.
+
 namespace WEX::TestExecution
 {
     template<>

src/inc/winrtTaefTemplates.hpp

@@ -0,0 +1,50 @@
+/*++
+Copyright (c) Microsoft Corporation
+Licensed under the MIT license.
+
+Module Name:
+- winrtTaefTemplates.hpp
+
+Abstract:
+- This module contains common TAEF templates for winrt-isms that don't otherwise
+  have them. This is very similar to consoleTaefTemplates, but this one presumes
+  that the winrt headers have already been included.
+
+Author:
+- Mike Griese 2021
+
+--*/
+
+#pragma once
+
+// Thinking of adding a new VerifyOutputTraits for a new type? MAKE SURE that
+// you include this header (or at least the relevant definition) before _every_
+// Verify for that type.
+//
+// From a thread on the matter in 2018:
+// > my best guess is that one of your cpp files uses a COORD in a Verify macro
+// > without including consoletaeftemplates.hpp. This caused the
+// > VerifyOutputTraits<COORD> symbol to be used with the default
+// > implementation. In other of your cpp files, you did include
+// > consoletaeftemplates.hpp properly and they would have compiled the actual
+// > specialization from consoletaeftemplates.hpp into the corresponding obj
+// > file for that cpp file. When the test DLL was linked, the linker picks one
+// > of the multiple definitions available from the different obj files for
+// > VerifyOutputTraits<COORD>. The linker happened to pick the one from the cpp
+// > file where consoletaeftemplates.hpp was not included properly. I’ve
+// > encountered a similar situation before and it was baffling because the
+// > compiled code was obviously doing different behavior than what the source
+// > code said. This can happen when you violate the one-definition rule.
+
+namespace WEX::TestExecution
+{
+    template<>
+    class VerifyOutputTraits<winrt::hstring>
+    {
+    public:
+        static WEX::Common::NoThrowString ToString(const winrt::hstring& hstr)
+        {
+            return WEX::Common::NoThrowString().Format(L"%s", hstr.c_str());
+        }
+    };
+}