Add 38 C++ MSTest tests for FileLocksmith#46936
Add 38 C++ MSTest tests for FileLocksmith#46936crutkas wants to merge 5 commits intomicrosoft:mainfrom
Conversation
Path normalization (forward->backslash, UNC paths, spaces), case-insensitive path comparison including UNC paths, ProcessResult operations, output formatting, and registry constants. Split from PR microsoft#46905 (7/7) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
crutkas
added
the
Needs-Review
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
An engineer--not a computer--needs to verify each of these test cases to ensure that they are worthwhile and on-target. |
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
I read thirteen of these tests and they are, at best, useless. They are testing the test harness, they are testing C++ string equality, and they are testing no product code of value.
…g CLI tests - Deleted src/modules/FileLocksmith/UnitTests/ (27/38 tests were hollow) - Added 8 Constants validation tests to existing CLI test project - Added 3 ProcessResult struct tests - Added 3 new CLI scenario tests using existing mock infrastructure - Every test has deep comments explaining what it tests and why Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
DWORD pid -> DWORD pid{} to zero-initialize and satisfy
code analysis warning C26495 (uninitialized member variable).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Headers should include their own dependencies explicitly, not rely on a project-specific pch.h. Constants.h only needs WCHAR from Windows.h. When included from the test project, the quoted pch.h search found the wrong pch (or no pch), causing build failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
I would throw all of the new ProcessResultsTests in the trash.
| // Product code: ProcessResult.h — ProcessResult struct | ||
| // What: Validates all four fields (name, pid, user, files) round-trip correctly | ||
| // Why: find_processes_recursive() populates these; get_json()/get_text() reads them | ||
| // Risk: Field misalignment silently corrupts IPC data between CLI and UI |
There was a problem hiding this comment.
This test is unable to actually verify field alignment over the IPC channel, because it does not use an IPC channel. It is only capable of testing that two .cpp files compiled into the same .dll see the same structure definition.
However, it is not even capable of that, because only the structure definition visible to this cpp file matters inside this test.
This test boils down to...
int foo = 1;
Assert::AreEqual(1, foo);
which is... i mean. not going to prevent a regression
| pr.name = L"proc.exe"; | ||
| pr.pid = 0; | ||
| pr.user = L""; |
There was a problem hiding this comment.
this is extraneous and not related to the test condition
| pr.files = { L"A", L"B", L"C", L"D" }; | ||
| Assert::AreEqual((size_t)4, pr.files.size()); | ||
| Assert::AreEqual(std::wstring(L"C"), pr.files[2]); |
There was a problem hiding this comment.
this is a test to make sure std::vector works properly. I assume the C++ STL has 10,000 of those already, and this will not prevent a new regression in PowerToys.
3 participants
Summary
38 new TEST_METHODs covering FileLocksmith:
Split from #46905 (7/7) — see that PR for full context.
Files Changed (6)
src/modules/FileLocksmith/UnitTests/FileLocksmithTests.cpp(new)src/modules/FileLocksmith/UnitTests/UnitTests-FileLocksmith.vcxproj(new)src/modules/FileLocksmith/UnitTests/UnitTests-FileLocksmith.vcxproj.filters(new)src/modules/FileLocksmith/UnitTests/pch.cpp(new)src/modules/FileLocksmith/UnitTests/pch.h(new)PowerToys.slnx(modified — +1 project ref in /modules/FileLocksmith/Tests/)