Skip to content

Conversation

@desertaxle
Copy link
Collaborator

@desertaxle desertaxle commented Oct 27, 2025

Summary

This PR refactors the CLI test suite to use subprocess-based execution instead of Typer's in-process CliRunner, improving test isolation and fixing test hangs that were occurring on Python 3.10.

Changes

CLI Test Execution

  • New subprocess-based test utility (tests/cli/utils.py): Implements run_cli() that spawns python -m docket as a subprocess
  • Refactored all CLI tests to use the new run_cli() utility instead of CliRunner.invoke()
  • Better isolation: Each CLI command runs in its own process with proper coverage tracking via sitecustomize.py

Memory Backend Support

  • Skip markers: CLI tests are now skipped when REDIS_VERSION=memory since the CLI rejects memory:// URLs
  • Coverage configuration: Added .coveragerc-memory to handle coverage for memory backend tests
  • Test compatibility: Ensured tests work across Redis, Valkey, and memory backends

Test Reliability Improvements

  • Timing adjustments: Increased task durations in snapshot tests to account for subprocess startup latency
  • Removed flaky assertions: Cleaned up assertions that were timing-dependent

Why These Changes?

The in-process CliRunner approach was causing test hangs, particularly on Python 3.10. By running CLI commands in separate subprocesses:

  1. Tests are properly isolated from each other
  2. No shared asyncio event loop conflicts
  3. More realistic testing of actual CLI behavior
  4. Proper coverage tracking in subprocess execution

@claude
Copy link
Contributor

claude bot commented Oct 27, 2025

Claude finished @desertaxle's task —— View job


PR Review: Update CLI test suite to use subprocesses

I've reviewed this PR that refactors the CLI test suite from using typer.testing.CliRunner to running actual CLI commands via subprocess. Here's my comprehensive feedback:

Strengths

  1. Improved Test Realism: Moving from CliRunner to subprocess-based testing better reflects real-world CLI usage, catching issues that might only appear when running as separate processes.

  2. Excellent Coverage Handling: The implementation cleverly handles coverage collection across subprocess boundaries:

    • Uses sitecustomize.py for automatic coverage initialization
    • Properly detects pytest-cov vs standalone coverage scenarios
    • Maintains 100% test coverage requirement
  3. Clean Architecture: The new tests/cli/utils.py provides a well-structured abstraction:

    • CliResult maintains compatibility with the old CliRunner.Result API
    • Async implementation allows for proper timeout handling
    • Environment variable management is clean and secure
  4. Memory Backend Support: Smart handling of the memory backend limitation:

    • CLI properly validates and rejects memory:// URLs with helpful error messages
    • Tests are systematically skipped for memory backend using pytest markers
    • Separate coverage configuration (.coveragerc-memory) properly excludes CLI from coverage

🔍 Observations & Suggestions

1. Security Considerations

The subprocess implementation appears secure:

  • Uses sys.executable to ensure the same Python interpreter
  • Properly escapes and passes arguments via list (not shell strings)
  • Environment variable handling doesn't expose sensitive data

2. Performance Impact ⚠️

  • Default 90-second timeout is generous but reasonable for CI environments
  • Tests now run actual Python processes which adds overhead
  • Consider if parallel test execution might help offset this cost

3. Potential Race Conditions

The async implementation properly handles concurrency:

  • Uses asyncio.wait_for() with timeout protection
  • Tests with timing dependencies (e.g., worker heartbeats) use appropriate sleep intervals
  • No obvious race conditions detected

4. Error Handling

Good coverage of error cases:

  • Tests verify non-zero exit codes for invalid inputs
  • Both stdout and stderr are captured and available
  • Connection errors to Redis are properly tested

🎯 Minor Suggestions for Improvement

  1. Consider adding timeout tests: While the utility supports timeout, there are no explicit tests verifying timeout behavior. Consider adding a test that intentionally triggers a timeout.

  2. Documentation: Consider adding a comment in utils.py explaining why the coverage setup is needed and how it works with both pytest-cov and standalone coverage.

  3. PYTHONPATH management: The code appends to PYTHONPATH safely, but consider documenting why the repo root needs to be added (for sitecustomize.py discovery).

