RetroAchievements: Updated PatchAllowlistTest to generate new allowlist

Refactors the PatchAllowlistTest to streamline the experience for developers. Instead of a textual description of what needs to change in ApprovedInis.json for RetroAchievements compatibility, the test will now generate a replacement file and instruct the coder where to copy it in their local branch, and what to update APPROVED_LIST_HASH to. The result should be easier and more instructive for developers to make changes, while still maintaining that allowed codes cannot be added or modified without recompiling Dolphin.

As ApprovedInis.json no longer needs to be user-readable for this process, it no longer contains titles or pretty formatting and as such is updated in this commit, hash included.

Source/Core/Core/AchievementManager.h

@@ -90,8 +90,8 @@ public:
   static constexpr std::string_view BLUE = "#0B71C1";
   static constexpr std::string_view APPROVED_LIST_FILENAME = "ApprovedInis.json";
   static const inline Common::SHA1::Digest APPROVED_LIST_HASH = {
-      0xE1, 0x29, 0xD1, 0x33, 0x4D, 0xF2, 0xF8, 0xA8, 0x4E, 0xCA,
-      0xF6, 0x87, 0xE6, 0xEC, 0xEC, 0xB3, 0x18, 0x69, 0x34, 0x45};
+      0x6D, 0x91, 0xF5, 0xC1, 0xE2, 0x4C, 0xC3, 0x39, 0xF5, 0x7F,
+      0xEC, 0xA9, 0x8C, 0xA9, 0xBD, 0x61, 0x28, 0x54, 0x11, 0x62};
 
   struct LeaderboardEntry
   {

Source/UnitTests/Core/PatchAllowlistTest.cpp

@@ -18,6 +18,7 @@
 #include "Common/IOFile.h"
 #include "Common/IniFile.h"
 #include "Common/JsonUtil.h"
+#include "Core/AchievementManager.h"
 #include "Core/ActionReplay.h"
 #include "Core/CheatCodes.h"
 #include "Core/GeckoCode.h"
@@ -35,46 +36,23 @@ using AllowList = std::map<std::string /*ID*/, GameHashes>;
 template <typename T>
 void ReadVerified(const Common::IniFile& ini, const std::string& filename,
                   const std::string& section, bool enabled, std::vector<T>* codes);
-void CheckHash(const std::string& game_id, GameHashes* game_hashes, const std::string& hash,
-               const std::string& patch_name);
 
 TEST(PatchAllowlist, VerifyHashes)
 {
-  // Load allowlist
-  static constexpr std::string_view APPROVED_LIST_FILENAME = "ApprovedInis.json";
-  picojson::value json_tree;
-  std::string error;
+  // Iterate over GameSettings directory
+  picojson::object new_allowlist;
   std::string cur_directory = File::GetExeDirectory()
 #if defined(__APPLE__)
                               + DIR_SEP "Tests"  // FIXME: Ugly hack.
 #endif
       ;
   std::string sys_directory = cur_directory + DIR_SEP "Sys";
-  const auto& list_filepath = fmt::format("{}{}{}", sys_directory, DIR_SEP, APPROVED_LIST_FILENAME);
-  ASSERT_TRUE(JsonFromFile(list_filepath, &json_tree, &error))
-      << "Failed to open file at " << list_filepath;
-  // Parse allowlist - Map<game id, Map<hash, name>>
-  ASSERT_TRUE(json_tree.is<picojson::object>());
-  AllowList allow_list;
-  for (const auto& entry : json_tree.get<picojson::object>())
-  {
-    ASSERT_TRUE(entry.second.is<picojson::object>());
-    GameHashes& game_entry = allow_list[entry.first];
-    for (const auto& line : entry.second.get<picojson::object>())
-    {
-      ASSERT_TRUE(line.second.is<std::string>());
-      if (line.first == "title")
-        game_entry.game_title = line.second.get<std::string>();
-      else
-        game_entry.hashes[line.first] = line.second.get<std::string>();
-    }
-  }
-  // Iterate over GameSettings directory
   auto directory =
       File::ScanDirectoryTree(fmt::format("{}{}GameSettings", sys_directory, DIR_SEP), false);
   for (const auto& file : directory.children)
   {
     // Load ini file
+    picojson::object approved;
     Common::IniFile ini_file;
     ini_file.Load(file.physicalName, true);
     std::string game_id = file.virtualName.substr(0, file.virtualName.find_first_of('.'));
@@ -90,9 +68,6 @@ TEST(PatchAllowlist, VerifyHashes)
                                    &geckos);
     ReadVerified<ActionReplay::ARCode>(ini_file, game_id, "AR_RetroAchievements_Verified", true,
                                        &action_replays);
-    // Get game section from allow list
-    auto game_itr = allow_list.find(game_id);
-    bool itr_end = (game_itr == allow_list.end());
     // Iterate over approved patches
     for (const auto& patch : patches)
     {
@@ -110,8 +85,7 @@ TEST(PatchAllowlist, VerifyHashes)
         context->Update(Common::BitCastToArray<u8>(entry.conditional));
       }
       auto digest = context->Finish();
-      CheckHash(game_id, itr_end ? nullptr : &game_itr->second,
-                Common::SHA1::DigestToString(digest), patch.name);
+      approved[patch.name] = picojson::value(Common::SHA1::DigestToString(digest));
     }
     // Iterate over approved geckos
     for (const auto& code : geckos)
@@ -127,8 +101,7 @@ TEST(PatchAllowlist, VerifyHashes)
         context->Update(Common::BitCastToArray<u8>(entry.data));
       }
       auto digest = context->Finish();
