[LYN-9953] Asset Processor: Fix file watcher event handlers calling APM methods directly instead … (#7289)

* Fix file watcher event handlers calling APM methods directly instead of queuing them to run on the APM thread

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Add unit test

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Fix compile errors

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Fix compile errors

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Fix compile errors

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Fix compile errors

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Change ASSERT_ checks to EXPECT_ checks

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Refactored Test

Removed leftover direct call
Changed invokeMethod to use lambda form

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

* Fix compile error

Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com>

Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.cpp

@@ -0,0 +1,91 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include "ApplicationManagerTests.h"
+
+#include <QCoreApplication>
+#include <tests/assetmanager/MockAssetProcessorManager.h>
+#include <tests/assetmanager/MockFileProcessor.h>
+
+namespace UnitTests
+{
+    bool DatabaseLocationListener::GetAssetDatabaseLocation(AZStd::string& location)
+    {
+        location = m_databaseLocation;
+        return true;
+    }
+
+    void ApplicationManagerTest::SetUp()
+    {
+        ScopedAllocatorSetupFixture::SetUp();
+        
+        AZ::IO::Path tempDir(m_tempDir.GetDirectory());
+        m_databaseLocationListener.m_databaseLocation = (tempDir / "test_database.sqlite").Native();
+
+        // We need a QCoreApplication to run the event loop
+        int argc = 0;
+        m_coreApplication = AZStd::make_unique<QCoreApplication>(argc, nullptr);
+
+        m_applicationManager = AZStd::make_unique<MockBatchApplicationManager>(&argc, nullptr);
+        m_applicationManager->m_platformConfiguration = new AssetProcessor::PlatformConfiguration{ nullptr };
+        m_applicationManager->m_fileStateCache = AZStd::make_unique<AssetProcessor::FileStateCache>();
+
+        m_mockAPM = AZStd::make_unique<MockAssetProcessorManager>(m_applicationManager->m_platformConfiguration, nullptr);
+        m_applicationManager->m_assetProcessorManager = m_mockAPM.get();
+
+        AZStd::vector<AssetBuilderSDK::PlatformInfo> platforms;
+        m_applicationManager->m_platformConfiguration->EnablePlatform(AssetBuilderSDK::PlatformInfo{ "pc", { "tag" } });
+        m_applicationManager->m_platformConfiguration->PopulatePlatformsForScanFolder(platforms);
+        m_applicationManager->m_platformConfiguration->AddScanFolder(
+            AssetProcessor::ScanFolderInfo{ tempDir.c_str(), "test", "test", true, true, platforms });
+        
+        m_apmThread = AZStd::make_unique<QThread>(nullptr);
+        m_apmThread->setObjectName("APM Thread");
+        m_applicationManager->m_assetProcessorManager->moveToThread(m_apmThread.get());
+        m_apmThread->start();
+
+        m_fileProcessorThread = AZStd::make_unique<QThread>(nullptr);
+        m_fileProcessorThread->setObjectName("File Processor Thread");
+        auto fileProcessor = AZStd::make_unique<MockFileProcessor>(m_applicationManager->m_platformConfiguration);
+        fileProcessor->moveToThread(m_fileProcessorThread.get());
+        m_mockFileProcessor = fileProcessor.get();
+        m_applicationManager->m_fileProcessor = AZStd::move(fileProcessor); // The manager is taking ownership
+        m_fileProcessorThread->start();
+
+        auto fileWatcher = AZStd::make_unique<FileWatcher>();
+        m_fileWatcher = fileWatcher.get();
+
+        // This is what we're testing, it will set up connections between the fileWatcher and the 2 QObject handlers we'll check
+        m_applicationManager->InitFileMonitor(AZStd::move(fileWatcher)); // The manager is going to take ownership of the file watcher
+    }
+
+    void ApplicationManagerTest::TearDown()
+    {
+        m_apmThread->exit();
+        m_fileProcessorThread->exit();
+        m_mockAPM = nullptr;
+
+        ScopedAllocatorSetupFixture::TearDown();
+    }
+
+    TEST_F(ApplicationManagerTest, FileWatcherEventsTriggered_ProperlySignalledOnCorrectThread)
+    {
+        AZ::IO::Path tempDir(m_tempDir.GetDirectory());
+
+        Q_EMIT m_fileWatcher->fileAdded((tempDir / "test").c_str());
+        Q_EMIT m_fileWatcher->fileModified((tempDir / "test2").c_str());
+        Q_EMIT m_fileWatcher->fileRemoved((tempDir / "test3").c_str());
+        
+        EXPECT_TRUE(m_mockAPM->m_events[Added].WaitAndCheck()) << "APM Added event failed";
+        EXPECT_TRUE(m_mockAPM->m_events[Modified].WaitAndCheck()) << "APM Modified event failed";
+        EXPECT_TRUE(m_mockAPM->m_events[Deleted].WaitAndCheck()) << "APM Deleted event failed";
+
+        EXPECT_TRUE(m_mockFileProcessor->m_events[Added].WaitAndCheck()) << "File Processor Added event failed";
+        EXPECT_TRUE(m_mockFileProcessor->m_events[Deleted].WaitAndCheck()) << "File Processor Deleted event failed";
+    }
+}

Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.h

@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#pragma once
+
+#include <utilities/BatchApplicationManager.h>
+#include <AzCore/UnitTest/TestTypes.h>
+#include "assetmanager/MockAssetProcessorManager.h"
+#include "assetmanager/MockFileProcessor.h"
+
+namespace UnitTests
+{
+    struct MockBatchApplicationManager : BatchApplicationManager
+    {
+        using ApplicationManagerBase::InitFileMonitor;
+        using ApplicationManagerBase::m_assetProcessorManager;
+        using ApplicationManagerBase::m_fileProcessor;
+        using ApplicationManagerBase::m_fileStateCache;
+        using ApplicationManagerBase::m_platformConfiguration;
+        using BatchApplicationManager::BatchApplicationManager;
+    };
+
+    class DatabaseLocationListener : public AzToolsFramework::AssetDatabase::AssetDatabaseRequests::Bus::Handler
+    {
+    public:
+        DatabaseLocationListener()
+        {
+            BusConnect();
+        }
+        ~DatabaseLocationListener() override
+        {
+            BusDisconnect();
+        }
+
+        bool GetAssetDatabaseLocation(AZStd::string& location) override;
+
+        AZStd::string m_databaseLocation;
+    };
+
+    struct ApplicationManagerTest : ::UnitTest::ScopedAllocatorSetupFixture
+    {
+    protected:
+        void SetUp() override;
+        void TearDown() override;
+
+        AZ::Test::ScopedAutoTempDirectory m_tempDir;
+        DatabaseLocationListener m_databaseLocationListener;
+
+        AZStd::unique_ptr<QCoreApplication> m_coreApplication;
+        AZStd::unique_ptr<MockBatchApplicationManager> m_applicationManager;
+        AZStd::unique_ptr<QThread> m_apmThread;
+        AZStd::unique_ptr<QThread> m_fileProcessorThread;
+        AZStd::unique_ptr<MockAssetProcessorManager> m_mockAPM;
+
+        // These are just aliases, no need to manage/delete them
+        FileWatcher* m_fileWatcher{};
+        MockFileProcessor* m_mockFileProcessor{};
+    };
+}

Code/Tools/AssetProcessor/native/tests/AssetProcessorMessagesTests.cpp

@@ -126,18 +126,18 @@ namespace AssetProcessorMessagesTests
             m_batchApplicationManager->InitAssetProcessorManager();
 
             m_assetCatalog = AZStd::make_unique<MockAssetCatalog>(nullptr, m_batchApplicationManager->m_platformConfiguration);
-            
+
             m_batchApplicationManager->m_assetCatalog = m_assetCatalog.get();
             m_batchApplicationManager->InitRCController();
             m_batchApplicationManager->InitFileStateCache();
-            m_batchApplicationManager->InitFileMonitor();
+            m_batchApplicationManager->InitFileMonitor(AZStd::make_unique<FileWatcher>());
             m_batchApplicationManager->InitApplicationServer();
             m_batchApplicationManager->InitConnectionManager();
             // Note this must be constructed after InitConnectionManager is called since it will interact with the connection manager
             m_assetRequestHandler = new MockAssetRequestHandler();
             m_batchApplicationManager->InitAssetRequestHandler(m_assetRequestHandler);
 
-            m_batchApplicationManager->m_fileWatcher.StartWatching();
+            m_batchApplicationManager->m_fileWatcher->StartWatching();
 
             QObject::connect(m_batchApplicationManager->m_connectionManager, &ConnectionManager::ConnectionError, [](unsigned /*connId*/, QString error)
                 {
@@ -222,7 +222,7 @@ namespace AssetProcessorMessagesTests
 
             thread.join();
         }
-        
+
     protected:
 
         MockAssetRequestHandler* m_assetRequestHandler{}; // Not owned, AP will delete this pointer
@@ -246,7 +246,7 @@ namespace AssetProcessorMessagesTests
     {
         // Test that we can successfully send network messages and have them arrive for processing
         // For messages that have a response, it also verifies the response comes back
-        // Note that several harmless warnings will be triggered due to the messages not having any data set 
+        // Note that several harmless warnings will be triggered due to the messages not having any data set
         using namespace AzFramework::AssetSystem;
         using namespace AzToolsFramework::AssetSystem;
 

Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.cpp

@@ -12,10 +12,12 @@
 
 namespace UnitTests
 {
+    using AssetFileInfo = AssetProcessor::AssetFileInfo;
+
     void FileStateCacheTests::SetUp()
     {
         m_temporarySourceDir = QDir(m_temporaryDir.path());
-        m_fileStateCache = AZStd::make_unique<FileStateCache>();
+        m_fileStateCache = AZStd::make_unique<AssetProcessor::FileStateCache>();
     }
 
     void FileStateCacheTests::TearDown()
@@ -26,9 +28,9 @@ namespace UnitTests
     void FileStateCacheTests::CheckForFile(QString path, bool shouldExist)
     {
         bool exists = false;
-        FileStateInfo fileInfo;
+        AssetProcessor::FileStateInfo fileInfo;
 
-        auto* fileStateInterface = AZ::Interface<IFileStateRequests>::Get();
+        auto* fileStateInterface = AZ::Interface<AssetProcessor::IFileStateRequests>::Get();
 
         ASSERT_NE(fileStateInterface, nullptr);
         exists = fileStateInterface->Exists(path);
@@ -142,7 +144,7 @@ namespace UnitTests
     TEST_F(FileStateCacheTests, PassthroughTest)
     {
         m_fileStateCache = nullptr; // Need to release the existing one first since only one handler can exist for the ebus
-        m_fileStateCache = AZStd::make_unique<FileStatePassthrough>();
+        m_fileStateCache = AZStd::make_unique<AssetProcessor::FileStatePassthrough>();
         QString testPath = m_temporarySourceDir.absoluteFilePath("test.txt");
 
         CheckForFile(testPath, false);

Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.h

@@ -16,7 +16,6 @@ namespace UnitTests
 {
     using namespace testing;
     using ::testing::NiceMock;
-    using namespace AssetProcessor;
 
     class FileStateCacheTests : public ::testing::Test
     {
@@ -29,6 +28,6 @@ namespace UnitTests
     protected:
         QTemporaryDir m_temporaryDir;
         QDir m_temporarySourceDir;
-        AZStd::unique_ptr<FileStateBase> m_fileStateCache;
+        AZStd::unique_ptr<AssetProcessor::FileStateBase> m_fileStateCache;
     };
 }

Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.cpp

@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include <tests/assetmanager/MockAssetProcessorManager.h>
+
+namespace UnitTests
+{
+    void MockAssetProcessorManager::AssessAddedFile(QString filePath)
+    {
+        m_events[TestEvents::Added].Signal();
+    }
+
+    void MockAssetProcessorManager::AssessModifiedFile(QString filePath)
+    {
+        m_events[TestEvents::Modified].Signal();
+    }
+
+    void MockAssetProcessorManager::AssessDeletedFile(QString filePath)
+    {
+        m_events[TestEvents::Deleted].Signal();
+    }
+}

Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.h

@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#pragma once
+
+#include <QObject>
+#include <native/AssetManager/assetProcessorManager.h>
+#include <tests/assetmanager/TestEventSignal.h>
+
+namespace UnitTests
+{
+    struct MockAssetProcessorManager : ::AssetProcessor::AssetProcessorManager
+    {
+        Q_OBJECT
+
+        using AssetProcessorManager::AssetProcessorManager;
+
+    public Q_SLOTS:
+        void AssessAddedFile(QString filePath) override;
+        void AssessModifiedFile(QString filePath) override;
+        void AssessDeletedFile(QString filePath) override;
+
+    public:
+
+        TestEventPair m_events[TestEvents::NumEvents];
+    };
+}

Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.cpp

@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include "MockFileProcessor.h"
+
+namespace UnitTests
+{
+     void MockFileProcessor::AssessAddedFile(QString fileName)
+    {
+        m_events[TestEvents::Added].Signal();
+    }
+
+     void MockFileProcessor::AssessDeletedFile(QString fileName)
+    {
+        m_events[TestEvents::Deleted].Signal();
+    }
+}

Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.h

@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#pragma once
+
+#include <QObject>
+#include <FileProcessor/FileProcessor.h>
+#include <tests/assetmanager/TestEventSignal.h>
+
+namespace UnitTests
+{
+    struct MockFileProcessor : ::AssetProcessor::FileProcessor
+    {
+        Q_OBJECT
+
+        using FileProcessor::FileProcessor;
+
+    public Q_SLOTS:
+        void AssessAddedFile(QString fileName) override;
+        void AssessDeletedFile(QString fileName) override;
+
+    public:
+        TestEventPair m_events[TestEvents::NumEvents];
+    };
+}

Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.cpp

@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include <native/tests/assetmanager/TestEventSignal.h>
+#include <AzCore/std/parallel/thread.h>
+#include <AzCore/UnitTest/TestTypes.h>
+
+namespace UnitTests
+{
+    void TestEventPair::Signal()
+    {
+        bool expected = false;
+        ASSERT_TRUE(m_signaled.compare_exchange_strong(expected, true));
+        ASSERT_EQ(m_threadId, AZStd::thread_id{});
+        m_threadId = AZStd::this_thread::get_id();
+        m_event.release();
+    }
+
+    bool TestEventPair::WaitAndCheck()
+    {
+        constexpr int MaxWaitTimeMilliseconds = 100;
+
+        auto thisThreadId = AZStd::this_thread::get_id();
+        bool acquireSuccess = m_event.try_acquire_for(AZStd::chrono::milliseconds(MaxWaitTimeMilliseconds));
+
+        EXPECT_TRUE(acquireSuccess);
+        EXPECT_NE(m_threadId, AZStd::thread_id{});
+        EXPECT_TRUE(m_threadId != thisThreadId);
+
+        return acquireSuccess && m_threadId != AZStd::thread_id{} && m_threadId != thisThreadId;
+    }
+}

Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.h

@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#pragma once
+
+#include <AzCore/std/parallel/binary_semaphore.h>
+
+namespace UnitTests
+{
+    class TestEventPair
+    {
+        AZStd::atomic_bool m_signaled{ false };
+        AZStd::thread_id m_threadId;
+        AZStd::binary_semaphore m_event;
+
+    public:
+        void Signal();
+        bool WaitAndCheck();
+    };
+
+    enum TestEvents : int
+    {
+        Added = 0,
+        Modified,
+        Deleted,
+        NumEvents
+    };
+}