chore(xtest): replace cipherTexts caches with session-scoped fixture#442
chore(xtest): replace cipherTexts caches with session-scoped fixture#442dmihalcik-virtru merged 10 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCentralizes test encryption into a new session-scoped fixture module and registers it in pytest; increases scopes of Changes
Sequence DiagramsequenceDiagram
participant Test as Test Function
participant Factory as encrypted_tdf Factory
participant Cache as Session Cache (_encryption_cache)
participant TmpDir as tmp_dir
participant EncryptSDK as Encryption SDK
participant Disk as Filesystem
Test->>Factory: encrypted_tdf(encrypt_sdk, attr_values, target_mode, ...)
Factory->>Factory: Compute cache key (SDK id, container, mode, attrs, az, mime)
Factory->>Cache: Check key
alt Cache Hit
Cache-->>Factory: cached Path
Factory-->>Test: return cached .tdf Path
else Cache Miss
Factory->>TmpDir: build output filename (test name + short SHA1)
Factory->>EncryptSDK: encrypt(..., output_path, assert_value=az)
EncryptSDK->>Disk: write ciphertext .tdf
EncryptSDK-->>Factory: encryption complete
Factory->>Disk: verify file exists
Factory->>Cache: store Path in cache[key]
Factory-->>Test: return new .tdf Path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 40 minutes and 16 seconds.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a session-scoped memoized encryption fixture, encrypted_tdf, and a corresponding cache to replace manual caching and the do_encrypt_with helper across the test suite. This change improves test efficiency by avoiding redundant encryption operations. Feedback indicates that several tests in test_pqc.py were missed during the migration, which will cause regressions. Additionally, because the new fixture does not perform the implicit manifest validation previously handled by the removed helper, explicit validation needs to be added back to format-specific tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtest/test_pqc.py (1)
200-205:⚠️ Potential issue | 🔴 CriticalRemove leftover
cipherTextscache references — they currently break lint and test execution.Line 200 and Line 267 still use
cipherTextsafter the module cache was removed, which causesF821and blocks the suite. Refactor these two tests to useencrypted_tdflike the rest of this PR; also fix the secpmlkem_5 filename prefix (secpmlkem_3typo).🛠️ Proposed fix
def test_secpmlkem_3_roundtrip( @@ kas_url_km1: str, in_focus: set[tdfs.SDK], + encrypted_tdf: EncryptFactory, ): @@ - sample_name = f"secpmlkem_3-{encrypt_sdk}" - if sample_name in cipherTexts: - ct_file = cipherTexts[sample_name] - else: - ct_file = tmp_dir / f"{sample_name}.tdf" - cipherTexts[sample_name] = ct_file - encrypt_sdk.encrypt( - pt_file, - ct_file, - mime_type="text/plain", - container="ztdf", - attr_values=attr.value_fqns, - target_mode=tdfs.select_target_version(encrypt_sdk, decrypt_sdk), - ) + ct_file = encrypted_tdf( + encrypt_sdk, + attr_values=attr.value_fqns, + target_mode=tdfs.select_target_version(encrypt_sdk, decrypt_sdk), + ) @@ def test_secpmlkem_5_roundtrip( @@ kas_url_km1: str, in_focus: set[tdfs.SDK], + encrypted_tdf: EncryptFactory, ): @@ - sample_name = f"secpmlkem_3-{encrypt_sdk}" - if sample_name in cipherTexts: - ct_file = cipherTexts[sample_name] - else: - ct_file = tmp_dir / f"{sample_name}.tdf" - cipherTexts[sample_name] = ct_file - encrypt_sdk.encrypt( - pt_file, - ct_file, - mime_type="text/plain", - container="ztdf", - attr_values=attr.value_fqns, - target_mode=tdfs.select_target_version(encrypt_sdk, decrypt_sdk), - ) + ct_file = encrypted_tdf( + encrypt_sdk, + attr_values=attr.value_fqns, + target_mode=tdfs.select_target_version(encrypt_sdk, decrypt_sdk), + ) @@ - rt_file = tmp_dir / f"secpmlkem_3-{encrypt_sdk}-{decrypt_sdk}.untdf" + rt_file = tmp_dir / f"secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf"Also applies to: 267-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/test_pqc.py` around lines 200 - 205, Tests reference a removed module-level cache variable cipherTexts (used with sample_name and ct_file) causing F821; replace those uses with the local encrypted_tdf variable like the rest of the PR: remove any cipherTexts lookup/assignment and set ct_file (or the path passed to encrypt_sdk.encrypt) to tmp_dir / encrypted_tdf (or the existing encrypted_tdf Path) instead of using cipherTexts[sample_name]; do the same for the other occurrence around the later block (previously lines 267-272). Also fix the filename prefix typo by changing any "secpmlkem_3" used for the secpmlkem_5 test file to "secpmlkem_5" so the generated filename matches the intended test vector.
🧹 Nitpick comments (1)
xtest/fixtures/encryption.py (1)
52-54: Harden cache hits by verifying the cached ciphertext still exists before reuse.If a cached file is removed/cleaned mid-session, returning the stale
Pathcan cause unrelated downstream failures. Re-encrypt when the cached path no longer exists.Based on learnings: "When modifying test fixtures, be aware changes affect all tests using that fixture".♻️ Proposed fix
cached = _encryption_cache.get(key) - if cached is not None: + if cached is not None and cached.is_file(): return cached + if cached is not None and not cached.is_file(): + _encryption_cache.pop(key, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/fixtures/encryption.py` around lines 52 - 54, The cache lookup returns a Path stored in _encryption_cache for key but doesn’t verify the file still exists; change the branch that reads cached = _encryption_cache.get(key) to check whether cached is not None and cached.exists() (or is_file()) before returning it, and if the file is missing remove the stale entry from _encryption_cache (del or pop) so the code falls through to re-encrypt and produce a fresh Path; ensure you reference the same cache variable _encryption_cache and the lookup key to locate where to add the existence check and cache eviction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtest/fixtures/encryption.py`:
- Line 48: Update the shared encryption fixture to use the same default MIME
type as the SDK by changing the mime_type default from "text/plain" to
"application/octet-stream"; locate the fixture function in
xtest/fixtures/encryption.py and adjust the mime_type parameter default so
callers that omit mime_type keep consistent behavior with tdfs.SDK.encrypt.
---
Outside diff comments:
In `@xtest/test_pqc.py`:
- Around line 200-205: Tests reference a removed module-level cache variable
cipherTexts (used with sample_name and ct_file) causing F821; replace those uses
with the local encrypted_tdf variable like the rest of the PR: remove any
cipherTexts lookup/assignment and set ct_file (or the path passed to
encrypt_sdk.encrypt) to tmp_dir / encrypted_tdf (or the existing encrypted_tdf
Path) instead of using cipherTexts[sample_name]; do the same for the other
occurrence around the later block (previously lines 267-272). Also fix the
filename prefix typo by changing any "secpmlkem_3" used for the secpmlkem_5 test
file to "secpmlkem_5" so the generated filename matches the intended test
vector.
---
Nitpick comments:
In `@xtest/fixtures/encryption.py`:
- Around line 52-54: The cache lookup returns a Path stored in _encryption_cache
for key but doesn’t verify the file still exists; change the branch that reads
cached = _encryption_cache.get(key) to check whether cached is not None and
cached.exists() (or is_file()) before returning it, and if the file is missing
remove the stale entry from _encryption_cache (del or pop) so the code falls
through to re-encrypt and produce a fresh Path; ensure you reference the same
cache variable _encryption_cache and the lookup key to locate where to add the
existence check and cache eviction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fc6fb55-7ed7-4c25-9dfa-6ebf4b9da18d
📒 Files selected for processing (6)
xtest/conftest.pyxtest/fixtures/encryption.pyxtest/test_abac.pyxtest/test_policytypes.pyxtest/test_pqc.pyxtest/test_tdfs.py
X-Test Results✅ go-main |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtest/test_pqc.py (1)
289-291:⚠️ Potential issue | 🟡 MinorFix the output filename in the 5-roundtrip test.
test_secpmlkem_5_roundtripstill writes tosecpmlkem_3-...untdf, which can collide with the 3-roundtrip test and make failures harder to triage now that the temp directory is shared more broadly.🛠️ Suggested fix
- rt_file = tmp_dir / f"secpmlkem_3-{encrypt_sdk}-{decrypt_sdk}.untdf" + rt_file = tmp_dir / f"secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/test_pqc.py` around lines 289 - 291, The test test_secpmlkem_5_roundtrip currently writes its recovered plaintext to a filename hardcoded as "secpmlkem_3-...untdf", which collides with the 3-roundtrip test; update the rt_file construction in test_secpmlkem_5_roundtrip (the variable rt_file) to use "secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf" so each test writes to a unique output file before calling decrypt and asserting with filecmp.cmp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@xtest/test_pqc.py`:
- Around line 289-291: The test test_secpmlkem_5_roundtrip currently writes its
recovered plaintext to a filename hardcoded as "secpmlkem_3-...untdf", which
collides with the 3-roundtrip test; update the rt_file construction in
test_secpmlkem_5_roundtrip (the variable rt_file) to use
"secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf" so each test writes to a unique
output file before calling decrypt and asserting with filecmp.cmp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f23b863-dc29-4fb2-872a-e2b5b0f4ddba
📒 Files selected for processing (2)
xtest/test_pqc.pyxtest/test_tdfs.py
X-Test Results✅ js-v0.14.0 |
Four test modules each maintained a module-level cipherTexts dict and inline if-cache-hit-else-encrypt blocks at every call site (40+ blocks plus do_encrypt_with). Replace with a single auto-keyed factory fixture so pytest's own scoping owns the cache lifetime and callers don't pick scenario strings. Also fixes a latent ordering bug where manifest-shape assertions in do_encrypt_with only fired on cache miss. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two tests added in 5958b3f (test_secpmlkem_3_roundtrip and test_secpmlkem_5_roundtrip) still used the removed cipherTexts dict. Move them onto the encrypted_tdf fixture introduced in 81fcefa. Side note: the second test had a copy-paste sample_name of "secpmlkem_3-..." which would have made it cache-collide with the first under the old keying. Auto-keying by attr_values fixes that incidentally — the two tests use different attribute fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-1745-refactor
branch
from
8b448f6 to
b299d16
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtest/test_pqc.py (1)
289-291:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
secpmlkem_5artifact name.This path still uses
secpmlkem_3, so the 5-roundtrip case overwrites the 3-roundtrip output in the sharedtmp_dir. Use the matching prefix here.🛠️ Proposed fix
- rt_file = tmp_dir / f"secpmlkem_3-{encrypt_sdk}-{decrypt_sdk}.untdf" + rt_file = tmp_dir / f"secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/test_pqc.py` around lines 289 - 291, The artifact name for the 5-roundtrip case is wrong: the rt_file string uses "secpmlkem_3-…" and overwrites the 3-roundtrip output; update the rt_file construction in xtest/test_pqc.py (the line assigning rt_file in the 5-roundtrip block) to use the matching "secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf" prefix so decrypt_sdk.decrypt writes to a distinct file in tmp_dir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@xtest/test_pqc.py`:
- Around line 289-291: The artifact name for the 5-roundtrip case is wrong: the
rt_file string uses "secpmlkem_3-…" and overwrites the 3-roundtrip output;
update the rt_file construction in xtest/test_pqc.py (the line assigning rt_file
in the 5-roundtrip block) to use the matching
"secpmlkem_5-{encrypt_sdk}-{decrypt_sdk}.untdf" prefix so decrypt_sdk.decrypt
writes to a distinct file in tmp_dir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eeb03e9-3328-43bb-92bd-ad69bcbd7ace
📒 Files selected for processing (6)
xtest/conftest.pyxtest/fixtures/encryption.pyxtest/test_abac.pyxtest/test_policytypes.pyxtest/test_pqc.pyxtest/test_tdfs.py
X-Test Failure Report |
…fstrings
Avoids hand-crafted unique names by constructing rt_file as
ct_file.with_name(f"{ct_file.stem}-{decrypt_sdk}.untdf"), which is
automatically unique across all encrypt params (already encoded in
ct_file) and the decrypt sdk/version. Also removes the now-redundant
short_names variable and tmp_dir fixture parameter from test_policytypes,
and fixes a copy-paste bug where test_secpmlkem_5_roundtrip was writing
to a secpmlkem_3-named file.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
X-Test Failure Report |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
X-Test Failure Report |
X-Test Failure Report |
- Convert encrypted_tdf factory closure to EncryptFactory class with rt_file() method that includes the current test label, preventing rt_file path collisions between tests that share a cached ciphertext - Replace all ct_file.with_name()/b_file.with_name() rt_file patterns with encrypted_tdf.rt_file() across all test files; standardize test_policytypes.py from .returned to .untdf extension - Bump assertion fixtures from package to session scope to match the session-scoped tmp_dir they write into (latent path collision fix) - Fix stale module docstring in fixtures/encryption.py - Remove unused pt_file parameter from test_manifest_validity and test_manifest_validity_with_assertions - Delete dead commented-out audit_logs blocks in test_tdf_with_unbound_policy and test_tdf_with_altered_policy_binding Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
X-Test Failure Report |
X-Test Failure Report |
|
❌ The last analysis has failed. |
X-Test Results✅ go-v0.14.0 |
2 participants
Refactor to remove per-test-module encrypted-file-cache with module scoped fixture.
Why?
Each test module maintained its own
cipherTextsdictand inlined the if-cache-hit-else-encrypt blocks at every call site.
They had to repeat the same pattern and be careful to construct a unique 'scenario name', which, if not unique, would result in two entries sharing a cache entry, a cause of several hard-to-debug errors and incorrectly passing tests in the past.
What?
Replaces the
cacheobjects with a single auto-keyed factory fixture,using pytest scoping to control the cache lifetime.
By using the
pytest.requestmagic fixture for naming, we can be avoid incorrect collisions.Also, this fixes a latent ordering bug where manifest-shape assertions in
do_encrypt_withonly fired on cache miss.But also...
While I'm here, I also fixed several places where files were not getting unique names. While this won't cause any trouble with a single, sequential pass, this may cause difficulties when using x-dist or when debugging changes locally and running the same or similar tests multiple times with different options.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Summary by CodeRabbit