Fix flaky process model tests wrt data stores

[jbirddog]

The test runner was not initializing thread local data like the process instance processor does, so if the current thread local data did not have a process model identifier that allowed the data store's location to be resolved the test would fail. This is why you'd see something like the reported ~70% fail rates on a prod box - likely 1/4 worker threads was initialized as needed.

Also added the kkv getters into task data when using the test runner.

Summary by CodeRabbit

• Refactor
  • Improved test runner initialization and simplified its public API for clearer usage.
  • Enhanced thread-local context handling during test runs for better isolation and reliability.
  • Integrated runtime data-store support to enrich task execution within process model tests.
  • Removed a legacy service wrapper to streamline test-run orchestration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a required first parameter process_model_identifier to ProcessModelTestRunner, stores it on the instance and places it into Flask thread-local context during runs. Removes ProcessModelTestRunnerService. Integrates KKVDataStore.add_data_store_getters_to_spiff_task into delegate task execution. Tests updated to pass an identifier.

Changes

Cohort / File(s)	Summary
Process Model Test Runner Service spiffworkflow_backend/services/process_model_test_runner_service.py	ProcessModelTestRunner.__init__ now requires process_model_identifier: str (stored on instance). ProcessModelTestRunner.run assigns the identifier into thread-local context via Flask current_app when available. KKVDataStore.add_data_store_getters_to_spiff_task is invoked in delegate task execution. The ProcessModelTestRunnerService class was removed.
Process Model Controller spiffworkflow_backend/routes/process_models_controller.py	Call sites updated: ProcessModelTestRunner is instantiated with process_model_identifier as the first positional argument.
Delegates / Task Execution ...process_model_test_runner_service.py (delegate classes)	ProcessModelTestRunnerMostlyPureSpiffDelegate.execute_task updated to call KKVDataStore.add_data_store_getters_to_spiff_task(spiff_task) before executing the task and to use thread-local data where provided.
Tests spiffworkflow_backend/tests/spiffworkflow_backend/unit/test_process_model_test_generator_service.py, spiffworkflow_backend/tests/spiffworkflow_backend/unit/test_process_model_test_runner.py	Updated ProcessModelTestRunner constructor calls to include an additional first positional argument (empty string "" in tests) to match the new signature.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Controller
    participant ProcessModelTestRunner
    participant current_app as Flask context
    participant Delegate
    participant KKVDataStore
    participant SpiffTask

    Controller->>ProcessModelTestRunner: instantiate(process_model_identifier, ...)
    Controller->>ProcessModelTestRunner: run()
    ProcessModelTestRunner->>current_app: set thread-local/process_model_identifier
    ProcessModelTestRunner->>Delegate: delegate.execute_task(spiff_task)
    Delegate->>KKVDataStore: add_data_store_getters_to_spiff_task(spiff_task)
    KKVDataStore-->>Delegate: getters attached to spiff_task
    Delegate->>SpiffTask: execute (with datastore getters & thread-local available)
    SpiffTask-->>Delegate: result
    Delegate-->>ProcessModelTestRunner: task result
    ProcessModelTestRunner-->>Controller: run result

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Review thread-local usage via current_app: ensure operations occur inside an application context and consider concurrency implications.
• Verify removal of ProcessModelTestRunnerService does not break external callers or imports.
• Inspect KKVDataStore.add_data_store_getters_to_spiff_task usage to confirm it safely mutates/augments SpiffTask and that lifecycle/cleanup expectations are met.
• Check all updated test constructor calls use intentional identifiers (empty string in tests may hide issues).

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'Fix flaky process model tests wrt data stores' accurately summarizes the main change: fixing flaky tests by ensuring thread-local data and data store initialization in the test runner.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch dstest

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d564f35 and b40e58c.

📒 Files selected for processing (4)

• spiffworkflow-backend/src/spiffworkflow_backend/routes/process_models_controller.py (1 hunks)
• spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_test_runner_service.py (5 hunks)
• spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_model_test_generator_service.py (1 hunks)
• spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_model_test_runner.py (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_model_test_runner.py
• spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_model_test_generator_service.py
• spiffworkflow-backend/src/spiffworkflow_backend/routes/process_models_controller.py

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-11-25T14:52:05.543Z

Learnt from: CR
Repo: sartography/spiff-arena PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T14:52:05.543Z
Learning: Run a single backend test file using: cd [backend_dir] && poet [test_file_path] (e.g., cd ~/projects/github/sartography/spiff-arena/spiffworkflow-backend && poet tests/spiffworkflow_backend/integration/test_process_model_milestones.py)

Applied to files:

• spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_test_runner_service.py

🧬 Code graph analysis (1)

spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_test_runner_service.py (1)

spiffworkflow-backend/src/spiffworkflow_backend/data_stores/kkv.py (3)

• KKVDataStore (15-205)
• add_data_store_getters_to_spiff_task (69-79)
• get (81-113)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

• GitHub Check: build
• GitHub Check: tests 3.12 / ubuntu-latest sqlite
• GitHub Check: tests 3.10 / ubuntu-latest sqlite
• GitHub Check: tests 3.11 / ubuntu-latest sqlite
• GitHub Check: typeguard 3.11 / ubuntu-latest sqlite
• GitHub Check: typeguard 3.12 / ubuntu-latest sqlite
• GitHub Check: tests 3.11 / macos-latest sqlite
• GitHub Check: tests 3.12 / ubuntu-latest mysql
• GitHub Check: tests 3.12 / ubuntu-latest postgres
• GitHub Check: tests 3.11 / ubuntu-latest postgres
• GitHub Check: tests 3.13.7 / ubuntu-latest mysql
• GitHub Check: tests 3.11 / ubuntu-latest mysql

🔇 Additional comments (4)

spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_test_runner_service.py (4)

18-18: LGTM: Imports support thread-local data and data store integration.

The new imports are used correctly to fix the thread-local data initialization issue and integrate KKV data store getters.

Also applies to: 28-28

---

269-270: LGTM: Data store getters properly integrated before task execution.

Adding the data store getters immediately before task execution ensures they're available when needed, addressing the data store resolution issues mentioned in the PR objectives.

---

393-396: This defensive check is correct; no action needed.

The .get() call with conditional handling is intentional defensive programming. THREAD_LOCAL_DATA is initialized during app configuration (in config/__init__.py:339), so it should always exist in normal operation. Other services like workflow_execution_service.py and feature_flag_service.py use the same pattern of defensive access. ProcessInstanceProcessor itself directly accesses THREAD_LOCAL_DATA without checking for None (line 436), which works because it assumes the config is initialized. The test runner's defensive approach ensures it won't crash if tld is unexpectedly absent—the code simply continues without setting the identifier, which is safe since downstream code (like ProcessInstanceProcessor.__get_augment_methods) also checks hasattr() before accessing thread-local attributes.

---

342-351: Empty string is a valid process_model_identifier.

Tests confirm that passing an empty string "" as the process_model_identifier works without errors. The code safely handles empty strings through defensive patterns: feature flag lookups use .get(), data store queries check if location is not None, and other services use hasattr() guards before accessing the identifier. The identifier is straightforward to resolve as a data store location key since database queries and feature flag lookups are designed to handle any string value, including empty strings.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[burnettk]

hot