📊 Code Quality

  • Cleanliness: Code is clean and follows project conventions
  • Test Organization: Tests are well-organized with clear separation of concerns
  • Type Safety: Proper type hints throughout the new code
  • Backwards Compatibility: The CliResult.output property maintains API compatibility

Test Coverage

The PR maintains the required 100% test coverage:

  • All new code is covered
  • Coverage configuration properly handles the memory backend case
  • CI matrix appropriately adjusts thresholds for different Python versions

🚀 Conclusion

This is a well-executed refactoring that improves test quality while maintaining coverage and adding proper validation for the memory backend limitation. The subprocess approach better reflects real-world usage and will catch more potential issues.

Recommendation: ✅ Approve - This PR improves the test suite's reliability and real-world accuracy without introducing security concerns or breaking changes.

Review Checklist

  • Understand the refactoring approach
  • Review subprocess implementation safety
  • Check test coverage and completeness
  • Look for potential bugs or race conditions
  • Evaluate code organization and best practices
  • Provide comprehensive feedback

Great work on this refactoring! The subprocess-based testing approach is a solid improvement that will help catch real-world issues earlier in development.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (d96d99a) to head (94da98f).

Additional details and impacted files

Impacted file tree graph

@@                                 Coverage Diff                                  @@
##           claude/support-python-310-011CUQFhrhzXGxLGyRjXk8k9      #176   +/-   ##
====================================================================================
  Coverage                                              100.00%   100.00%           
====================================================================================
  Files                                                      33        34    +1     
  Lines                                                    5103      5107    +4     
  Branches                                                  263       264    +1     
====================================================================================
+ Hits                                                     5103      5107    +4     
Flag Coverage Δ
python-3.10 100.00% <100.00%> (+15.18%) ⬆️
python-3.11 98.76% <100.00%> (+<0.01%) ⬆️
python-3.12 100.00% <100.00%> (ø)
python-3.13 100.00% <100.00%> (ø)
python-3.14 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/docket/cli.py 100.00% <100.00%> (ø)
tests/cli/test_clear.py 100.00% <100.00%> (ø)
tests/cli/test_module.py 100.00% <100.00%> (ø)
tests/cli/test_parsing.py 100.00% <100.00%> (ø)
tests/cli/test_snapshot.py 100.00% <100.00%> (ø)
tests/cli/test_striking.py 100.00% <100.00%> (ø)
tests/cli/test_tasks.py 100.00% <100.00%> (ø)
tests/cli/test_url_validation.py 100.00% <100.00%> (ø)
tests/cli/test_version.py 100.00% <100.00%> (ø)
tests/cli/test_worker.py 100.00% <100.00%> (ø)
... and 2 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

desertaxle and others added 3 commits October 27, 2025 11:03
CLI commands reject memory:// URLs since they don't persist across
processes. This adds pytest skip markers to CLI tests and configures
coverage exclusion for the memory backend testing environment.

- Add pytest skip markers to CLI tests when REDIS_VERSION=memory
- Create .coveragerc-memory with CLI exclusions for memory backend
- Update CI to use --cov-config=.coveragerc-memory for memory backend
- Set coverage threshold to 99% for memory backend (vs 100% for Redis)
- Add comprehensive URL validation tests that run in all environments

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@desertaxle desertaxle changed the title Run CLI commands in subprocesses for CLI tests Update CLI test suite to use subprocesses Oct 27, 2025
@desertaxle desertaxle marked this pull request as ready for review October 27, 2025 18:17
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

hell yea!

@desertaxle desertaxle merged commit a16fcbe into claude/support-python-310-011CUQFhrhzXGxLGyRjXk8k9 Oct 27, 2025
106 of 107 checks passed
@desertaxle desertaxle deleted the fix-hanging-tests branch October 27, 2025 18:23
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.

4 participants