[PowerRename] Fix tests inconsistency, improve test performance (#8129)

* Move retrieveing file attibutes to PowerRenameRegex
Move file attributes unit tests to PowerRenameRegexTests
Add file time field to MockPowerRenameItem

* Add file attributes unittests to PowerRenameManagerTests

* Change variable name

* Rearrange function arguments

* Check if file attributes are used only once

* Change variable name LocalTime -> fileTime, date -> time

* Set fileTime as a member of PowerRenameRegEx rather than passing as an argument

* Change function name isFileAttributesUsed() -> isFileTimeUsed()
Check before resetting fileTime

* Fix small bugs

* Fix typos

* Refactor for readability, move free calls to reachable places

* Fix search for area empty bug
searchTerm being empty is not an invalid argument rather it must return OK without any operation
Tests must check if Replace()  returns S_OK becuase later it checks its result

* Check return values of method calls in PowerRenameManager
Remove received argments checks from some methods because argument being null or empty string doesnt mean it is invalid or method fails

* Fix formatting. Remove overlooked comment. Fix error message.

* Change HRESULT declarations according to coding style

* Fix unhandled case. Refactor.

src/modules/powerrename/unittests/MockPowerRenameItem.cpp

@@ -1,14 +1,14 @@
 #include "pch.h"
 #include "MockPowerRenameItem.h"
 
-HRESULT CMockPowerRenameItem::CreateInstance(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder, _Outptr_ IPowerRenameItem** ppItem)
+HRESULT CMockPowerRenameItem::CreateInstance(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder, _In_ SYSTEMTIME time, _Outptr_ IPowerRenameItem** ppItem)
 {
     *ppItem = nullptr;
     CMockPowerRenameItem* newItem = new CMockPowerRenameItem();
-    HRESULT hr = newItem ? S_OK : E_OUTOFMEMORY;
-    if (SUCCEEDED(hr))
+    HRESULT hr = E_OUTOFMEMORY;
+    if (newItem)
     {
-        newItem->Init(path, originalName, depth, isFolder);
+        newItem->Init(path, originalName, depth, isFolder, time);
         hr = newItem->QueryInterface(IID_PPV_ARGS(ppItem));
         newItem->Release();
     }
@@ -16,7 +16,7 @@ HRESULT CMockPowerRenameItem::CreateInstance(_In_opt_ PCWSTR path, _In_opt_ PCWS
     return hr;
 }
 
-void CMockPowerRenameItem::Init(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder)
+void CMockPowerRenameItem::Init(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder, _In_ SYSTEMTIME time)
 {
     if (path != nullptr)
     {
@@ -30,4 +30,6 @@ void CMockPowerRenameItem::Init(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalNa
 
     m_depth = depth;
     m_isFolder = isFolder;
+    m_time = time;
+    m_isTimeParsed = true;
 }

src/modules/powerrename/unittests/MockPowerRenameItem.h

@@ -7,6 +7,6 @@ class CMockPowerRenameItem :
     public CPowerRenameItem
 {
 public:
-    static HRESULT CreateInstance(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder, _Outptr_ IPowerRenameItem** ppItem);
-    void Init(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder);
-};
+    static HRESULT CreateInstance(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder, _In_ SYSTEMTIME time, _Outptr_ IPowerRenameItem** ppItem);
+    void Init(_In_opt_ PCWSTR path, _In_opt_ PCWSTR originalName, _In_ UINT depth, _In_ bool isFolder, _In_ SYSTEMTIME time);
+};

src/modules/powerrename/unittests/MockPowerRenameManagerEvents.cpp

@@ -81,8 +81,8 @@ HRESULT CMockPowerRenameManagerEvents::s_CreateInstance(_In_ IPowerRenameManager
 {
     *ppsrui = nullptr;
     CMockPowerRenameManagerEvents* events = new CMockPowerRenameManagerEvents();
-    HRESULT hr = events != nullptr ? S_OK : E_OUTOFMEMORY;
-    if (SUCCEEDED(hr))
+    HRESULT hr = E_OUTOFMEMORY;
+    if (events != nullptr)
     {
         hr = events->QueryInterface(IID_PPV_ARGS(ppsrui));
         events->Release();

src/modules/powerrename/unittests/MockPowerRenameRegExEvents.cpp

@@ -56,12 +56,18 @@ IFACEMETHODIMP CMockPowerRenameRegExEvents::OnFlagsChanged(_In_ DWORD flags)
     return S_OK;
 }
 
+IFACEMETHODIMP CMockPowerRenameRegExEvents::OnFileTimeChanged(_In_ SYSTEMTIME fileTime)
+{
+    m_fileTime = fileTime;
+    return S_OK;
+}
+
 HRESULT CMockPowerRenameRegExEvents::s_CreateInstance(_Outptr_ IPowerRenameRegExEvents** ppsrree)
 {
     *ppsrree = nullptr;
     CMockPowerRenameRegExEvents* psrree = new CMockPowerRenameRegExEvents();
-    HRESULT hr = psrree ? S_OK : E_OUTOFMEMORY;
-    if (SUCCEEDED(hr))
+    HRESULT hr = E_OUTOFMEMORY;
+    if (psrree)
     {
         hr = psrree->QueryInterface(IID_PPV_ARGS(ppsrree));
         psrree->Release();

src/modules/powerrename/unittests/MockPowerRenameRegExEvents.h

@@ -18,6 +18,7 @@ public:
     IFACEMETHODIMP OnSearchTermChanged(_In_ PCWSTR searchTerm);
     IFACEMETHODIMP OnReplaceTermChanged(_In_ PCWSTR replaceTerm);
     IFACEMETHODIMP OnFlagsChanged(_In_ DWORD flags);
+    IFACEMETHODIMP OnFileTimeChanged(_In_ SYSTEMTIME fileTime);
 
     static HRESULT s_CreateInstance(_Outptr_ IPowerRenameRegExEvents** ppsrree);
 
@@ -35,5 +36,6 @@ public:
     PWSTR m_searchTerm = nullptr;
     PWSTR m_replaceTerm = nullptr;
     DWORD m_flags = 0;
+    SYSTEMTIME m_fileTime = { 0 };
     long m_refCount;
 };

src/modules/powerrename/unittests/PowerRenameManagerTests.cpp

@@ -32,7 +32,7 @@ namespace PowerRenameManagerTests
             int depth;
         };
 
-        void RenameHelper(_In_ rename_pairs * renamePairs, _In_ int numPairs, _In_ std::wstring searchTerm, _In_ std::wstring replaceTerm, SYSTEMTIME LocalTime, _In_ DWORD flags)
+        void RenameHelper(_In_ rename_pairs * renamePairs, _In_ int numPairs, _In_ std::wstring searchTerm, _In_ std::wstring replaceTerm, SYSTEMTIME fileTime, _In_ DWORD flags)
         {
             // Create a single item (in a temp directory) and verify rename works as expected
             CTestFileHelper testFileHelper;
@@ -59,12 +59,11 @@ namespace PowerRenameManagerTests
             for (int i = 0; i < numPairs; i++)
             {
                 CComPtr<IPowerRenameItem> item;
-                CMockPowerRenameItem::CreateInstance(testFileHelper.GetFullPath(
-                                                                       renamePairs[i].originalName)
-                                                         .c_str(),
+                CMockPowerRenameItem::CreateInstance(testFileHelper.GetFullPath(renamePairs[i].originalName).c_str(),
                                                      renamePairs[i].originalName.c_str(),
                                                      renamePairs[i].depth,
                                                      !renamePairs[i].isFile,
+                                                     fileTime,
                                                      &item);
 
                 int itemId = 0;
@@ -84,20 +83,21 @@ namespace PowerRenameManagerTests
             Assert::IsTrue(mgr->GetRenameRegEx(&renRegEx) == S_OK);
             renRegEx->PutFlags(flags);
             renRegEx->PutSearchTerm(searchTerm.c_str());
-            if (isFileAttributesUsed(replaceTerm.c_str()) && SUCCEEDED(GetDatedFileName(newReplaceTerm, ARRAYSIZE(newReplaceTerm), replaceTerm.c_str(), LocalTime)))
-            {
-                renRegEx->PutReplaceTerm(newReplaceTerm);
-            }
-            else
-            {
-                renRegEx->PutReplaceTerm(replaceTerm.c_str());
-            }
-            Sleep(1000);
+            renRegEx->PutReplaceTerm(replaceTerm.c_str());
 
             // Perform the rename
-            Assert::IsTrue(mgr->Rename(0) == S_OK);
+            bool replaceSuccess = false;
+            for (int step = 0; step < 20; step++)
+            {
+                replaceSuccess = mgr->Rename(0) == S_OK;
+                if (replaceSuccess)
+                {
+                    break;
+                }
+                Sleep(10);
+            }
 
-            Sleep(1000);
+            Assert::IsTrue(replaceSuccess);
 
             // Verify the rename occurred
             for (int i = 0; i < numPairs; i++)
@@ -128,7 +128,7 @@ namespace PowerRenameManagerTests
             CComPtr<IPowerRenameManager> mgr;
             Assert::IsTrue(CPowerRenameManager::s_CreateInstance(&mgr) == S_OK);
             CComPtr<IPowerRenameItem> item;
-            CMockPowerRenameItem::CreateInstance(L"foo", L"foo", 0, false, &item);
+            CMockPowerRenameItem::CreateInstance(L"foo", L"foo", 0, false, SYSTEMTIME{0}, &item);
             mgr->AddItem(item);
             Assert::IsTrue(mgr->Shutdown() == S_OK);
         }
@@ -143,7 +143,7 @@ namespace PowerRenameManagerTests
             DWORD cookie = 0;
             Assert::IsTrue(mgr->Advise(mgrEvents, &cookie) == S_OK);
             CComPtr<IPowerRenameItem> item;
-            CMockPowerRenameItem::CreateInstance(L"foo", L"foo", 0, false, &item);
+            CMockPowerRenameItem::CreateInstance(L"foo", L"foo", 0, false, SYSTEMTIME{0}, &item);
             int itemId = 0;
             Assert::IsTrue(item->GetId(&itemId) == S_OK);
             mgr->AddItem(item);
@@ -290,6 +290,7 @@ namespace PowerRenameManagerTests
             RenameHelper(renamePairs, ARRAYSIZE(renamePairs), L"foo", L"bar", SYSTEMTIME{ 2020, 7, 3, 22, 15, 6, 42, 453 }, DEFAULT_FLAGS | Lowercase | ExtensionOnly);
         }
 
+
         TEST_METHOD (VerifyFileAttributesNoPadding)
         {
             rename_pairs renamePairs[] = {
@@ -311,26 +312,26 @@ namespace PowerRenameManagerTests
         TEST_METHOD (VerifyFileAttributesMonthandDayNames)
         {
             std::locale::global(std::locale(""));
-            SYSTEMTIME LocalTime = { 2020, 1, 3, 1, 15, 6, 42, 453 };
+            SYSTEMTIME fileTime = { 2020, 1, 3, 1, 15, 6, 42, 453 };
             wchar_t localeName[LOCALE_NAME_MAX_LENGTH];
             wchar_t result[MAX_PATH] = L"bar";
             wchar_t formattedDate[MAX_PATH];
             if (GetUserDefaultLocaleName(localeName, LOCALE_NAME_MAX_LENGTH) == 0)
                 StringCchCopy(localeName, LOCALE_NAME_MAX_LENGTH, L"en_US");
 
-            GetDateFormatEx(localeName, NULL, &LocalTime, L"MMM", formattedDate, MAX_PATH, NULL);
+            GetDateFormatEx(localeName, NULL, &fileTime, L"MMM", formattedDate, MAX_PATH, NULL);
             formattedDate[0] = towupper(formattedDate[0]);
             StringCchPrintf(result, MAX_PATH, TEXT("%s%s"), result, formattedDate);
-            
-            GetDateFormatEx(localeName, NULL, &LocalTime, L"MMMM", formattedDate, MAX_PATH, NULL);
+
+            GetDateFormatEx(localeName, NULL, &fileTime, L"MMMM", formattedDate, MAX_PATH, NULL);
             formattedDate[0] = towupper(formattedDate[0]);
             StringCchPrintf(result, MAX_PATH, TEXT("%s-%s"), result, formattedDate);
 
-            GetDateFormatEx(localeName, NULL, &LocalTime, L"ddd", formattedDate, MAX_PATH, NULL);
+            GetDateFormatEx(localeName, NULL, &fileTime, L"ddd", formattedDate, MAX_PATH, NULL);
             formattedDate[0] = towupper(formattedDate[0]);
             StringCchPrintf(result, MAX_PATH, TEXT("%s-%s"), result, formattedDate);
 
-            GetDateFormatEx(localeName, NULL, &LocalTime, L"dddd", formattedDate, MAX_PATH, NULL);
+            GetDateFormatEx(localeName, NULL, &fileTime, L"dddd", formattedDate, MAX_PATH, NULL);
             formattedDate[0] = towupper(formattedDate[0]);
             StringCchPrintf(result, MAX_PATH, TEXT("%s-%s"), result, formattedDate);
 

src/modules/powerrename/unittests/PowerRenameRegExBoostTests.cpp

@@ -59,7 +59,7 @@ TEST_METHOD(ReplaceNoSearchOrReplaceTerm)
     CComPtr<IPowerRenameRegEx> renameRegEx;
     Assert::IsTrue(CPowerRenameRegEx::s_CreateInstance(&renameRegEx) == S_OK);
     PWSTR result = nullptr;
-    Assert::IsTrue(renameRegEx->Replace(L"foobar", &result) != S_OK);
+    Assert::IsTrue(renameRegEx->Replace(L"foobar", &result) == S_OK);
     Assert::IsTrue(result == nullptr);
     CoTaskMemFree(result);
 }
@@ -427,6 +427,8 @@ TEST_METHOD(VerifyEventsFire)
     Assert::IsTrue(renameRegEx->PutFlags(flags) == S_OK);
     Assert::IsTrue(renameRegEx->PutSearchTerm(L"FOO") == S_OK);
     Assert::IsTrue(renameRegEx->PutReplaceTerm(L"BAR") == S_OK);
+    Assert::IsTrue(renameRegEx->PutFileTime(SYSTEMTIME{0}) == S_OK);
+    Assert::IsTrue(renameRegEx->ResetFileTime() == S_OK);
     Assert::IsTrue(lstrcmpi(L"FOO", mockEvents->m_searchTerm) == 0);
     Assert::IsTrue(lstrcmpi(L"BAR", mockEvents->m_replaceTerm) == 0);
     Assert::IsTrue(flags == mockEvents->m_flags);

src/modules/powerrename/unittests/PowerRenameRegExTests.cpp

@@ -53,7 +53,7 @@ TEST_METHOD(ReplaceNoSearchOrReplaceTerm)
     CComPtr<IPowerRenameRegEx> renameRegEx;
     Assert::IsTrue(CPowerRenameRegEx::s_CreateInstance(&renameRegEx) == S_OK);
     PWSTR result = nullptr;
-    Assert::IsTrue(renameRegEx->Replace(L"foobar", &result) != S_OK);
+    Assert::IsTrue(renameRegEx->Replace(L"foobar", &result) == S_OK);
     Assert::IsTrue(result == nullptr);
     CoTaskMemFree(result);
 }
@@ -369,6 +369,103 @@ TEST_METHOD(VerifyHandleCapturingGroups)
     }
 }
 
+TEST_METHOD (VerifyFileAttributesNoPadding)
+{
+    CComPtr<IPowerRenameRegEx> renameRegEx;
+    Assert::IsTrue(CPowerRenameRegEx::s_CreateInstance(&renameRegEx) == S_OK);
+    DWORD flags = MatchAllOccurences | UseRegularExpressions ;
+    SYSTEMTIME fileTime = SYSTEMTIME{ 2020, 7, 3, 22, 15, 6, 42, 453 };
+    Assert::IsTrue(renameRegEx->PutFlags(flags) == S_OK);
+
+    SearchReplaceExpected sreTable[] = {
+        //search, replace, test, result
+        { L"foo", L"bar$YY-$M-$D-$h-$m-$s-$f", L"foo", L"bar20-7-22-15-6-42-4" },
+    };
+
+    for (int i = 0; i < ARRAYSIZE(sreTable); i++)
+    {
+        PWSTR result = nullptr;
+        Assert::IsTrue(renameRegEx->PutSearchTerm(sreTable[i].search) == S_OK);
+        Assert::IsTrue(renameRegEx->PutReplaceTerm(sreTable[i].replace) == S_OK);
+        Assert::IsTrue(renameRegEx->PutFileTime(fileTime) == S_OK);
+        Assert::IsTrue(renameRegEx->Replace(sreTable[i].test, &result) == S_OK);
+        Assert::IsTrue(wcscmp(result, sreTable[i].expected) == 0);
+        CoTaskMemFree(result);
+    }
+}
+
+TEST_METHOD (VerifyFileAttributesPadding)
+{
+    CComPtr<IPowerRenameRegEx> renameRegEx;
+    Assert::IsTrue(CPowerRenameRegEx::s_CreateInstance(&renameRegEx) == S_OK);
+    DWORD flags = MatchAllOccurences | UseRegularExpressions;
+    Assert::IsTrue(renameRegEx->PutFlags(flags) == S_OK);
+    SYSTEMTIME fileTime = SYSTEMTIME{ 2020, 7, 3, 22, 15, 6, 42, 453 };
+    SearchReplaceExpected sreTable[] = {
+        //search, replace, test, result
+        { L"foo", L"bar$YYYY-$MM-$DD-$hh-$mm-$ss-$fff", L"foo", L"bar2020-07-22-15-06-42-453" },
+    };
+
+    for (int i = 0; i < ARRAYSIZE(sreTable); i++)
+    {
+        PWSTR result = nullptr;
+        Assert::IsTrue(renameRegEx->PutSearchTerm(sreTable[i].search) == S_OK);
+        Assert::IsTrue(renameRegEx->PutReplaceTerm(sreTable[i].replace) == S_OK);
+        Assert::IsTrue(renameRegEx->PutFileTime(fileTime) == S_OK);
+        Assert::IsTrue(renameRegEx->Replace(sreTable[i].test, &result) == S_OK);
+        Assert::IsTrue(wcscmp(result, sreTable[i].expected) == 0);
+        CoTaskMemFree(result);
+    }
+}
+
+TEST_METHOD (VerifyFileAttributesMonthandDayNames)
+{
+    CComPtr<IPowerRenameRegEx> renameRegEx;
+    Assert::IsTrue(CPowerRenameRegEx::s_CreateInstance(&renameRegEx) == S_OK);
+    DWORD flags = MatchAllOccurences | UseRegularExpressions;
+    Assert::IsTrue(renameRegEx->PutFlags(flags) == S_OK);
+
+    std::locale::global(std::locale(""));
+    SYSTEMTIME fileTime = { 2020, 1, 3, 1, 15, 6, 42, 453 };
+    wchar_t localeName[LOCALE_NAME_MAX_LENGTH];
+    wchar_t result[MAX_PATH] = L"bar";
+    wchar_t formattedDate[MAX_PATH];
+    if (GetUserDefaultLocaleName(localeName, LOCALE_NAME_MAX_LENGTH) == 0)
+        StringCchCopy(localeName, LOCALE_NAME_MAX_LENGTH, L"en_US");
+
+    GetDateFormatEx(localeName, NULL, &fileTime, L"MMM", formattedDate, MAX_PATH, NULL);
+    formattedDate[0] = towupper(formattedDate[0]);
+    StringCchPrintf(result, MAX_PATH, TEXT("%s%s"), result, formattedDate);
+
+    GetDateFormatEx(localeName, NULL, &fileTime, L"MMMM", formattedDate, MAX_PATH, NULL);
+    formattedDate[0] = towupper(formattedDate[0]);
+    StringCchPrintf(result, MAX_PATH, TEXT("%s-%s"), result, formattedDate);
+
+    GetDateFormatEx(localeName, NULL, &fileTime, L"ddd", formattedDate, MAX_PATH, NULL);
+    formattedDate[0] = towupper(formattedDate[0]);
+    StringCchPrintf(result, MAX_PATH, TEXT("%s-%s"), result, formattedDate);
+
+    GetDateFormatEx(localeName, NULL, &fileTime, L"dddd", formattedDate, MAX_PATH, NULL);
+    formattedDate[0] = towupper(formattedDate[0]);
+    StringCchPrintf(result, MAX_PATH, TEXT("%s-%s"), result, formattedDate);
+
+    SearchReplaceExpected sreTable[] = {
+        //search, replace, test, result
+        { L"foo", L"bar$MMM-$MMMM-$DDD-$DDDD", L"foo", result },
+    };
+
+    for (int i = 0; i < ARRAYSIZE(sreTable); i++)
+    {
+        PWSTR result = nullptr;
+        Assert::IsTrue(renameRegEx->PutSearchTerm(sreTable[i].search) == S_OK);
+        Assert::IsTrue(renameRegEx->PutReplaceTerm(sreTable[i].replace) == S_OK);
+        Assert::IsTrue(renameRegEx->PutFileTime(fileTime) == S_OK);
+        Assert::IsTrue(renameRegEx->Replace(sreTable[i].test, &result) == S_OK);
+        Assert::IsTrue(wcscmp(result, sreTable[i].expected) == 0);
+        CoTaskMemFree(result);
+    }
+}
+
 TEST_METHOD(VerifyLookbehindFails)
 {
     // Standard Library Regex Engine does not support lookbehind, thus test should fail.
@@ -406,6 +503,8 @@ TEST_METHOD(VerifyEventsFire)
     Assert::IsTrue(renameRegEx->PutFlags(flags) == S_OK);
     Assert::IsTrue(renameRegEx->PutSearchTerm(L"FOO") == S_OK);
     Assert::IsTrue(renameRegEx->PutReplaceTerm(L"BAR") == S_OK);
+    Assert::IsTrue(renameRegEx->PutFileTime(SYSTEMTIME{ 0 }) == S_OK);
+    Assert::IsTrue(renameRegEx->ResetFileTime() == S_OK);
     Assert::IsTrue(lstrcmpi(L"FOO", mockEvents->m_searchTerm) == 0);
     Assert::IsTrue(lstrcmpi(L"BAR", mockEvents->m_replaceTerm) == 0);
     Assert::IsTrue(flags == mockEvents->m_flags);