Skip to content

test(file_type): add URL-form identity regression tests (ITL-474)#211

Merged
eywalker merged 8 commits into
mainfrom
eywalker/itl-474-opfile-audit-url-form-identity-preservation-for-engmfs
Jul 7, 2026
Merged

test(file_type): add URL-form identity regression tests (ITL-474)#211
eywalker merged 8 commits into
mainfrom
eywalker/itl-474-opfile-audit-url-form-identity-preservation-for-engmfs

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Audit result for ITL-474: op.File already preserves URL-form identity correctly at every layer — no production code changes needed.

This PR adds regression tests that lock in that behaviour:

  • TestURLFormIdentity (5 tests) in tests/test_extension_types/test_file_type.py: covers str(), hash() stability, hash equals hash((protocol, vfspath)), LogicalFile.python_to_storage() encoding, and LogicalFile round-trip.
  • test_cache_key_preserves_url_form in tests/test_hashing/test_file_hashers.py: confirms CachedFileHasher does not resolve URL-form paths to concrete backend paths when building the FileHashKey.

Both test classes use memory:// (always-available fsspec protocol) as a stand-in for engm://.

Closes ITL-474

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds regression coverage for ITL-474 to ensure op.File (UPath-based) preserves URL-form identity for non-local fsspec protocols (using memory:// as a stand-in), and documents the audit findings and test plan.

Changes:

  • Add TestURLFormIdentity to validate File string/hash() behavior and LogicalFile serialization round-trips for URL-form paths.
  • Add a CachedFileHasher regression test to ensure the cache key retains the original URL-form path.
  • Add supporting design/spec and implementation plan documents for the audit and tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
tests/test_extension_types/test_file_type.py Adds memory://-backed regression tests for URL-form identity across File and LogicalFile.
tests/test_hashing/test_file_hashers.py Adds a regression test ensuring CachedFileHasher cache keys preserve URL-form paths.
superpowers/specs/2026-07-07-itl-474-opfile-url-form-identity-design.md Records audit findings and specifies the regression tests added in this PR.
docs/metamorphic/plans/2026-07-07-itl-474-opfile-url-form-identity.md Implementation plan documenting how to add/verify the regression tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_extension_types/test_file_type.py
Comment on lines +214 to +223
def test_hash_equals_upath_protocol_tuple(self):
# UPath.__hash__ = hash((protocol, vfspath))
# For memory://ns/x.bin: protocol="memory", vfspath="/ns/x.bin"
# This test pins the exact hash contract so any regression is immediately visible.
expected = hash(("memory", "/ns/x.bin"))
actual = hash(File("memory://ns/x.bin"))
assert actual == expected, (
f"hash(File('memory://ns/x.bin')) should equal hash(('memory', '/ns/x.bin')), "
f"got {actual} vs {expected}"
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — replaced the hard-coded hash(("memory", "/ns/x.bin")) with hash(UPath("memory://ns/x.bin")). The test now derives the expected value directly from UPath, so it stays focused on verifying that File follows the same hash contract as its wrapped UPath rather than asserting against a specific internal representation.

Comment thread tests/test_hashing/test_file_hashers.py Outdated
Comment on lines +289 to +296
cached.hash_file(UPath("memory://ns/cache_key_test.bin"))

keys = list(cacher._cache.keys())
assert len(keys) == 1, f"Expected 1 cache entry, got {len(keys)}"
key = keys[0]
assert str(key.path) == "memory://ns/cache_key_test.bin", (
f"Cache key path must preserve URL form; got {str(key.path)!r}"
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — removed the cacher._cache inspection. The test now reconstructs the expected FileHashKey using url_path.resolve() + stat() (mirroring exactly what CachedFileHasher does internally), then verifies a cache hit via the public cacher.get() API. The URL-form assertion then checks str(expected_key.path) on that reconstructed key, which confirms the key path was not resolved to a concrete backend path.

Comment on lines +92 to +96
| Test class | File | What it covers |
|---|---|---|
| `TestURLFormIdentity` | `tests/test_extension_types/test_file_type.py` | `str()`, `hash()`, `LogicalFile` round-trip |
| `TestCachedFileHasherURLKey` (new method in existing class) | `tests/test_hashing/test_file_hashers.py` | `CachedFileHasher` cache-key URL preservation |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — updated the table entry to reference the actual test: TestCachedFileHasher.test_cache_key_preserves_url_form.

Comment on lines +115 to +117
| Test | Assertion |
|---|---|
| `test_cache_key_preserves_url_form` | After hashing `memory://ns/x.bin`, the `FileHashKey.path` in the cache has `str(key.path) == "memory://ns/x.bin"` |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — updated the example path to memory://ns/cache_key_test.bin to match the implemented test.

- Replace fs.store membership checks with fs.exists() public API in
  TestURLFormIdentity fixture (avoids fsspec internals)
- Derive expected hash from UPath directly in
  test_hash_equals_upath_protocol_tuple (no hard-coded vfspath string)
- Replace cacher._cache inspection with FileHashKey reconstruction +
  cacher.get() in test_cache_key_preserves_url_form (uses public API)
- Fix spec doc: correct TestCachedFileHasher test name and example path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary

All 5 Copilot comments addressed in commit ed3f1d3.

Comment Change
Fixture uses fs.store (internal attribute) Replaced both if "/ns/x.bin" in fs.store: guards with if fs.exists("/ns/x.bin"): — uses the public fsspec API
test_hash_equals_upath_protocol_tuple hard-codes "/ns/x.bin" Replaced hash(("memory", "/ns/x.bin")) with hash(UPath("memory://ns/x.bin")) — derives expected value from UPath directly
test_cache_key_preserves_url_form inspects cacher._cache Replaced private-dict inspection with FileHashKey reconstruction + cacher.get() public API
Spec table names the test TestCachedFileHasherURLKey Updated to TestCachedFileHasher.test_cache_key_preserves_url_form
Spec shows memory://ns/x.bin for the cache-key test Updated to memory://ns/cache_key_test.bin to match the implemented test

All 6 affected tests still pass.

@eywalker eywalker merged commit f60f49d into main Jul 7, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-474-opfile-audit-url-form-identity-preservation-for-engmfs branch July 7, 2026 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants