Compare commits

...

3 commits

Author SHA1 Message Date
Mike Griese 37af3f00fe Just try some things.
This is work that I was doing during #2455 to try and get the TabTests working again. It doesn't work, as much as I'd like them to.

  I have mail out to try and get them working but I think we're stumped right now.
2019-09-13 08:34:06 -07:00
Mike Griese 00bf9f6757 Presumably fixes #2455
Catch all calls to FindProfile and MakeSettings, which might fail if the profile doesn't exist
2019-09-12 13:15:40 -07:00
Mike Griese 3a89c6e5fa Add some tests for #2455 2019-09-12 13:12:55 -07:00
5 changed files with 259 additions and 42 deletions

View file

@ -32,6 +32,9 @@ namespace TerminalAppLocalTests
TEST_METHOD(TryCreateWinRTType); TEST_METHOD(TryCreateWinRTType);
TEST_METHOD(ValidateProfilesExist); TEST_METHOD(ValidateProfilesExist);
TEST_METHOD(FindMissingProfile);
TEST_METHOD(MakeSettingsForProfileThatDoesntExist);
TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);
TEST_METHOD(ValidateDefaultProfileExists); TEST_METHOD(ValidateDefaultProfileExists);
TEST_METHOD(ValidateDuplicateProfiles); TEST_METHOD(ValidateDuplicateProfiles);
TEST_METHOD(ValidateManyWarnings); TEST_METHOD(ValidateManyWarnings);
@ -127,6 +130,154 @@ namespace TerminalAppLocalTests
} }
} }
void SettingsTests::FindMissingProfile()
{
// Test that CascadiaSettings::FindProfile returns null for a GUID that
// doesn't exist
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"profiles": [
{
"name" : "profile0",
"guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
},
{
"name" : "profile1",
"guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}"
}
]
})" };
const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
auto settings = CascadiaSettings::FromJson(settingsJsonObj);
const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");
const Profile* const profile1 = settings->FindProfile(guid1);
const Profile* const profile2 = settings->FindProfile(guid2);
const Profile* const profile3 = settings->FindProfile(guid3);
VERIFY_IS_NOT_NULL(profile1);
VERIFY_IS_NOT_NULL(profile2);
VERIFY_IS_NULL(profile3);
VERIFY_ARE_EQUAL(L"profile0", profile1->GetName());
VERIFY_ARE_EQUAL(L"profile1", profile2->GetName());
}
void SettingsTests::MakeSettingsForProfileThatDoesntExist()
{
// Test that MakeSettings throws when the GUID doesn't exist
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"profiles": [
{
"name" : "profile0",
"guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"historySize": 1
},
{
"name" : "profile1",
"guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"historySize": 2
}
]
})" };
const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
auto settings = CascadiaSettings::FromJson(settingsJsonObj);
const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");
try
{
auto terminalSettings = settings->MakeSettings(guid1);
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(1, terminalSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to MakeSettings should succeed");
}
try
{
auto terminalSettings = settings->MakeSettings(guid2);
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(2, terminalSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to MakeSettings should succeed");
}
try
{
auto terminalSettings = settings->MakeSettings(guid3);
VERIFY_IS_TRUE(false, L"This call to MakeSettings should fail");
}
catch (...)
{
VERIFY_IS_TRUE(true, L"This call to MakeSettings successfully failed");
}
try
{
auto terminalSettings = settings->MakeSettings(std::nullopt);
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(1, terminalSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to MakeSettings should succeed");
}
}
void SettingsTests::MakeSettingsForDefaultProfileThatDoesntExist()
{
// Test that MakeSettings _doesnt_ throw when we load settings with a
// defaultProfile that's not in the list, we validate the settings, and
// then call MakeSettings(nullopt). The validation should ensure that
// the default profile is something reasonable
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-3333-49a3-80bd-e8fdd045185c}",
"profiles": [
{
"name" : "profile0",
"guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"historySize": 1
},
{
"name" : "profile1",
"guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"historySize": 2
}
]
})" };
const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
auto settings = CascadiaSettings::FromJson(settingsJsonObj);
settings->_ValidateSettings();
VERIFY_ARE_EQUAL(1u, settings->_warnings.size());
VERIFY_ARE_EQUAL(2u, settings->_profiles.size());
VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid());
try
{
auto terminalSettings = settings->MakeSettings(std::nullopt);
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(1, terminalSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to MakeSettings should succeed");
}
}
void SettingsTests::ValidateDefaultProfileExists() void SettingsTests::ValidateDefaultProfileExists()
{ {
const std::string goodProfiles{ R"( const std::string goodProfiles{ R"(

View file

@ -31,7 +31,8 @@ namespace TerminalAppLocalTests
// deploying the appx takes a bit, so use sparingly (though it will // deploying the appx takes a bit, so use sparingly (though it will
// deploy once per class when used like this.) // deploy once per class when used like this.)
BEGIN_TEST_CLASS(TabTests) BEGIN_TEST_CLASS(TabTests)
TEST_CLASS_PROPERTY(L"RunAs", L"UAP") TEST_METHOD_PROPERTY(L"RunAs", L"UAP")
TEST_METHOD_PROPERTY(L"UAP:Host", L"PackagedCwa")
TEST_CLASS_PROPERTY(L"UAP:AppXManifest", L"TerminalApp.LocalTests.AppxManifest.xml") TEST_CLASS_PROPERTY(L"UAP:AppXManifest", L"TerminalApp.LocalTests.AppxManifest.xml")
END_TEST_CLASS() END_TEST_CLASS()
@ -43,6 +44,8 @@ namespace TerminalAppLocalTests
TEST_METHOD(TryCreateXamlObjects); TEST_METHOD(TryCreateXamlObjects);
TEST_METHOD(TryCreateTab); TEST_METHOD(TryCreateTab);
TEST_METHOD(TryDuplicateBadTab);
TEST_CLASS_SETUP(ClassSetup) TEST_CLASS_SETUP(ClassSetup)
{ {
winrt::init_apartment(winrt::apartment_type::single_threaded); winrt::init_apartment(winrt::apartment_type::single_threaded);
@ -107,4 +110,16 @@ namespace TerminalAppLocalTests
VERIFY_IS_NOT_NULL(newTab); VERIFY_IS_NOT_NULL(newTab);
} }
void TabTests::TryDuplicateBadTab()
{
// Create a tab with a profile with GUID A
// Reload the settings so that GUID A is no longer in the list of profiles
// Try calling _DuplicateTabViewItem on tab A
// No new tab should be created (and more importantly, the app should not crash)
// This is a tests that was inspired by GH#2455, but at the time,
// GH#2472 was still not solved, so this test was not possible to be
// authored.
}
} }

View file

@ -30,6 +30,10 @@
</Properties> </Properties>
<Dependencies> <Dependencies>
<TargetDeviceFamily Name="Windows.Universal" MinVersion="10.0.18362.0" MaxVersionTested="10.0.18362.0" /> <TargetDeviceFamily Name="Windows.Universal" MinVersion="10.0.18362.0" MaxVersionTested="10.0.18362.0" />
<!-- If you encounter errors deploying the test because these aren't
installed, try deploying the actual app package first. Sideloading this test
package doesn't install these dependencies if they're not already installed.
-->
<PackageDependency Name="Microsoft.VCLibs.140.00.Debug" MinVersion="14.0.27023.1" Publisher="CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US" /> <PackageDependency Name="Microsoft.VCLibs.140.00.Debug" MinVersion="14.0.27023.1" Publisher="CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US" />
<PackageDependency Name="Microsoft.VCLibs.140.00.Debug.UWPDesktop" MinVersion="14.0.27027.1" Publisher="CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US" /> <PackageDependency Name="Microsoft.VCLibs.140.00.Debug.UWPDesktop" MinVersion="14.0.27027.1" Publisher="CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US" />
</Dependencies> </Dependencies>

View file

@ -451,7 +451,7 @@ TerminalSettings CascadiaSettings::MakeSettings(std::optional<GUID> profileGuidA
const Profile* const profile = FindProfile(profileGuid); const Profile* const profile = FindProfile(profileGuid);
if (profile == nullptr) if (profile == nullptr)
{ {
throw E_INVALIDARG; THROW_HR(E_INVALIDARG);
} }
TerminalSettings result = profile->CreateTerminalSettings(_globals.GetColorSchemes()); TerminalSettings result = profile->CreateTerminalSettings(_globals.GetColorSchemes());

View file

@ -333,19 +333,30 @@ namespace winrt::TerminalApp::implementation
profileGuid = globalSettings.GetDefaultProfile(); profileGuid = globalSettings.GetDefaultProfile();
} }
TerminalSettings settings = _settings->MakeSettings(profileGuid); try
_CreateNewTabFromSettings(profileGuid, settings); {
// MakeSettings might throw and E_INVALIDARG if the profile guid
// doesn't exist. In that case, silently do nothing.
TerminalSettings settings = _settings->MakeSettings(profileGuid);
_CreateNewTabFromSettings(profileGuid, settings);
const int tabCount = static_cast<int>(_tabs.size()); const int tabCount = static_cast<int>(_tabs.size());
TraceLoggingWrite( TraceLoggingWrite(
g_hTerminalAppProvider, // handle to TerminalApp tracelogging provider g_hTerminalAppProvider, // handle to TerminalApp tracelogging provider
"TabInformation", "TabInformation",
TraceLoggingDescription("Event emitted upon new tab creation in TerminalApp"), TraceLoggingDescription("Event emitted upon new tab creation in TerminalApp"),
TraceLoggingInt32(tabCount, "TabCount", "Count of tabs curently opened in TerminalApp"), TraceLoggingInt32(tabCount, "TabCount", "Count of tabs curently opened in TerminalApp"),
TraceLoggingBool(profileIndex.has_value(), "ProfileSpecified", "Whether the new tab specified a profile explicitly"), TraceLoggingBool(profileIndex.has_value(), "ProfileSpecified", "Whether the new tab specified a profile explicitly"),
TraceLoggingGuid(profileGuid, "ProfileGuid", "The GUID of the profile spawned in the new tab"), TraceLoggingGuid(profileGuid, "ProfileGuid", "The GUID of the profile spawned in the new tab"),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance)); TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance));
}
CATCH_LOG();
// TODO: Should we display a dialog when we do nothing because we
// couldn't find the associated profile? This can only really happen in
// the duplicate tab scenario currently, but maybe in the future of
// keybindings with args, someone could manually open a tab/pane with
// specific a GUID.
} }
// Method Description: // Method Description:
@ -355,18 +366,29 @@ namespace winrt::TerminalApp::implementation
// - settings: the TerminalSettings object to use to create the TerminalControl with. // - settings: the TerminalSettings object to use to create the TerminalControl with.
void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, TerminalSettings settings) void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, TerminalSettings settings)
{ {
// Initialize the new tab // First, ensure that a profile exists with the provided GUID. If it
// doesn't then we're not going to be able to create a connection for
// it.
const auto* const profile = _settings->FindProfile(profileGuid);
if (profile == nullptr)
{
return;
}
// Initialize the new tab.
// Create a connection based on the values in our settings object. // Create a connection based on the values in our settings object.
const auto connection = _CreateConnectionFromSettings(profileGuid, settings); const auto connection = _CreateConnectionFromSettings(profileGuid, settings);
if (!connection)
{
// If we weren't able to create a connection, then lets just bail. This should only happen if profile is nullptr, but we'll
return;
}
TermControl term{ settings, connection }; TermControl term{ settings, connection };
// Add the new tab to the list of our tabs. // Add the new tab to the list of our tabs.
auto newTab = _tabs.emplace_back(std::make_shared<Tab>(profileGuid, term)); auto newTab = _tabs.emplace_back(std::make_shared<Tab>(profileGuid, term));
const auto* const profile = _settings->FindProfile(profileGuid);
// Hookup our event handlers to the new terminal // Hookup our event handlers to the new terminal
_RegisterTerminalEvents(term, newTab); _RegisterTerminalEvents(term, newTab);
@ -407,6 +429,10 @@ namespace winrt::TerminalApp::implementation
winrt::Microsoft::Terminal::Settings::TerminalSettings settings) winrt::Microsoft::Terminal::Settings::TerminalSettings settings)
{ {
const auto* const profile = _settings->FindProfile(profileGuid); const auto* const profile = _settings->FindProfile(profileGuid);
if (profile == nullptr)
{
return { nullptr };
}
TerminalConnection::ITerminalConnection connection{ nullptr }; TerminalConnection::ITerminalConnection connection{ nullptr };
@ -589,9 +615,16 @@ namespace winrt::TerminalApp::implementation
const auto& _tab = _tabs.at(focusedTabIndex); const auto& _tab = _tabs.at(focusedTabIndex);
const auto& profileGuid = _tab->GetFocusedProfile(); const auto& profileGuid = _tab->GetFocusedProfile();
const auto& settings = _settings->MakeSettings(profileGuid); try
{
// MakeSettings might throw and E_INVALIDARG if the profile guid
// doesn't exist in our list of profiles. In that case, silently do
// nothing.
const auto& settings = _settings->MakeSettings(profileGuid);
_CreateNewTabFromSettings(profileGuid.value(), settings); _CreateNewTabFromSettings(profileGuid.value(), settings);
}
CATCH_LOG();
} }
// Method Description: // Method Description:
@ -832,26 +865,33 @@ namespace winrt::TerminalApp::implementation
const auto realGuid = profileGuid ? profileGuid.value() : const auto realGuid = profileGuid ? profileGuid.value() :
_settings->GlobalSettings().GetDefaultProfile(); _settings->GlobalSettings().GetDefaultProfile();
const auto controlSettings = _settings->MakeSettings(realGuid); try
const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings);
const int focusedTabIndex = _GetFocusedTabIndex();
auto focusedTab = _tabs[focusedTabIndex];
const auto canSplit = focusedTab->CanSplitPane(splitType);
if (!canSplit)
{ {
return; // MakeSettings might throw and E_INVALIDARG if the profile guid
// doesn't exist in our list of profiles. In that case, silently do
// nothing.
const auto controlSettings = _settings->MakeSettings(realGuid);
const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings);
const int focusedTabIndex = _GetFocusedTabIndex();
auto focusedTab = _tabs[focusedTabIndex];
const auto canSplit = focusedTab->CanSplitPane(splitType);
if (!canSplit)
{
return;
}
TermControl newControl{ controlSettings, controlConnection };
// Hookup our event handlers to the new terminal
_RegisterTerminalEvents(newControl, focusedTab);
focusedTab->SplitPane(splitType, realGuid, newControl);
} }
CATCH_LOG();
TermControl newControl{ controlSettings, controlConnection };
// Hookup our event handlers to the new terminal
_RegisterTerminalEvents(newControl, focusedTab);
focusedTab->SplitPane(splitType, realGuid, newControl);
} }
// Method Description: // Method Description:
@ -1217,13 +1257,20 @@ namespace winrt::TerminalApp::implementation
for (auto& profile : profiles) for (auto& profile : profiles)
{ {
const GUID profileGuid = profile.GetGuid(); const GUID profileGuid = profile.GetGuid();
TerminalSettings settings = _settings->MakeSettings(profileGuid); try
for (auto& tab : _tabs)
{ {
// Attempt to reload the settings of any panes with this profile // MakeSettings might throw and E_INVALIDARG if the profile guid
tab->UpdateSettings(settings, profileGuid); // doesn't exist in our list of profiles. In that case, silently
// do nothing.
TerminalSettings settings = _settings->MakeSettings(profileGuid);
for (auto& tab : _tabs)
{
// Attempt to reload the settings of any panes with this profile
tab->UpdateSettings(settings, profileGuid);
}
} }
CATCH_LOG();
} }
// Update the icon of the tab for the currently focused profile in that tab. // Update the icon of the tab for the currently focused profile in that tab.