Skip to content

test: harden test suite — real objects over mocks, integration as pytest#88

Merged
marcinpsk merged 7 commits into
mainfrom
chore/tests
Jun 14, 2026
Merged

test: harden test suite — real objects over mocks, integration as pytest#88
marcinpsk merged 7 commits into
mainfrom
chore/tests

Conversation

@marcinpsk

@marcinpsk marcinpsk commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Overview

Hardens the test suite along two axes:

  1. Replace data-stub MagicMocks with real objects so absent-field branches are exercised faithfully. A MagicMock synthesizes any attribute on demand, so a mock-heavy test stays green even when the production path reads a field that was never set.
  2. Promote the integration test to a proper pytest modulepytest.fail instead of sys.exit, an integration marker, a collection-time skip guard, and CI invoking it via pytest rather than python.

Changes

  • Integration test → pytest module: register the integration marker; module-level pytestmark; collection-time skip guard that marks the whole package skipped when NETBOX_URL/NETBOX_TOKEN are absent (so it shows s, not errors); autouse mock fixtures bail out for integration-marked tests; CI runs pytest tests/integration/ -m integration -x -v --timeout=600.
  • Mocks → real objects:
    • test_change_detector / test_exporter / test_repo / test_nb_dt_import: DotDict (the production GraphQL-record wrapper), a strict _Stub, and real tmp_path directories instead of os.listdir/exists/isdir patches.
    • test_normalization: choice records → SimpleNamespacehasattr(.value) was always True under MagicMock.
    • test_log_handler: port records → SimpleNamespace, now exercising the hasattr(port,'type') else-branch.
    • test_update_failure_resolver: template/device records → SimpleNamespace.
    • Boundary mocks kept: pynetbox client, HTTP responses, Rich console, loggers, and the template asserting .delete().
  • Fixes / polish:
    • Restore test_default_mode_no_device_types, whose def line had been accidentally deleted (its body was folded into the preceding test; count 73 → 74).
    • Drop the redundant main() runner from the integration test (its hardcoded test list could silently drift).
    • Document that the skip guard sees .env values via load_dotenv; fix stale sys.exit/usage references in the integration docstring.

Verification

  • uv run pytest tests/ → 939 passed
  • uvx ruff check tests/ → clean
  • Pre-commit gate (ruff, ruff-format, interrogate, pytest) passed

Summary by CodeRabbit

Release Notes

  • Tests
    • Reorganized integration test infrastructure with pytest markers and automatic skipping when environment variables are missing.
    • Enhanced test fixtures to properly exclude mocks during integration test execution.
    • Refactored test utilities to use simpler object constructors and real temporary directories for more deterministic and reliable testing.
    • Updated CI workflow to run integration tests with improved error detection and per-test timeout handling.

- test_exporter.py: add _Stub class; replace _make_mfr/_make_dt/_make_mt/_make_rt
  MagicMock() with _Stub(**kwargs) so absent-field access raises AttributeError
  instead of silently returning a truthy sub-mock

- test_nb_dt_import.py: upgrade bare assert_called_once() to also assert first
  positional arg and key kwargs on create_device_types, create_module_types,
  create_rack_types, and the three _process_*_types patched in the vendor loop test

- test_change_detector.py: replace MagicMock() for netbox_dt/existing_comp/
  netbox_comp/existing with DotDict({...}) — the real type the change detector
  receives from GraphQL; absent fields now raise AttributeError instead of
  returning a truthy sub-mock, so a new comparison field added to
  DEVICE_TYPE_PROPERTIES actually fails rather than silently passing

- test_repo.py: replace patch('os.listdir') / patch('os.path.exists') /
  patch('os.path.isdir') in TestGetDevices and TestDiscoverVendors with real
  tmp_path directories; catch path-construction bugs bypassed by the old mocks;
  keep OS-error test mock-based (simulating permission errors is not portable)
Restore test_default_mode_no_device_types, whose `def` line was
accidentally deleted in e57b3e7 — its body had been folded into the
preceding test (count 73 -> 74).

Replace data-stub MagicMocks with real objects so absent-field branches
are exercised faithfully (a MagicMock fabricates any attribute, masking
them):
- test_change_detector: TestCompareImageProperties netbox_dt -> DotDict
- test_normalization: choice records -> SimpleNamespace (hasattr(.value)
  was always True under MagicMock)
- test_log_handler: port records -> SimpleNamespace (exercises the
  hasattr(port,'type') else-branch)
- test_update_failure_resolver: template/device records -> SimpleNamespace

Boundary mocks kept: pynetbox client, HTTP responses, Rich console,
loggers, and the template asserting .delete().

Polish: drop the redundant main() runner from the integration test;
document that the skip guard sees .env values via load_dotenv; fix stale
sys.exit/usage references in the integration docstring.
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b40ff0bc-e372-483f-9806-43dcb273aba0

📥 Commits

Reviewing files that changed from the base of the PR and between 2cb1ec7 and 855ea71.

📒 Files selected for processing (13)
  • .github/workflows/test-netbox-main.yaml
  • pyproject.toml
  • tests/conftest.py
  • tests/integration/__init__.py
  • tests/integration/conftest.py
  • tests/integration/test_import.py
  • tests/test_change_detector.py
  • tests/test_exporter.py
  • tests/test_log_handler.py
  • tests/test_nb_dt_import.py
  • tests/test_normalization.py
  • tests/test_repo.py
  • tests/test_update_failure_resolver.py

Walkthrough

The PR migrates the integration test suite from a standalone Python script to pytest by registering an integration marker, adding a collection hook, making autouse fixtures conditional on that marker, and removing the script's main() entry point. In parallel, unit tests across multiple modules replace MagicMock with typed stubs (DotDict, SimpleNamespace, _Stub), test_repo.py switches to real tmp_path filesystems, and test_nb_dt_import.py gains stronger positional-argument assertions.

Changes

Integration test suite migration to pytest

Layer / File(s) Summary
pytest marker registration and CI command update
pyproject.toml, .github/workflows/test-netbox-main.yaml
Declares the integration marker under [tool.pytest.ini_options] and replaces the CI script invocation with uv run pytest tests/integration/ -m integration -x -v --timeout=600.
Integration conftest collection hook and autouse fixture bypass
tests/integration/conftest.py, tests/conftest.py
Adds pytest_collection_modifyitems to attach the integration mark and a conditional skip when NETBOX_URL/NETBOX_TOKEN are absent. Updates four autouse fixtures to accept request and bypass env/mock/session patching for integration-marked tests.
test_import.py conversion to pytest module
tests/integration/test_import.py
Removes main()/__main__ entry point and sys.path manipulation, adds pytest import, sets pytestmark, replaces fail() with pytest.fail(), and updates docstrings and run-command references.

Unit test mock cleanup and assertion strengthening

Layer / File(s) Summary
_Stub helper and exporter fixture updates
tests/test_exporter.py
Adds _Stub, a class that raises AttributeError for absent fields, and updates _make_mfr, _make_dt, _make_mt, _make_rt to return _Stub instead of MagicMock.
DotDict replacements in change detector tests
tests/test_change_detector.py
Imports DotDict and replaces all MagicMock-with-assigned-attributes usages across device-type comparison, component detection, change detection, and image property tests.
SimpleNamespace replacements across unit tests
tests/test_normalization.py, tests/test_log_handler.py, tests/test_update_failure_resolver.py
Replaces MagicMock with SimpleNamespace for choice objects, port fixtures, device bay templates, and dependent device objects, exercising hasattr branches deterministically.
test_repo.py refactored to real filesystem
tests/test_repo.py
Rewrites TestGetDevices and TestDiscoverVendors to use tmp_path-backed real directories instead of os.path/os.listdir mocks, adding coverage for sorting, slug formatting, deduplication, and OSError handling.
Strengthened assertions in test_nb_dt_import.py
tests/test_nb_dt_import.py
Adds positional-argument assertions for create_device_types, create_module_types, create_rack_types, and index-4 argument checks for _process_device_types, _process_module_types, _process_rack_types.

Sequence Diagram(s)

sequenceDiagram
  participant CI as GitHub Actions
  participant pytest as pytest runner
  participant int_conftest as integration/conftest.py
  participant root_conftest as tests/conftest.py
  participant test as test_import.py tests

  CI->>pytest: uv run pytest tests/integration/ -m integration -x -v --timeout=600
  pytest->>int_conftest: pytest_collection_modifyitems(items)
  int_conftest->>int_conftest: attach integration mark to path-matched items
  int_conftest->>int_conftest: read NETBOX_URL, NETBOX_TOKEN from os.environ
  alt env vars missing
    int_conftest->>test: attach pytest.mark.skip
  end
  pytest->>root_conftest: mock_env_vars(request), mock_git_repo(request), mock_graphql_requests(request)
  root_conftest->>root_conftest: check request.node integration mark
  alt integration mark present
    root_conftest-->>test: yield (no patching)
  else unit test
    root_conftest->>root_conftest: patch env vars / git repo / requests.Session
    root_conftest-->>test: yield mocked dependencies
  end
  pytest->>test: run test functions via pytest.fail() on failure
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐇 Hoppity-hop, no more sys.exit!
The tests now run under pytest's bright light.
MagicMock banished, SimpleNamespace instead,
DotDict and _Stub fill the fixture bed.
Integration marks bloom, env vars gate the gate—
A rabbit approves: the test suite is great! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: hardening the test suite by replacing mock objects with real objects and converting integration tests to use pytest.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/tests

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

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

@marcinpsk marcinpsk merged commit 71bb410 into main Jun 14, 2026
7 checks passed
@marcinpsk marcinpsk deleted the chore/tests branch June 14, 2026 13:29
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.

1 participant