Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/ApplicationState.idl
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Microsoft.Terminal.Settings.Model
{
enum InfoBarMessage
{
// We need a sentinel value to map deleted keys to, as we can't actually delete them (GH#16874)
// Unknown and deprecated dismissed message names map to this sentinel value.
IgnoredDeprecatedEntry = 0,
CloseOnExitInfo,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,21 @@ JSON_FLAG_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::IntenseStyle)

JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage)
{
ValueType FromJson(const Json::Value& json)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the deserializer. What about ToJson? We should be able to prevent serializing IgnoredDeprecatedEntry altogether.

{
const auto name{ Detail::GetStringView(json) };
for (const auto& pair : mappings)
{
if (pair.first == name)
{
return pair.second;
}
}

// Unknown names may come from another version of Terminal. They must not prevent startup.
return ValueType::IgnoredDeprecatedEntry;
}

JSON_MAPPINGS(4) = {
pair_type{ "deprecated", ValueType::IgnoredDeprecatedEntry },
pair_type{ "closeOnExitInfo", ValueType::CloseOnExitInfo },
Expand Down
45 changes: 42 additions & 3 deletions src/cascadia/UnitTests_SettingsModel/ApplicationStateTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace SettingsModelUnitTests
{
// Covers the workspace-persistence APIs added to ApplicationState:
// SaveWorkspace / RemoveWorkspace / RenameWorkspace / TakeWorkspace /
// AllPersistedWorkspaces.
// All tests operate on a throw-away ApplicationState instance pointed at
// a temp directory, so they don't touch the real user state.
class ApplicationStateTests
Expand All @@ -29,6 +26,7 @@ namespace SettingsModelUnitTests
TEST_METHOD(RenameWorkspaceNoOpForMissingEntry);
TEST_METHOD(TakeWorkspaceRemovesAndReturns);
TEST_METHOD(TakeWorkspaceReturnsNullWhenMissing);
TEST_METHOD(IgnoreUnknownDismissedMessages);

private:
static std::filesystem::path _tempRoot()
Expand Down Expand Up @@ -56,6 +54,47 @@ namespace SettingsModelUnitTests
}
};

void ApplicationStateTests::IgnoreUnknownDismissedMessages()
{
Log::Comment(L"GH #16874 - Unknown dismissed message names must not terminate state loading");

Json::Value json{ Json::objectValue };
json["settingsHash"] = "test-hash";
json["dismissedMessages"] = Json::Value{ Json::arrayValue };
for (const auto name : { "futureMessage", "closeOnExitInfo", "removedMessage", "keyboardServiceWarning", "setAsDefault", "" })
{
json["dismissedMessages"].append(name);
}
json["allowedCommandlines"] = Json::Value{ Json::arrayValue };
json["allowedCommandlines"].append("test-command");

auto state = _make();
state->FromJson(json, implementation::FileSource::Shared | implementation::FileSource::Local);

const auto messages = state->DismissedMessages();
VERIFY_IS_NOT_NULL(messages);
VERIFY_ARE_EQUAL(6u, messages.Size());
VERIFY_ARE_EQUAL(InfoBarMessage::IgnoredDeprecatedEntry, messages.GetAt(0));
VERIFY_ARE_EQUAL(InfoBarMessage::CloseOnExitInfo, messages.GetAt(1));
for (uint32_t i = 2; i < messages.Size(); ++i)
{
VERIFY_ARE_EQUAL(InfoBarMessage::IgnoredDeprecatedEntry, messages.GetAt(i));
}

// Invalid names must not prevent fields before or after this one from loading.
VERIFY_ARE_EQUAL(L"test-hash", state->SettingsHash());
VERIFY_ARE_EQUAL(1u, state->AllowedCommandlines().Size());
VERIFY_ARE_EQUAL(L"test-command", state->AllowedCommandlines().GetAt(0));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you shrink the tests to just spot check the core issue to protect against future regressions?

(You got to really reign in your LLM when it comes to tests - they always tend to write overly defensive / broad tests. 😅)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, yeah 😅 I'll fix the tests and keep them focused on the bug. Thanks!


const auto serialized = state->ToJson(implementation::FileSource::Shared);
VERIFY_ARE_EQUAL(6u, serialized["dismissedMessages"].size());
VERIFY_ARE_EQUAL("closeOnExitInfo", serialized["dismissedMessages"][1].asString());
for (const auto i : { 0, 2, 3, 4, 5 })
{
VERIFY_ARE_EQUAL("deprecated", serialized["dismissedMessages"][i].asString());
}
}

void ApplicationStateTests::SaveAndLookupWorkspace()
{
auto state = _make();
Expand Down
Loading