From f5b1c4460d1172a091cb8660eb213510507a88ec Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Fri, 19 Jun 2026 10:05:05 +1000 Subject: [PATCH] Fix some code coverage issues --- sdk_v2/cpp/run_coverage.ps1 | 16 ++- sdk_v2/cpp/test/CMakeLists.txt | 1 + .../cpp/test/internal_api/file_lock_test.cc | 101 ++++++++++++++++++ sdk_v2/cpp/test/sdk_api/download_test.cc | 4 +- 4 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 sdk_v2/cpp/test/internal_api/file_lock_test.cc diff --git a/sdk_v2/cpp/run_coverage.ps1 b/sdk_v2/cpp/run_coverage.ps1 index 9cf63c1c3..7c53012f2 100644 --- a/sdk_v2/cpp/run_coverage.ps1 +++ b/sdk_v2/cpp/run_coverage.ps1 @@ -121,15 +121,25 @@ Write-Host "`n=== Merging coverage reports ===" -ForegroundColor Cyan $htmlDir = Join-Path $covDir "html" $cobFile = Join-Path $covDir "coverage.xml" -# OpenCppCoverage merge: run a no-op command, feed the binary inputs, export both formats. +# OpenCppCoverage merge: feed the two binary inputs and export both formats. # HTML gives per-line browsing; Cobertura XML gives machine-readable per-line hit data # that we can union across modules (foundry_local.dll vs foundry_local_tests.exe). +# +# OpenCppCoverage requires a child program after `--`. We use the no-arg `hostname.exe` +# rather than `cmd.exe /c exit 0` because OpenCppCoverage re-quotes each trailing token, +# turning the latter into `cmd.exe /c "exit" "0"` — cmd then fails to find a command named +# "exit" and returns exit code 1, producing a misleading error in the log. A no-arg program +# sidesteps the quoting entirely. The no-op child loads no instrumentable foundry_local +# module, so OpenCppCoverage emits a benign "No modules were selected" warning for this run; +# the real coverage comes entirely from --input_coverage, so we drop that one expected line. $mergeAllArgs = $mergeArgs + @( "--export_type", "html:$htmlDir", "--export_type", "cobertura:$cobFile", - "--", "cmd.exe", "/c", "exit", "0" + "--", "hostname.exe" ) -& $opencpp @mergeAllArgs +& $opencpp @mergeAllArgs 2>&1 | + Where-Object { $_ -notmatch "No modules were selected" } | + ForEach-Object { Write-Host $_ } $indexHtml = Join-Path $htmlDir "index.html" if (-not (Test-Path $indexHtml)) { diff --git a/sdk_v2/cpp/test/CMakeLists.txt b/sdk_v2/cpp/test/CMakeLists.txt index 08e23cafc..efa1bf1c9 100644 --- a/sdk_v2/cpp/test/CMakeLists.txt +++ b/sdk_v2/cpp/test/CMakeLists.txt @@ -27,6 +27,7 @@ add_executable(foundry_local_tests internal_api/ep_detector_test.cc internal_api/exception_test.cc internal_api/execution_provider_test.cc + internal_api/file_lock_test.cc internal_api/file_uri_test.cc internal_api/genai_config_test.cc internal_api/http_retry_test.cc diff --git a/sdk_v2/cpp/test/internal_api/file_lock_test.cc b/sdk_v2/cpp/test/internal_api/file_lock_test.cc new file mode 100644 index 000000000..cafa1bab6 --- /dev/null +++ b/sdk_v2/cpp/test/internal_api/file_lock_test.cc @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +// Tests for the RAII cross-process FileLock in src/util/file_lock.{h,cc}. +// +// Windows-only by request. The contention cases rely on CreateFileW's exclusive +// share mode (dwShareMode == 0), which denies a second open of the same path even +// within the same process — giving deterministic single-process lock and timeout +// behavior. POSIX flock semantics differ, so these tests do not run there. + +#include "util/file_lock.h" + +#include + +#ifdef _WIN32 + +#include +#include +#include +#include + +namespace fs = std::filesystem; + +namespace { + +class FileLockFixture : public ::testing::Test { + protected: + void SetUp() override { + base_dir_ = fs::temp_directory_path() / + ("fl_file_lock_test_" + + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count())); + fs::create_directories(base_dir_); + } + + void TearDown() override { + std::error_code ec; + fs::remove_all(base_dir_, ec); + } + + fs::path base_dir_; +}; + +} // namespace + +// Acquiring a lock creates the lock file; releasing it leaves the file in place +// and allows the path to be locked again. +TEST_F(FileLockFixture, AcquireCreatesLockFileAndReleaseAllowsReacquire) { + const fs::path lock_path = base_dir_ / "basic.lock"; + + { + fl::FileLock lock(lock_path); + EXPECT_TRUE(fs::exists(lock_path)); + } + + // The lock file persists after release (it is not deleted on unlock). + EXPECT_TRUE(fs::exists(lock_path)); + + // Re-acquiring the now-free lock must not throw. + EXPECT_NO_THROW({ fl::FileLock relock(lock_path); }); +} + +// The constructor creates any missing parent directories for the lock file. +TEST_F(FileLockFixture, CreatesMissingParentDirectories) { + const fs::path nested = base_dir_ / "a" / "b" / "c" / "nested.lock"; + ASSERT_FALSE(fs::exists(nested.parent_path())); + + fl::FileLock lock(nested); + + EXPECT_TRUE(fs::exists(nested.parent_path())); + EXPECT_TRUE(fs::exists(nested)); +} + +// While one lock is held, a second lock on the same path times out and throws, +// and the error message identifies the contended path. +TEST_F(FileLockFixture, SecondLockTimesOutWhileFirstHeld) { + const fs::path lock_path = base_dir_ / "contended.lock"; + + fl::FileLock first(lock_path); + + // A small positive timeout forces at least one retry/sleep cycle before the + // deadline elapses, exercising the constructor's wait loop. + try { + fl::FileLock second(lock_path, /*timeout_ms=*/200); + FAIL() << "Expected FileLock to throw while the lock is already held"; + } catch (const std::runtime_error& e) { + EXPECT_NE(std::string(e.what()).find(lock_path.string()), std::string::npos); + } +} + +// Once the first lock is released, the same path can be locked again immediately. +TEST_F(FileLockFixture, SecondLockSucceedsAfterFirstReleased) { + const fs::path lock_path = base_dir_ / "sequential.lock"; + + { + fl::FileLock first(lock_path); + } // released here + + // timeout_ms == 0: the lock is free, so acquisition succeeds on the first try. + EXPECT_NO_THROW({ fl::FileLock second(lock_path, /*timeout_ms=*/0); }); +} + +#endif // _WIN32 diff --git a/sdk_v2/cpp/test/sdk_api/download_test.cc b/sdk_v2/cpp/test/sdk_api/download_test.cc index 23c6ffcc8..9ebb33d3f 100644 --- a/sdk_v2/cpp/test/sdk_api/download_test.cc +++ b/sdk_v2/cpp/test/sdk_api/download_test.cc @@ -77,7 +77,7 @@ TEST_F(DISABLED_DownloadFixture, RemoveAndRedownloadSmallestModel) { std::vector progress_values; target->Download([&progress_values](float pct) { progress_values.push_back(pct); - return true; // Continue downloading. + return 0; // 0 = continue (non-zero would cancel; see flProgressCallback contract). }); EXPECT_TRUE(target->IsCached()) @@ -122,7 +122,7 @@ TEST_F(DISABLED_DownloadFixture, DownloadAlreadyCachedModelIsNoOp) { std::vector progress_values; model->Download([&progress_values](float pct) { progress_values.push_back(pct); - return true; + return 0; // 0 = continue (non-zero would cancel; see flProgressCallback contract). }); EXPECT_TRUE(model->IsCached());