Addressing comments

src/AppInstallerCLITests/Experiment.cpp

@@ -19,20 +19,23 @@ TEST_CASE("Experiment_GroupPolicyControl", "[experiment]")
 {
     SECTION("Not configured")
     {
+        Experiment::ResetStates();
         GroupPolicyTestOverride policies;
         policies.SetState(TogglePolicy::Policy::Experiments, PolicyState::NotConfigured);
         ASSERT_EXPERIMENT(true, ExperimentToggleSource::Default);
     }
 
     SECTION("Enabled")
     {
+        Experiment::ResetStates();
         GroupPolicyTestOverride policies;
         policies.SetState(TogglePolicy::Policy::Experiments, PolicyState::Enabled);
         ASSERT_EXPERIMENT(true, ExperimentToggleSource::Default);
     }
 
     SECTION("Disabled")
     {
+        Experiment::ResetStates();
         GroupPolicyTestOverride policies;
         policies.SetState(TogglePolicy::Policy::Experiments, PolicyState::Disabled);
         ASSERT_EXPERIMENT(false, ExperimentToggleSource::Policy);
@@ -41,7 +44,8 @@ TEST_CASE("Experiment_GroupPolicyControl", "[experiment]")
 
 TEST_CASE("Experiment_GroupPolicyDisabled_ReturnFalse", "[experiment]")
 {
-    TestUserSettings settings; \
+    Experiment::ResetStates();
+    TestUserSettings settings;
     settings.Set<Setting::Experiments>({{"TestExperiment", true}});
 
     // If the policy is disabled, then also the user settings should be ignored.
@@ -55,18 +59,21 @@ TEST_CASE("Experiment_UserSettingsIndividualControl", "[experiment]")
     SECTION("Experiments not configured in user settings")
     {
         // Default value are used
+        Experiment::ResetStates();
         ASSERT_EXPERIMENT(true, ExperimentToggleSource::Default);
     }
 
     SECTION("Experiments enabled in user settings")
     {
-        TestUserSettings settings; \
+        Experiment::ResetStates();
+        TestUserSettings settings;
         settings.Set<Setting::Experiments>({{"TestExperiment", true}});
         ASSERT_EXPERIMENT(true, ExperimentToggleSource::UserSettingIndividualControl);
     }
 
     SECTION("Experiments disabled in user settings")
     {
+        Experiment::ResetStates();
         TestUserSettings settings;
         settings.Set<Setting::Experiments>({{"TestExperiment", false}});
         ASSERT_EXPERIMENT(false, ExperimentToggleSource::UserSettingIndividualControl);
@@ -77,13 +84,15 @@ TEST_CASE("Experiment_UserSettingsGlobalControl", "[experiment]")
 {
     SECTION("'Allow experiments' not configured in user settings")
     {
+        Experiment::ResetStates();
         TestUserSettings settings;
         settings.Set<Setting::Experiments>({{"TestExperiment", true}});
         ASSERT_EXPERIMENT(true, ExperimentToggleSource::UserSettingIndividualControl);
     }
 
     SECTION("'Allow experiments' enabled in user settings")
     {
+        Experiment::ResetStates();
         TestUserSettings settings;
         settings.Set<Setting::AllowExperiments>(true);
         settings.Set<Setting::Experiments>({{"TestExperiment", true}});
@@ -92,6 +101,7 @@ TEST_CASE("Experiment_UserSettingsGlobalControl", "[experiment]")
 
     SECTION("'Allow experiments' disabled in user settings")
     {
+        Experiment::ResetStates();
         TestUserSettings settings;
         settings.Set<Setting::AllowExperiments>(false);
         settings.Set<Setting::Experiments>({{"TestExperiment", true}});

src/AppInstallerCommonCore/AppInstallerTelemetry.cpp

@@ -81,7 +81,7 @@ namespace AppInstaller::Logging
             }
         }
 
-        std::wstring GetExperimentsJson(const std::map<ExperimentKey, ExperimentState>& experiments)
+        std::wstring GetExperimentsJson(const Settings:: ExperimentStateCache& experiments)
         {
             Json::Value root;
             for (const auto& experiment : experiments)
@@ -778,17 +778,20 @@ namespace AppInstaller::Logging
 
     Settings::ExperimentState TelemetryTraceLogger::GetExperimentState(ExperimentKey key)
     {
-#ifndef AICLI_DISABLE_TEST_HOOKS
-        m_summary.Experiments.clear();
-#endif
-
-        if (m_summary.Experiments.find(key) == m_summary.Experiments.end())
+        auto it = m_summary.ExperimentCache.find(key);
+        if (it == m_summary.ExperimentCache.end())
         {
-            m_summary.Experiments[key] =  Settings::Experiment::GetStateInternal(key);
+            it = m_summary.ExperimentCache.emplace(key, Settings::Experiment::GetStateInternal(key)).first;
         }
+        return it->second;
+    }
 
-        return m_summary.Experiments[key];
+#ifndef AICLI_DISABLE_TEST_HOOKS
+    void TelemetryTraceLogger::ResetExperiments()
+    {
+        m_summary.ExperimentCache.clear();
     }
+#endif
 
     TelemetryTraceLogger::~TelemetryTraceLogger()
     {
@@ -803,7 +806,7 @@ namespace AppInstaller::Logging
 
             if (m_useSummary)
             {
-                auto experimentsJson = GetExperimentsJson(m_summary.Experiments);
+                auto experimentsJson = GetExperimentsJson(m_summary.ExperimentCache);
 
                 TraceLoggingWriteActivity(
                     g_hTraceProvider,

src/AppInstallerCommonCore/Experiment.cpp

@@ -15,38 +15,38 @@ namespace AppInstaller::Settings
     {
         ExperimentState GetExperimentStateInternal(ExperimentKey key, const UserSettings& userSettings)
         {
-            if (!GroupPolicies().IsEnabled(TogglePolicy::Policy::Experiments))
-            {
-                AICLI_LOG(Core, Info, << "Experiment " << Experiment::GetExperiment(key).Name() <<
-                    " is disabled due to group policy: " << TogglePolicy::GetPolicy(TogglePolicy::Policy::Experiments).RegValueName());
-                return { false, ExperimentToggleSource::Policy };
-            }
-
             if (key == ExperimentKey::None)
             {
                 return { false, ExperimentToggleSource::Default };
             }
 
             auto experiment = Experiment::GetExperiment(key);
-            auto experimentJsonName = experiment.JsonName();
+            if (!GroupPolicies().IsEnabled(TogglePolicy::Policy::Experiments))
+            {
+                AICLI_LOG(Core, Verbose, << "Experiment " << experiment.Name() <<
+                    " is disabled due to group policy: " << TogglePolicy::GetPolicy(TogglePolicy::Policy::Experiments).RegValueName());
+                return { false, ExperimentToggleSource::Policy };
+            }
 
+            auto experimentJsonName = experiment.JsonName();
             if (!userSettings.Get<Setting::AllowExperiments>())
             {
-                AICLI_LOG(Core, Info, << "Experiment " << Experiment::GetExperiment(key).Name() <<
+                AICLI_LOG(Core, Verbose, << "Experiment " << experiment.Name() <<
                     " is disabled due to experiments not allowed from user settings");
                 return { false, ExperimentToggleSource::UserSettingGlobalControl };
             }
 
             auto userSettingsExperiments = userSettings.Get<Setting::Experiments>();
-            if (userSettingsExperiments.find(experimentJsonName) != userSettingsExperiments.end())
+            auto userSettingsExperimentIter = userSettingsExperiments.find(experimentJsonName);
+            if (userSettingsExperimentIter != userSettingsExperiments.end())
             {
-                auto isEnabled = userSettingsExperiments[experimentJsonName];
-                AICLI_LOG(Core, Info, << "Experiment " << experiment.Name() << " is set to " << (isEnabled ? "true" : "false") << " in user settings");
+                auto isEnabled = userSettingsExperimentIter->second;
+                AICLI_LOG(Core, Verbose, << "Experiment " << experiment.Name() << " is set to " << (isEnabled ? "true" : "false") << " in user settings");
                 return { isEnabled, ExperimentToggleSource::UserSettingIndividualControl };
             }
 
             auto isEnabled = AppInstaller::Experiment::IsEnabled(experiment.GetKey());
-            AICLI_LOG(Core, Info, << "Experiment " << experiment.Name() << " is set to " << (isEnabled ? "true" : "false"));
+            AICLI_LOG(Core, Verbose, << "Experiment " << experiment.Name() << " is set to " << (isEnabled ? "true" : "false"));
             return { isEnabled, ExperimentToggleSource::Default };
         }
 
@@ -87,6 +87,13 @@ namespace AppInstaller::Settings
         return Logging::Telemetry().GetExperimentState(key);
     }
 
+#ifndef AICLI_DISABLE_TEST_HOOKS
+    void Experiment::ResetStates()
+    {
+        Logging::Telemetry().ResetExperiments();
+    }
+#endif
+
     Experiment Experiment::GetExperiment(ExperimentKey key)
     {
         switch (key)

src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h

@@ -12,6 +12,7 @@
 namespace AppInstaller::Settings
 {
     struct UserSettings;
+    typedef std::map<AppInstaller::Experiment::ExperimentKey, Settings::ExperimentState> ExperimentStateCache;
 }
 
 namespace AppInstaller::Logging
@@ -130,7 +131,7 @@ namespace AppInstaller::Logging
         std::string DOUrl;
         HRESULT DOHResult = S_OK;
 
-        std::map<AppInstaller::Experiment::ExperimentKey, Settings::ExperimentState> Experiments;
+        Settings::ExperimentStateCache ExperimentCache;
     };
 
     // This type contains the registration lifetime of the telemetry trace logging provider.
@@ -272,6 +273,10 @@ namespace AppInstaller::Logging
 
         Settings::ExperimentState GetExperimentState(AppInstaller::Experiment::ExperimentKey key);
 
+#ifndef AICLI_DISABLE_TEST_HOOKS
+        void ResetExperiments();
+#endif
+
     protected:
         bool IsTelemetryEnabled() const noexcept;
 

src/AppInstallerCommonCore/Public/winget/Experiment.h

@@ -48,6 +48,10 @@ namespace AppInstaller::Settings
         static Experiment GetExperiment(AppInstaller::Experiment::ExperimentKey key);
         static std::vector<Experiment> GetAllExperiments();
 
+#ifndef AICLI_DISABLE_TEST_HOOKS
+        static void ResetStates();
+#endif
+
         std::string Name() const { return m_name; }
         std::string JsonName() const { return m_jsonName; }
         std::string Link() const { return m_link; }

src/AppInstallerRepositoryCore/SourceList.cpp

@@ -259,19 +259,18 @@ namespace AppInstaller::Repository
         return {};
     }
 
-    std::string_view GetWellKnownSourceArg(WellKnownSource source)
+    bool IsWellKnownSourceArg(std::string_view arg, WellKnownSource source)
     {
         switch (source)
         {
         case WellKnownSource::WinGet:
-            return GetWingetCommunitySource();
+            return Utility::CaseInsensitiveEquals(arg, s_Source_WingetCommunityDefault_Arg) || Utility::CaseInsensitiveEquals(arg, s_Source_WingetCommunityExperimental_Arg);
         case WellKnownSource::MicrosoftStore:
-            return s_Source_MSStoreDefault_Arg;
+            return Utility::CaseInsensitiveEquals(arg, s_Source_MSStoreDefault_Arg);
         case WellKnownSource::DesktopFrameworks:
-            return s_Source_DesktopFrameworks_Arg;
+            return Utility::CaseInsensitiveEquals(arg, s_Source_DesktopFrameworks_Arg);
         }
-
-        return {};
+        return false;
     }
 
     std::string_view GetWellKnownSourceIdentifier(WellKnownSource source)
@@ -291,17 +290,17 @@ namespace AppInstaller::Repository
 
     std::optional<WellKnownSource> CheckForWellKnownSourceMatch(std::string_view name, std::string_view arg, std::string_view type)
     {
-        if (name == s_Source_WingetCommunityDefault_Name && arg == GetWingetCommunitySource() && type == Microsoft::PreIndexedPackageSourceFactory::Type())
+        if (name == s_Source_WingetCommunityDefault_Name && IsWellKnownSourceArg(arg, WellKnownSource::WinGet) && type == Microsoft::PreIndexedPackageSourceFactory::Type())
         {
             return WellKnownSource::WinGet;
         }
 
-        if (name == s_Source_MSStoreDefault_Name && arg == s_Source_MSStoreDefault_Arg && type == Rest::RestSourceFactory::Type())
+        if (name == s_Source_MSStoreDefault_Name && IsWellKnownSourceArg(arg, WellKnownSource::MicrosoftStore) && type == Rest::RestSourceFactory::Type())
         {
             return WellKnownSource::MicrosoftStore;
         }
 
-        if (name == s_Source_DesktopFrameworks_Name && arg == s_Source_DesktopFrameworks_Arg && type == Microsoft::PreIndexedPackageSourceFactory::Type())
+        if (name == s_Source_DesktopFrameworks_Name && IsWellKnownSourceArg(arg, WellKnownSource::DesktopFrameworks) && type == Microsoft::PreIndexedPackageSourceFactory::Type())
         {
             return WellKnownSource::DesktopFrameworks;
         }

src/AppInstallerRepositoryCore/SourceList.h

@@ -8,8 +8,8 @@
 namespace AppInstaller::Repository
 {
     // Built-in values for default sources
+    bool IsWellKnownSourceArg(std::string_view arg, WellKnownSource source);
     std::string_view GetWellKnownSourceName(WellKnownSource source);
-    std::string_view GetWellKnownSourceArg(WellKnownSource source);
     std::string_view GetWellKnownSourceIdentifier(WellKnownSource source);
     std::optional<WellKnownSource> CheckForWellKnownSourceMatch(std::string_view name, std::string_view arg, std::string_view type);
 

src/AppInstallerRepositoryCore/SourcePolicy.cpp

@@ -110,14 +110,12 @@ namespace AppInstaller::Repository
         //  - Check only against the source argument and type as the user source may have a different name.
         //  - Do a case-insensitive check as the domain portion of the URL is case-insensitive,
         //    and we don't need case sensitivity for the rest as we control the domain.
-        if (Utility::CaseInsensitiveEquals(arg, GetWellKnownSourceArg(WellKnownSource::WinGet)) &&
-            Utility::CaseInsensitiveEquals(type, Microsoft::PreIndexedPackageSourceFactory::Type()))
+        if (IsWellKnownSourceArg(arg, WellKnownSource::WinGet) && Utility::CaseInsensitiveEquals(type, Microsoft::PreIndexedPackageSourceFactory::Type()))
         {
             return IsWellKnownSourceEnabled(WellKnownSource::WinGet) ? TogglePolicy::Policy::None : TogglePolicy::Policy::DefaultSource;
         }
 
-        if (Utility::CaseInsensitiveEquals(arg, GetWellKnownSourceArg(WellKnownSource::MicrosoftStore)) &&
-            Utility::CaseInsensitiveEquals(type, Rest::RestSourceFactory::Type()))
+        if (IsWellKnownSourceArg(arg, WellKnownSource::MicrosoftStore) && Utility::CaseInsensitiveEquals(type, Rest::RestSourceFactory::Type()))
         {
             return IsWellKnownSourceEnabled(WellKnownSource::MicrosoftStore) ? TogglePolicy::Policy::None : TogglePolicy::Policy::MSStoreSource;
         }