-      CheckHash(game_id, itr_end ? nullptr : &game_itr->second,
-                Common::SHA1::DigestToString(digest), code.name);
+      approved[code.name] = picojson::value(Common::SHA1::DigestToString(digest));
     }
     // Iterate over approved AR codes
     for (const auto& code : action_replays)
@@ -144,27 +117,43 @@ TEST(PatchAllowlist, VerifyHashes)
         context->Update(Common::BitCastToArray<u8>(entry.value));
       }
       auto digest = context->Finish();
-      CheckHash(game_id, itr_end ? nullptr : &game_itr->second,
-                Common::SHA1::DigestToString(digest), code.name);
+      approved[code.name] = picojson::value(Common::SHA1::DigestToString(digest));
     }
-    // Report missing patches in map
-    if (itr_end)
-      continue;
-    for (auto& remaining_hashes : game_itr->second.hashes)
-    {
-      ADD_FAILURE() << "Hash in list not approved in ini." << std::endl
-                    << "Game ID: " << game_id << ":" << game_itr->second.game_title << std::endl
-                    << "Code: " << remaining_hashes.first << ":" << remaining_hashes.second;
-    }
-    //    Remove section from map
-    allow_list.erase(game_itr);
+    // Add approved patches and codes to tree
+    if (!approved.empty())
+      new_allowlist[game_id] = picojson::value(approved);
   }
-  //  Report remaining sections in map
-  for (auto& remaining_games : allow_list)
+
+  // Hash new allowlist
+  std::string new_allowlist_str = picojson::value(new_allowlist).serialize();
+  auto context = Common::SHA1::CreateContext();
+  context->Update(new_allowlist_str);
+  auto digest = context->Finish();
+  if (digest != AchievementManager::APPROVED_LIST_HASH)
   {
-    ADD_FAILURE() << "Game in list has no ini file." << std::endl
-                  << "Game ID: " << remaining_games.first << ":"
-                  << remaining_games.second.game_title;
+    ADD_FAILURE() << "Approved list hash does not match the one in AchievementMananger."
+                  << std::endl
+                  << "Please update APPROVED_LIST_HASH to the following:" << std::endl
+                  << Common::SHA1::DigestToString(digest);
+  }
+  // Compare with old allowlist
+  static constexpr std::string_view APPROVED_LIST_FILENAME = "ApprovedInis.json";
+  std::string old_allowlist;
+  std::string error;
+  const auto& list_filepath = fmt::format("{}{}{}", sys_directory, DIR_SEP, APPROVED_LIST_FILENAME);
+  if (!File::ReadFileToString(list_filepath, old_allowlist) || old_allowlist != new_allowlist_str)
+  {
+    static constexpr std::string_view NEW_APPROVED_LIST_FILENAME = "New-ApprovedInis.json";
+    const auto& new_list_filepath =
+        fmt::format("{}{}{}", sys_directory, DIR_SEP, NEW_APPROVED_LIST_FILENAME);
+    if (!JsonToFile(new_list_filepath, picojson::value(new_allowlist), false))
+    {
+      ADD_FAILURE() << "Failed to write new approved list to " << list_filepath;
+    }
+    ADD_FAILURE() << "Approved list needs to be updated. Please run this test in your" << std::endl
+                  << "local environment and copy" << std::endl
+                  << new_list_filepath << std::endl
+                  << "to Data/Sys/ApprovedInis.json to pass this test.";
   }
 }
 
@@ -202,30 +191,3 @@ void ReadVerified(const Common::IniFile& ini, const std::string& filename,
     }
   }
 }
-
-void CheckHash(const std::string& game_id, GameHashes* game_hashes, const std::string& hash,
-               const std::string& patch_name)
-{
-  // Check patch in list
-  if (game_hashes == nullptr)
-  {
-    // Report: no patches in game found in list
-    ADD_FAILURE() << "Approved hash missing from list." << std::endl
-                  << "Game ID: " << game_id << std::endl
-                  << "Code: \"" << hash << "\": \"" << patch_name << "\"";
-    return;
-  }
-  auto hash_itr = game_hashes->hashes.find(hash);
-  if (hash_itr == game_hashes->hashes.end())
-  {
-    // Report: patch not found in list
-    ADD_FAILURE() << "Approved hash missing from list." << std::endl
-                  << "Game ID: " << game_id << ":" << game_hashes->game_title << std::endl
-                  << "Code: \"" << hash << "\": \"" << patch_name << "\"";
-  }
-  else
-  {
-    // Remove patch from map if found
-    game_hashes->hashes.erase(hash_itr);
-  }
-}

Source/UnitTests/UnitTests.vcxproj

@@ -104,6 +104,7 @@
   <Import Project="$(ExternalsDir)Bochs_disasm\exports.props" />
   <Import Project="$(ExternalsDir)fmt\exports.props" />
   <Import Project="$(ExternalsDir)picojson\exports.props" />
+  <Import Project="$(ExternalsDir)rcheevos\exports.props" />
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>