Skip to content

fix(system): replace uptime with psutil#548

Merged
olivermeyer merged 1 commit intomainfrom
fix/remove-uptime-dp
Apr 17, 2026
Merged

fix(system): replace uptime with psutil#548
olivermeyer merged 1 commit intomainfrom
fix/remove-uptime-dp

Conversation

@olivermeyer
Copy link
Copy Markdown
Collaborator

Why?
uptime==3.0.1 is a source-only C extension with no pre-built wheels. On Windows, uv's build isolation uses Python 3.14's typing.py when compiling it for older Python versions (3.11–3.13), causing a SyntaxError that breaks CI.

How?
Remove uptime from dependencies and replace its two call sites in system/_service.py with psutil equivalents: boot time via psutil.boot_time() (already a direct dependency with binary wheels for all platforms) and uptime seconds via time.time() - boot_ts. The returned dict shape is unchanged.

Copilot AI review requested due to automatic review settings April 17, 2026 11:49
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@olivermeyer olivermeyer force-pushed the fix/remove-uptime-dp branch from cb45b98 to ebd809f Compare April 17, 2026 11:50
@sonarqubecloud
Copy link
Copy Markdown

@olivermeyer olivermeyer added the claude Trigger Claude Code automation label Apr 17, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the uptime dependency (a source-only C extension causing CI failures on Windows/Python 3.14 build isolation) and replaces its usage in the system module with psutil-based boot time and uptime computation, keeping the Service.info() output shape consistent.

Changes:

  • Drop uptime from project dependencies and lockfile.
  • Update src/aignostics/system/_service.py to compute boot time via psutil.boot_time() and uptime seconds via time.time() - boot_ts.
  • Add unit tests validating that Service.info() includes the expected uptime keys and formats.

Reviewed changes

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

File Description
pyproject.toml Removes uptime from declared dependencies.
uv.lock Removes the uptime package entry and dependency edges from the lockfile.
src/aignostics/system/_service.py Replaces uptime calls with psutil + time implementation for boot time/uptime seconds.
tests/aignostics/system/service_test.py Adds tests and shared mocking helper to validate the new uptime fields in Service.info().

Comment thread src/aignostics/system/_service.py
Comment thread tests/aignostics/system/service_test.py
@claude
Copy link
Copy Markdown

claude bot commented Apr 17, 2026

PR Review: fix(system): replace uptime with psutil

✅ SUMMARY

Status: APPROVED - All critical checks pass, excellent implementation!

This PR successfully fixes a Windows CI build issue by replacing the problematic uptime dependency (source-only C extension) with psutil (already a dependency with binary wheels for all platforms).


✅ CRITICAL CHECKS - ALL PASS

1. Test Markers ✅

All 3 new test functions have the required @pytest.mark.unit marker:

  • test_info_uptime_keys_present()
  • test_info_uptime_seconds_positive()
  • test_info_uptime_boottime_is_iso_string()

Verification: uv run pytest -m "not unit and not integration and not e2e" --collect-only shows 0 unmarked tests.

2. Test Coverage ✅

  • 114 lines of test code added (lines 11-76, 468-514)
  • Comprehensive mocking via _patch_info_dependencies() helper
  • Tests verify key presence, data types, ISO 8601 format, and timezone awareness
  • All 3 tests pass

3. Code Quality ✅

$ uv run ruff check src/aignostics/system/_service.py tests/aignostics/system/service_test.py
All checks passed!
  • Type hints preserved
  • Proper import organization (stdlib datetime, time added)
  • Clean code: psutil.boot_time() + datetime.datetime.fromtimestamp(boot_ts, tz=datetime.UTC)
  • Modern timezone handling: datetime.UTC instead of deprecated timezone.utc

4. Conventional Commits ✅

Format: fix(system): replace uptime with psutil

  • ✅ Type: fix (correct)
  • ✅ Scope: system (correct module)
  • ✅ Description: clear and concise

✅ REPOSITORY-SPECIFIC CHECKS - ALL PASS

5. Architecture Compliance ✅

  • Service layer only modification (no CLI/GUI coupling)
  • Service pattern maintained (BaseService.health(), BaseService.info())
  • No circular dependency issues
  • Module boundaries respected

6. Testing Strategy ✅

  • Markers: @pytest.mark.unit + @pytest.mark.asyncio (correct for async service method)
  • Timeout: Default 10s (no custom timeout needed)
  • Mocking strategy: Comprehensive - patches all external I/O including:
    • psutil.boot_time() → fixed timestamp
    • time.time() → simulated "now" (boot + 3600s)
    • All other system info sources
  • Test isolation: Proper use of context managers

7. Medical Device & Security ✅

  • No security implications
  • No secrets/tokens introduced
  • No medical data handling changes

8. Breaking Changes ✅

None - External API unchanged:

"uptime": {
    "seconds": <float>,      # Still float seconds
    "boottime": <ISO string>  # Still ISO 8601 + UTC
}

