[query-filter] Skip version update on failed parsing (#36038)

This change updates the query filter component installer to skip updating version if the json file parsing failed.

This change also adds logic to supply test only callback which gets fired after the component installer attempted to parse the json. This facilitates testing the ComponentReady functionality e2e.

Resolves brave/brave-browser#55124

components/query_filter/browser/query_filter_component_installer.cc

@@ -10,6 +10,7 @@
 #include <array>
 #include <vector>
 
+#include "base/check_is_test.h"
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -22,10 +23,14 @@
 #include "brave/components/query_filter/common/features.h"
 #include "components/component_updater/component_installer.h"
 #include "components/component_updater/component_updater_service.h"
+#include "third_party/abseil-cpp/absl/cleanup/cleanup.h"
 
 namespace {
 inline constexpr char kQueryFilterComponentName[] = "Query Filter";
 
+// IN-TEST
+base::OnceClosure* g_on_file_loaded_callback_for_testing_ = nullptr;
+
 // This is the SHA-256 of the query-filter component's public key.
 inline constexpr std::array<uint8_t, 32> kQueryFilterComponentPublicKeySHA256 =
     {0x24, 0xc3, 0xb0, 0x6e, 0x2e, 0x8c, 0xb4, 0x5b, 0xa5, 0xa9, 0xe8,
@@ -43,6 +48,14 @@ std::string ReadQueryFilterFile(const base::FilePath& path) {
 
 void OnQueryFilterFileRead(const base::Version& version,
                            const std::string& json_data) {
+  absl::Cleanup cleanup_for_test_only = []() {
+    if (g_on_file_loaded_callback_for_testing_) {
+      CHECK_IS_TEST();
+      CHECK(!g_on_file_loaded_callback_for_testing_->is_null());
+      std::move(*g_on_file_loaded_callback_for_testing_).Run();
+    }
+  };
+
   if (json_data.empty()) {
     return;
   }
@@ -51,6 +64,7 @@ void OnQueryFilterFileRead(const base::Version& version,
 
   if (!data->PopulateDataFromComponent(json_data)) {
     LOG(WARNING) << "Failed to populate data from component";
+    return;
   }
   data->UpdateVersion(version);
 }
@@ -131,6 +145,12 @@ bool QueryFilterComponentInstallerPolicy::IsBraveComponent() const {
   return true;
 }
 
+void QueryFilterComponentInstallerPolicy::
+    SetOnFileLoadedCallbackForTesting(  // IN-TEST
+        base::OnceClosure* callback) {
+  g_on_file_loaded_callback_for_testing_ = callback;
+}
+
 void RegisterQueryFilterComponent(
     component_updater::ComponentUpdateService* cus) {
   if (!base::FeatureList::IsEnabled(

components/query_filter/browser/query_filter_component_installer.h

@@ -49,6 +49,10 @@ class QueryFilterComponentInstallerPolicy
   std::string GetName() const override;
   update_client::InstallerAttributes GetInstallerAttributes() const override;
   bool IsBraveComponent() const override;
+
+  // A test only method to set a callback which would be fired when the
+  // component has finished attempting to load and parse the file.
+  void SetOnFileLoadedCallbackForTesting(base::OnceClosure* callback = nullptr);
 };
 
 // Registers the Query Filter component with the component updater.

components/query_filter/browser/query_filter_component_installer_unittest.cc

@@ -24,7 +24,6 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace {
 // Sample query filter JSON which would be written to a file during setup
 // and then read by the query filter data to prepopulate the default rules.
 constexpr char kSampleQueryFilterJson[] = R"json(
@@ -46,8 +45,6 @@ constexpr char kSampleQueryFilterJson[] = R"json(
   ]
   )json";
 
-}  // namespace
-
 class QueryFilterComponentInstallerTest : public testing::Test {
  public:
   QueryFilterComponentInstallerTest()
@@ -96,6 +93,29 @@ class QueryFilterComponentInstallerTest : public testing::Test {
       base::test::TaskEnvironment::ThreadPoolExecutionMode::DEFAULT};
 };
 
+class ScopedFileLoadedCallbackForTesting {
+ public:
+  ScopedFileLoadedCallbackForTesting(
+      component_updater::QueryFilterComponentInstallerPolicy& policy,
+      base::OnceClosure callback)
+      : policy_(policy), callback_(std::move(callback)) {
+    policy_->SetOnFileLoadedCallbackForTesting(&callback_);
+  }
+
+  ~ScopedFileLoadedCallbackForTesting() {
+    policy_->SetOnFileLoadedCallbackForTesting(nullptr);
+  }
+
+  ScopedFileLoadedCallbackForTesting(
+      const ScopedFileLoadedCallbackForTesting&) = delete;
+  ScopedFileLoadedCallbackForTesting& operator=(
+      const ScopedFileLoadedCallbackForTesting&) = delete;
+
+ private:
+  raw_ref<component_updater::QueryFilterComponentInstallerPolicy> policy_;
+  base::OnceClosure callback_;
+};
+
 // Tests covering disabled feature flag state.
 TEST(QueryFilterComponentInstallerFeatureOffTest,
      TestNoRegisterWhenFeatureDisabled) {
@@ -166,18 +186,18 @@ TEST_F(QueryFilterComponentInstallerTest, TestComponentReady) {
 
   // Initiate component ready which would load the json file and populate the
   // query filter data.
+  base::test::TestFuture<void> future;
   component_updater::QueryFilterComponentInstallerPolicy policy;
+  ScopedFileLoadedCallbackForTesting scoped(policy, future.GetCallback());
   policy.ComponentReady(version, GetInstallDirectoryPath(), base::DictValue());
+  ASSERT_TRUE(future.Wait());
 
-  // Verify the version and the rules are updated after the component is ready
-  EXPECT_TRUE(base::test::RunUntil(
-      [&]() { return GetQueryFilterVersion() == "1.0.0"; }));
+  // Verify the version and the rules are updated after the component is ready.
+  EXPECT_EQ("1.0.0", GetQueryFilterVersion());
   const auto& new_rules = GetQueryFilterRules();
   EXPECT_EQ(2U, new_rules.size());
 }
 
-// TODO(https://github.com/brave/brave-browser/issues/10188): Update this test
-// to pass a callback and remove the RunUntil.
 TEST_F(QueryFilterComponentInstallerTest,
        TestComponentReady_WithBadJson_DoesNotUpdateVersion) {
   // Test setup
@@ -193,14 +213,14 @@ TEST_F(QueryFilterComponentInstallerTest,
 
   // Initiate component ready which would load the json file and populate the
   // query filter data.
+  base::test::TestFuture<void> future;
   component_updater::QueryFilterComponentInstallerPolicy policy;
+  ScopedFileLoadedCallbackForTesting scoped(policy, future.GetCallback());
   policy.ComponentReady(version, GetInstallDirectoryPath(), base::DictValue());
+  ASSERT_TRUE(future.Wait());
 
-  // Verify the version and the rules are not updated after the component is
-  // ready as the parsing failed. This check is weak as the RunUntil may exit
-  // before the ComponentReady finished the async task inside.
-  EXPECT_TRUE(
-      base::test::RunUntil([&]() { return GetQueryFilterVersion() == ""; }));
+  // Verify neither the version not the rules are updated.
+  EXPECT_EQ("", GetQueryFilterVersion());
   const auto& new_rules = GetQueryFilterRules();
   EXPECT_TRUE(new_rules.empty());
 }