Skip to content

Conversation

@olivermeyer
Copy link
Collaborator

@olivermeyer olivermeyer commented Feb 6, 2026

Fix an import error causing test_cli_gui_run to fail with

AttributeError: 'module' object at aignostics.utils has no attribute 'utils'

E.g. this job.

@codecov
Copy link

codecov bot commented Feb 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.
see 9 files with indirect coverage changes

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 2026

@olivermeyer olivermeyer marked this pull request as ready for review February 6, 2026 09:38
@atlantis-platform-engineering
Copy link

Error: This repo is not allowlisted for Atlantis.

@claude
Copy link

claude bot commented Feb 6, 2026

Claude finished @olivermeyer's task in 3m 35s —— View job


Code Review Complete ✅

I've reviewed PR #419 and the import order fix looks good! Here's my comprehensive analysis:

Review Checklist

  • Read and understand the code changes
  • Verify test markers are present
  • Check commit message follows conventional commits
  • Validate code quality standards
  • Check for architectural compliance
  • Verify no breaking changes
  • Review test coverage impact
  • Provide final review summary

Summary

Status: ✅ APPROVED - Ready to Merge

This PR successfully fixes an AttributeError in test_cli_gui_run caused by incorrect monkeypatch usage with string paths. The fix is clean, minimal, and follows best practices.

What Changed

File: tests/aignostics/cli_test.py (+5/-1)

Problem:

# Before: Using string path for monkeypatch
monkeypatch.setattr("aignostics.utils._gui.gui_register_pages", mock_gui_register_pages)

This failed in CI with:

AttributeError: 'module' object at aignostics.utils has no attribute 'utils'

Solution:

# Import module explicitly before monkeypatching
import aignostics.utils._gui as _utils_gui

# Use the imported module reference
monkeypatch.setattr(_utils_gui, "gui_register_pages", mock_gui_register_pages)

Root Cause: In pytest-xdist parallel execution, string-based monkeypatch paths can fail if the module hasn't been imported yet in that worker process. Importing explicitly ensures the module is loaded before patching.

✅ Verification Results

1. Test Execution

  • test_cli_gui_run now passes
  • ✅ All 8 tests in cli_test.py pass (100% success rate)
  • ✅ No flaky behavior observed

2. Code Quality (All Checks Pass)

✅ ruff check    - No issues
✅ ruff format   - 156 files already formatted
✅ pyright       - 0 errors, 0 warnings
✅ mypy          - Success: no issues in 80 source files

3. Test Markers ✅ (CRITICAL CHECK)

All tests properly marked with @pytest.mark.integration:

  • ✅ No unmarked tests found (0 collected with marker filter)
  • ✅ All tests will run in CI pipeline

4. Commit Message ✅

test: fix import order in test_cli_gui_run
  • ✅ Follows conventional commits format
  • ✅ Type: test (appropriate for test fixes)
  • ✅ Clear, concise description

5. Architecture & Standards ✅

  • ✅ No breaking changes
  • ✅ No new dependencies
  • ✅ No changes to production code (test-only fix)
  • ✅ Follows pytest best practices (explicit imports > string paths)
  • ✅ Added helpful explanatory comment

Code Review Details

Lines 132-136 (New code):

# Import module explicitly to ensure it's loaded before monkeypatching with string paths.
# Without this, monkeypatch.setattr("aignostics.utils._gui...") fails in CI because
# pytest-xdist workers may not have aignostics.utils loaded as an attribute yet.
import aignostics.utils._gui as _utils_gui

Excellent: Clear comment explaining the reason for this pattern. This helps future maintainers understand why explicit import is needed.

Line 193 (Modified):

monkeypatch.setattr(_utils_gui, "gui_register_pages", mock_gui_register_pages)

Correct: Using the imported module reference instead of string path prevents the AttributeError in pytest-xdist workers.

Why This Fix Works

  1. Module Loading: Explicit import ensures aignostics.utils._gui is loaded in the worker's namespace
  2. Reference-Based Patching: Using the module object directly (_utils_gui) is more reliable than string paths in parallel execution
  3. No Side Effects: The import is scoped to the conditional block (only if nicegui is available)
  4. Consistent Pattern: Matches pytest best practices for monkeypatching in parallel test environments

Testing Strategy Compliance

From .github/CLAUDE.md:

  • ✅ Test has correct @pytest.mark.integration marker
  • ✅ Uses appropriate parallelization (XDIST_WORKER_FACTOR=0.2 for integration tests)
  • ✅ Timeout appropriate (default 10s, sufficient for this test)
  • ✅ Mocks external dependencies (nicegui.ui.run)
  • ✅ Does not call real external services

Impact Assessment

Risk Level: ⚠️ MINIMAL

  • Single test file modified
  • No production code changes
  • No API changes
  • No dependency changes
  • Fixes existing test failure

Coverage Impact: ✅ NEUTRAL (no change to coverage metrics)

CI/CD Impact: ✅ POSITIVE (resolves test failures in parallel execution)

Additional Observations

Good Practices Demonstrated:

  1. ✅ Minimal change principle - only fixed what was broken
  2. ✅ Added explanatory comment for future maintainers
  3. ✅ Follows pytest-xdist best practices
  4. ✅ Test remains comprehensive (verifies all expected behaviors)

No Issues Found:

  • No linting violations
  • No type checking errors
  • No missing test markers
  • No architectural violations
  • No security concerns

Recommendation

✅ APPROVE AND MERGE

This is a textbook example of a clean, focused bug fix:

  • Identifies the root cause (pytest-xdist module loading)
  • Applies minimal fix (explicit import)
  • Documents the reasoning (helpful comment)
  • Maintains all existing test coverage
  • Passes all CI checks

No further changes needed.


Branch: test/fix-import-order

@olivermeyer
Copy link
Collaborator Author

One failure on MacOS runner for a different reason:

FAILED tests/aignostics/bucket/cli_test.py::test_cli_bucket_flow - Failed: Timeout (>900.0s) from pytest-timeout.

I suggest we still merge the fix because (1) test_cli_gui_run succeeded on the same runner and (2) test_cli_bucket_flow is flaky.

@olivermeyer olivermeyer merged commit 283d2f8 into main Feb 6, 2026
28 of 30 checks passed
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.

2 participants