Minor change (safe):

  • Old: "boottime": bootdatetime.isoformat() if bootdatetime else None
  • New: "boottime": bootdatetime.isoformat()
  • ✅ Safe because psutil.boot_time() always returns a value (fundamental system metric)

9. CI/CD Impact ✅

  • No workflow file changes
  • Tests properly categorized for CI execution (unit marker)
  • Will run in standard CI pipeline on all Python versions

10. Documentation Updates ✅

  • No user-facing API changes → no README updates needed
  • Internal implementation change → CLAUDE.md updates not required
  • Functionality unchanged from user perspective

💡 SUGGESTIONS (Non-blocking)

1. Test Quality Enhancement

File: tests/aignostics/system/service_test.py:489-496

The test_info_uptime_seconds_positive() test could verify the exact calculation:

# Current test (line 494-496):
seconds = result["runtime"]["host"]["uptime"]["seconds"]
assert isinstance(seconds, float)
assert seconds > 0  # Only checks positive, not the actual value

# Suggested enhancement:
seconds = result["runtime"]["host"]["uptime"]["seconds"]
assert isinstance(seconds, float)
assert seconds == pytest.approx(3600.0, rel=0.1)  # Verify calculation is correct
# Fixture mocks: boot_time=1_000_000.0, time.time()=1_003_600.0
# Expected: time.time() - boot_ts = 3600.0 seconds (1 hour)

Rationale: Would catch bugs like reversed subtraction (boot_ts - time.time()) which would pass current test (negative is still a float).

Benefit: Higher confidence in calculation correctness.

2. Test Fixture Documentation

File: tests/aignostics/system/service_test.py:15, 28

Consider adding brief comments:

FIXED_BOOT_TIME = 1_000_000.0  # Arbitrary fixed epoch seconds for deterministic tests

now = boot_time + 3600.0  # Pretend system has been up 1 hour (3600 seconds)

3. Edge Case Test (Optional)

Consider adding a test for very recent boot time:

@pytest.mark.unit
@pytest.mark.asyncio
async def test_info_uptime_very_short() -> None:
    """Test uptime with very recent boot (< 1 second)."""
    recent_boot = time.time() - 0.5  # 500ms ago
    with _patch_info_dependencies(boot_time=recent_boot):
        result = await Service.info()
    
    seconds = result["runtime"]["host"]["uptime"]["seconds"]
    assert 0 < seconds < 1.0  # Between 0 and 1 second

🌟 EXCELLENT PRACTICES

  1. Excellent problem diagnosis: Identified Windows C extension build issue with Python 3.14's typing.py in build isolation
  2. Smart solution: Replace with psutil (already a dependency, pure Python with binary wheels)
  3. Comprehensive test mocking: _patch_info_dependencies() patches all external I/O for deterministic tests
  4. Timezone awareness: Tests verify boottime is timezone-aware UTC (parsed.tzinfo is not None)
  5. ISO 8601 validation: Tests verify datetime.datetime.fromisoformat(boottime_str) succeeds
  6. No dependency bloat: Removed uptime, used existing psutil
  7. Clean implementation: Simple, readable code with proper type hints

📋 REVIEW CHECKLIST

  • ✅ Test markers (unit/integration/e2e) present
  • ✅ Test coverage ≥85% for changed code
  • ✅ Code quality (ruff, mypy, pyright)
  • ✅ Conventional commit format
  • ✅ Architecture compliance (modulith, service pattern)
  • ✅ Testing strategy (markers, mocking, isolation)
  • ✅ No security issues
  • ✅ No breaking changes
  • ✅ CI/CD compatibility
  • ✅ Documentation (not needed for internal change)

🎯 FINAL VERDICT

APPROVED

This is a well-executed bug fix with:

  • Clear problem diagnosis
  • Smart, minimal solution
  • Comprehensive test coverage
  • No breaking changes
  • All critical checks passing

The suggestions above are non-blocking quality improvements. The PR is ready to merge as-is.

Great work! 🎉


Commands to verify:

# Run the new tests
uv run pytest tests/aignostics/system/service_test.py::test_info_uptime_keys_present -v
uv run pytest tests/aignostics/system/service_test.py::test_info_uptime_seconds_positive -v
uv run pytest tests/aignostics/system/service_test.py::test_info_uptime_boottime_is_iso_string -v

# Check linting
uv run ruff check src/aignostics/system/_service.py tests/aignostics/system/service_test.py

# Verify no unmarked tests
uv run pytest -m "not unit and not integration and not e2e" --collect-only

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines Coverage Δ
src/aignostics/system/_service.py 84.91% <100.00%> (-0.31%) ⬇️

... and 1 file with indirect coverage changes

@olivermeyer
Copy link
Copy Markdown
Collaborator Author

Tests passed on windows-latest; previously they failed. The fix works.

@olivermeyer olivermeyer merged commit 346a9f6 into main Apr 17, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude Trigger Claude Code automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants