feat(xtest): DSPX-1716 Optimize tests #347
Conversation
Summary of ChangesHello @ImDevinC, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the performance and maintainability of the test suite by introducing parallel execution capabilities and optimizing fixture management. The changes aim to drastically reduce test execution times, making the development feedback loop faster and more efficient. It also lays a robust foundation for future test optimizations and provides comprehensive documentation of the strategies employed. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 significant performance optimizations to the test suite by enabling parallel execution with pytest-xdist and promoting many pytest fixtures to session scope. The changes are well-documented with detailed analysis and planning documents. My review focuses on ensuring the thread-safety of the parallel execution and the correctness of the new documentation. I've found a couple of race conditions in the caching logic that could lead to redundant work, and some inaccuracies in the documentation. Overall, this is a great improvement, and with a few adjustments, it will be a solid foundation for faster test cycles.
There was a problem hiding this comment.
Leaving this file in the PR to showcase how I approached this situation. My original prompt to initiate this was
This folder is a set of python tests that can be run by running pytest with some arguments. Specifically, the full command is DEFAULT_TAGS="main lts" SCHEMA_FILE=manifest.schema.json PLATFORM_DIR=../../opentdf-platform OTDFCTL_HEADS='["main"]' sdk_repo=opentdf/java-sdk FOCUS_SDK=all PLATFORM_VERSION=0.10.0 pytest -ra -v test_tdfs.py test_policytypes.py . These tests can take a long time though, sometimes upwards of 30 minutes. I know it's because there are lot of tests in the test suite, but I'm wondering if it's possible to refactor these tests so they can run more efficiently. Please review the tests, and provide a list of suggestions that could be used to improve test performance. Do not make any code changes yet, just create a plan that we can implement later. Please save this plan into this repository as PLAN.md so that we can refer to it later
There was a problem hiding this comment.
Leaving this file in the PR to showcase I approached the situation. After creating PLAN.md, I followed it with this prompt (I had restarted my session, hence the first line)
This is a repository that is used for running a suite of python tests with pytest. The specific command to run the test is
SCHEMA_FILE=manifest.schema.json FOCUS_SDK=all PLATFORM_VERSION=0.10.0 pytest --collect-only test_tdfs.py test_policytypes.py. This test suite can take a long time, sometimes upwards of 30 minutes. In a previous session, we created a file named PLAN.md that has a plan of action to try and improve the tests performance. Please review this plan, and break the plan down into subtasks that can be completed in chunks. Do not begin work on the tasks yet, but rather save the the tasks to a file named TASKS.md as a checklist so that we can easily understand how far we've made it into the process.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ImDevinC
force-pushed
the
test-improvements
branch
from
3187955 to
dbeb4ef
Compare
|
X-Test Failure Results✅ go-v0.4.34 |
| cipherTexts: dict[str, Path] = {} | ||
| cipherTexts_lock = threading.Lock() | ||
| counter = 0 | ||
| counter_lock = threading.Lock() |
There was a problem hiding this comment.
These should probably be abstracted into someting useful.
Ideally you could do something like:
cipherTexts = WriteThroughCache()here, which would let you do someting like
with element_setter = cipherTexts.lock(scenario):
ct_file = tmp_dir / f"test-{encrypt_sdk}-{scenario}{c}.{container}"
...update ct_file...
element_setter(ct_file)With the idea that you can't hold the lock for an element_setter and the cipherTexts object at the same time
| - Strategy: | ||
| - **Smoke tests**: One version combination per SDK (reduce 80% of tests) | ||
| - **Compatibility matrix**: Weekly/nightly runs for full cross-version | ||
| - **Focus mode**: Already exists (conftest.py:133-143) - promote usage |
There was a problem hiding this comment.
Enhance focus mode to allow focusing on sdk+version, instead of just sdk
There was a problem hiding this comment.
This plan is primarily focused on performance improvements, not necessarily functionality enhancements. Although definitely something we could separately
| - Current: All SDK combinations ([email protected], go@main, [email protected], js@main, java variants) | ||
| - Strategy: | ||
| - **Smoke tests**: One version combination per SDK (reduce 80% of tests) | ||
| - **Compatibility matrix**: Weekly/nightly runs for full cross-version |
There was a problem hiding this comment.
Optional addition: 'full regression' mode, perhaps runs on release branches, perhaps only on release branches of opentdf/service?
There was a problem hiding this comment.
This plan is primarily focused on performance improvements, not necessarily functionality enhancements. Although definitely something we could separately
| @pytest.mark.decryption # Tests decrypt operations | ||
| @pytest.mark.abac # Policy tests | ||
| @pytest.mark.integration # Full SDK integration | ||
| @pytest.mark.unit # Isolated logic |
There was a problem hiding this comment.
This could be all of test_nano.py.
| - Sequential: 108.91s for 100 tests | ||
| - Parallel: 36.56s for 100 tests | ||
|
|
||
| ### 1.1 Parallel Execution Setup | ||
| - [x] Add `pytest-xdist` to requirements.txt | ||
| - [x] Install pytest-xdist in virtual environment | ||
| - [x] Test basic parallel execution with `pytest -n auto` | ||
| - [x] Verify tests pass with parallel execution (all 240 smoke tests pass) | ||
| - [x] Benchmark performance improvement | ||
| - **Sequential:** 107.57s for 100 smoke+roundtrip tests (FOCUS_SDK=go@main) | ||
| - **Parallel (14 workers):** 37.95s for same tests | ||
| - **Speedup:** 2.8x faster (65% reduction in execution time) | ||
| - **Full smoke suite:** 240 tests pass in 167.01s with parallel execution | ||
|
|
There was a problem hiding this comment.
Are these numbers for real?
There was a problem hiding this comment.
Yes, running locally, although this only minimal test results, not a full test suite
| **Impact: MEDIUM** | ||
| - Current: Tests run against multiple containers (nano, ztdf, nano-with-ecdsa, ztdf-ecwrap) | ||
| - Strategy: | ||
| - Default runs: Single container format |
There was a problem hiding this comment.
I'd suggest keeping nano and ztdf, and leaving the ztdf-with-ecwrap and nano-with-ecdsa as the extended ones, instead of just doing all ztdf.
|
|
||
| ### 4.2 Platform API Call Batching | ||
| **Impact: MEDIUM** | ||
| - Group attribute/namespace creation into batch operations |
There was a problem hiding this comment.
Could we perhaps make these fixtures use async calls? I think largely 4 is the most complex of the changes suggested in the plan
|
|
||
| markers = | ||
| fast: Fast tests (< 1 second) | ||
| slow: Slow tests (> 5 seconds) |
There was a problem hiding this comment.
Do these measurements include fixture setup time?
Commit 1: 68c34f4 - Refactor tests with pytest-xdist
Purpose
This commit introduces parallel test execution using pytest-xdist to dramatically improve test suite performance, achieving a 2.8x speedup (65% reduction in execution time) from ~108 seconds
to ~38 seconds for 100 tests.
Key Changes
• Added pytest-xdist==3.6.1 to requirements.txt enabling concurrent test execution across CPU cores
• Created pytest.ini configuration file with test markers, discovery settings, and strict marker enforcement
• Added .venv/ to .gitignore for local development isolation
• Added threading.Lock() to global cipherTexts cache in both test_tdfs.py and test_policytypes.py
• Modified tmp_dir fixture in conftest.py to use worker-specific directories (tmp/gw0/, tmp/gw1/, etc.) preventing file conflicts between parallel workers
• Implemented proper lock acquisition patterns around cache reads/writes to prevent race conditions
• Defined 8 pytest markers in pytest.ini: fast, slow, smoke, encryption, decryption, abac, integration, roundtrip
• Tagged critical path tests with @pytest.mark.smoke for quick validation runs
• Tagged domain-specific tests (ABAC, encryption, decryption, integration) for targeted test execution
• Applied multiple markers to test_policytypes.py tests for flexible test selection
• Created comprehensive PLAN.md (301 lines) detailing:
• Performance analysis identifying 696 total tests with ~30-minute baseline runtime
• 6-phase optimization roadmap with effort estimates and expected improvements
• Detailed bottleneck analysis (subprocess calls, sequential execution, fixture scoping)
• Implementation strategy targeting 70%+ improvement in Phases 1-3
• Created TASKS.md (267 lines) tracking optimization work across 6 phases
• Updated README.md with extensive test execution examples including parallel execution, marker-based selection, SDK filtering, and container format options
• Sequential execution: 107.57s for 100 tests (FOCUS_SDK=go@main)
• Parallel execution (14 workers): 37.95s for same tests
• Full smoke suite: 240 tests pass in 167.01s with parallel execution
• Efficiency: 7.2x CPU utilization with 14 workers
Impact
This refactoring establishes the foundation for efficient test development by enabling developers to run targeted test subsets (smoke tests, specific SDKs, specific markers) and leverage
multi-core execution. The infrastructure supports future optimizations while maintaining test stability and worker isolation.
Commit 2: dbeb4ef - Fixture optimization
Purpose
This commit optimizes pytest fixture scopes to reduce setup/teardown overhead by promoting 14 fixtures from module-scope to session-scope, eliminating redundant platform API calls and file I/O
operations while maintaining test isolation and parallel execution safety.
Key Changes
• Core Infrastructure: Promoted otdfctl (line 193), hs256_key (line 855), rs256_keys (line 860) - global singletons with no side effects
• KAS Entries (5 fixtures): Promoted kas_entry_default, kas_entry_value1, kas_entry_value2, kas_entry_attr, kas_entry_ns (lines 292, 306, 320, 334, 348) - uses idempotent create_if_not_present
API, safe for shared state
• Public Keys (2 fixtures): Promoted public_key_kas_default_kid_r1 and public_key_kas_default_kid_e1 (lines 368, 379) - creates public key entries once per test run
• Assertion Files (3 fixtures): Promoted assertion_file_no_keys, assertion_file_rs_and_hs_keys, assertion_verification_file_rs_and_hs_keys (lines 898, 918, 974) - static files in
worker-isolated directories
• Subject Condition Set: Promoted otdf_client_scs (line 996) - idempotent platform API call
• Created FIXTURE_ANALYSIS.md (206 lines) providing comprehensive analysis of all 39 fixtures (32 module-scoped, 7 session-scoped)⚠️ ⚠️
• Categorized fixtures into 5 groups:
• Category 1: Read-only/immutable (safe for session scope) ✅
• Category 2: Worker-isolated (safe for session scope) ✅
• Category 3: Platform state creation (requires careful analysis)
• Category 4: Attribute creation (blocked by namespace dependency)
• Category 5: Assertion files (worker-isolated, safe) ✅
• Documented promotion plan with Phase 2.2a (14 fixtures completed) and Phase 2.2b (11 attribute fixtures deferred pending namespace strategy decision)
• Created PHASE2_SUMMARY.md (298 lines) documenting:
• Complete breakdown of all Phase 2 tasks (2.1-2.5) with completion status
• Detailed rationale for each promoted fixture
• Analysis showing existing caching, lazy loading, and pre-generation are already optimal
• Performance results: 171.82s for 240 smoke tests with 14 workers
• Risk analysis and mitigation strategies
• Recommendations for Phase 2.2b decision (promote temporary_namespace + 11 attribute fixtures for 10-15% additional improvement vs keeping module-scope for better isolation)
• Updated TASKS.md to mark Phase 2 tasks complete
• Added detailed completion notes for subtasks 2.1-2.5
• Documented findings showing caching (2.3), lazy loading (2.4), and pre-generation (2.5) are already well-implemented and require no changes
• Flagged Phase 2.2b as deferred pending user decision on namespace strategy
Technical Rationale
Why These Fixtures Are Safe:
• Immutable fixtures (keys, static files): No state mutation between tests
• Idempotent platform operations (KAS entries, public keys): Using create_if_not_present API
• Worker-isolated artifacts (tmp_dir, assertion files): Each worker operates in separate directory
• Session-level caching (otdfctl): Reduces subprocess overhead for CLI tool initialization
Why Attribute Fixtures Were Deferred: All 11 attribute-related fixtures depend on temporary_namespace (line 199), which is currently module-scoped. Promoting attributes to session-scope
without also promoting the namespace would cause scope mismatch errors.