Enforce no real filesystem writes during unit tests by arrayka · Pull Request #700 · microsoft/DiskANN

arrayka · 2026-01-23T08:41:37Z

Why

To avoid writes to the real filesystem in unit tests.

What

Removed PhysicalFS references from tests and replaced with VirtualStorageProvider::new_overlay()
Added vfs::PhysicalFS::new to the list of disallowed methods in clippy.toml

…geProvider::new_overlay()

diskann-disk/src/utils/aligned_file_reader/storage_provider_aligned_file_reader.rs

diskann-providers/src/model/pq/pq_construction.rs

Copilot · 2026-01-23T08:59:53Z

@arrayka I've opened a new pull request, #701, to work on those changes. Once the pull request is ready, I'll request review from you.

* Initial plan * Address PR feedback: revert comments and refactor test helper functions Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com> * Revert test_index_path() to return String and remove .to_string_lossy() Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com> * Remove test_index_os_path() and use test_index_path() instead Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com> * Revert commit 90934ad Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>

hildebrandmw · 2026-01-23T18:45:33Z

diskann-providers/benches/benchmarks/copy_aligned_data_bench.rs

 pub fn benchmark_copy_aligned_data(c: &mut Criterion) {
    let tmp_dir = TempDir::with_prefix(BENCHMARK_ID).expect("Failed to create temporary directory");
    // Use physical file system rather than memory for testing the actual disk read/write
+    #[allow(clippy::disallowed_methods)]


You can use

#[expect(clippy::disallowed_methods, "reason")]

to do two things:

expect will complain if disallowed method is not actually used (helping with maintanence)

The following reason helps document why an exception is made.

hildebrandmw · 2026-01-23T18:45:51Z

diskann-providers/benches/benchmarks_iai/copy_aligned_data_bench_iai.rs

 pub fn benchmark_copy_aligned_data_iai() {
    let tmp_dir = TempDir::with_prefix(BENCHMARK_ID).expect("Failed to create temporary directory");
    // Use physical file system rather than memory for testing the actual disk read/write
+    #[allow(clippy::disallowed_methods)]


Same comment here with expect instead of allow.

diskann-providers/src/model/pq/fixed_chunk_pq_table.rs

diskann-providers/src/storage/virtual_storage_provider.rs

@hildebrandmw

) Test files were using hardcoded workspace root paths (`env!("CARGO_MANIFEST_DIR").parent().unwrap()`) instead of the centralized `test_data_root()` helper from diskann-utils. ## Changes - **Standardized test data resolution**: Replaced all hardcoded workspace_root patterns with `test_data_root()` across 17 test functions in 7 files - **Fixed path prefixes**: Removed `/test_data/` prefix from test file paths since `test_data_root()` already resolves to the test_data directory - **Updated dependencies**: Added `testing` feature for diskann-utils in dev-dependencies ### Before ```rust let workspace_root = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) .parent() .unwrap() .to_path_buf(); let storage_provider = VirtualStorageProvider::new_overlay(workspace_root); let file = "/test_data/sift/siftsmall_learn.bin"; ``` ### After ```rust let storage_provider = VirtualStorageProvider::new_overlay(test_data_root()); let file = "/sift/siftsmall_learn.bin"; ``` Files affected: - `diskann-providers/src/index/diskann_async.rs` - `diskann-providers/src/storage/{pq_storage.rs, index_storage.rs}` - `diskann-providers/src/utils/{normalizing_util.rs, kmeans.rs}` - `diskann-providers/src/model/pq/{fixed_chunk_pq_table.rs, pq_construction.rs}`  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Use VirtualStorageProvider::new_overlay(test_data()) in tests</issue_title> > <issue_description>Update the unit tests so that any hardcoded test_data in string paths and workspace_root usages are replaced with `VirtualStorageProvider::new_overlay(test_data_root())`. > > Here is an example of expected pattern: > ``` > let storage_provider = VirtualStorageProvider::new_overlay(test_data_root()); > let dataset_file = "/sift/siftsmall_learn.bin"; > let mut file = storage_provider.open_reader(dataset_file).unwrap(); > ``` > Ensure that all unit tests still pass. > > Original comment: > > Something we should also do (maybe in another PR) is to replace all uses of `test_data` with the file system resolution in `diskann-utils`. I think a few fell through the cracks when test data was consolidated. > > _Originally posted by @hildebrandmw in [#700](https://github.com/microsoft/DiskANN/pull/700/changes#r2722426616)_</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details>  - Fixes #704  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>

@hildebrandmw

## Completed Changes - [x] Made `VirtualStorageProvider::new()` fully private (not just doc-hidden) - [x] Updated all unit tests in virtual_storage_provider.rs to use `new_memory()` - [x] Updated all usages across diskann-providers, diskann-disk, and diskann-tools to use specific constructors - [x] Added `VirtualStorageProvider::new_physical` to clippy.toml disallowed-methods list - [x] Removed all unused imports (MemoryFS, OverlayFS) - [x] Made test helper functions generic over filesystem type where needed - [x] **Fixed doc tests:** - Added missing `use std::io::Write;` import to doc examples - Removed doc examples that write to physical filesystem (new_physical and new_overlay) - Simplified to only show `new_memory()` usage - Removed unnecessary turbofish notation in test code - [x] Verified all checks pass: - ✅ cargo fmt --all --check - ✅ cargo clippy --locked --workspace --all-targets --no-deps - ✅ cargo test --doc --workspace - ✅ All unit tests pass All PR review feedback has been addressed.  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>VirtualStorageProvider: encourage use of `new_overlay` or `new_memory`</issue_title> > <issue_description>Make `VirtualStorageProvider::new` private. > Also add VirtualStorageProvider<PhysicalFS>::new_physical<P: AsRef<std::path::Path>>(path: P). > Expose only new_overlay(), new_memory(), new_physical(). > > Examples like these should be replaced with VirtualStorageProvider::new_memory(): > let fs = OverlayFS::new(&[MemoryFS::default().into()]); > let storage_provider = VirtualStorageProvider::new(fs); > > Original post: > > Do you see any value in making `VirtualStorageProvider::new` much harder to call to encourage use of `new_overlay` or `new_memory`? Maybe with like a `new_physical` and then linting that with clippy? > > _Originally posted by @hildebrandmw in [#700](https://github.com/microsoft/DiskANN/pull/700/changes#r2722436904)_</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details>  - Fixes #703  --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com> Co-authored-by: arrayka <arrayka@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Remove PhysicalFS references from tests and replace with VirtualStora…

f491f15

…geProvider::new_overlay()

arrayka requested a review from hildebrandmw January 23, 2026 08:42

arrayka changed the title ~~Remove PhysicalFS references from tests and replace with VirtualStorageProvider::new_overlay()~~ Enforce no real filesystem writes during unit tests Jan 23, 2026

arrayka commented Jan 23, 2026

View reviewed changes

Copilot AI mentioned this pull request Jan 23, 2026

Address feedback on test filesystem setup and comments #701

Merged

7 tasks

arrayka assigned Copilot Jan 23, 2026

Copilot AI and others added 3 commits January 23, 2026 01:49

Addressing comments

cb71e64

Fixing failing tests

bdcca10

hildebrandmw reviewed Jan 23, 2026

View reviewed changes

Alex Razumov (from Dev Box) added 2 commits January 23, 2026 13:48

Merge branch 'main' into u/arrayka/remove_physicalfs_from_tests

f30d9a2

Addressing comments

dd7209a

hildebrandmw approved these changes Jan 23, 2026

View reviewed changes

arrayka merged commit b7d9be9 into main Jan 23, 2026
8 checks passed

arrayka deleted the u/arrayka/remove_physicalfs_from_tests branch January 23, 2026 22:48

Copilot AI mentioned this pull request Feb 6, 2026

Use VirtualStorageProvider::new_overlay(test_data_root()) in tests #726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Enforce no real filesystem writes during unit tests#700

Enforce no real filesystem writes during unit tests#700
arrayka merged 6 commits intomainfrom
u/arrayka/remove_physicalfs_from_tests

arrayka commented Jan 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Jan 23, 2026

Uh oh!

hildebrandmw Jan 23, 2026

Uh oh!

arrayka Jan 23, 2026

Uh oh!

hildebrandmw Jan 23, 2026

Uh oh!

arrayka Jan 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

arrayka commented Jan 23, 2026

Why

What

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Jan 23, 2026

Uh oh!

hildebrandmw Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

arrayka Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

arrayka Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants