Add tests group and pytest configuration - #7
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (46)
📝 WalkthroughWalkthroughThis PR configures pytest and adds broad automated coverage for CLI components, Foundry casting, molds, runes, Smithy utilities, installation flows, networking, monitoring, and template rendering. ChangesTest infrastructure and CLI
Foundry casting
Molds and models
Runes
Smithy
Templates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
64052b9 to
68b2350
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_foundry.py (1)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
BASE_SYSTEM/ensure_systemimports to module level.Both test functions re-import these locally instead of using the top-level import block (where
BASE_FEATURES/ensure_featuresalready live). Minor duplication.♻️ Suggested fix
from nullforge.molds.defaults import BASE_FEATURES +from nullforge.molds.defaults import BASE_SYSTEM from nullforge.molds.utils import ensure_features +from nullforge.molds.utils import ensure_systemthen drop the local imports inside each test body.
Also applies to: 35-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_foundry.py` around lines 15 - 18, Move the BASE_SYSTEM and ensure_system imports into the module-level import block alongside BASE_FEATURES and ensure_features, then remove the duplicated local imports from both test functions while preserving their existing usage.tests/conftest.py (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared
_opsmock across 7 unrelated operation modules risks cross-test call-history pollution.
server,systemd,files,git,apt,dnf, andpythonare all patched with the exact sameMagicMock()instance, and it's never reset between tests (patchers start once at import and stop only at session end). Any two of these modules sharing an attribute name will share call history, and history accumulates for the whole session. This is fine for the current cohort (no assertions inspect_ops), but it's a latent flakiness risk for later test layers (molds/runes/Smithy) that build on this fixture.♻️ Suggested fix: give each module its own mock
-_ops = MagicMock() - _patchers = [ patch("pyinfra.context.host", _mock_host), - patch("pyinfra.operations.server", _ops), - patch("pyinfra.operations.systemd", _ops), - patch("pyinfra.operations.files", _ops), - patch("pyinfra.operations.git", _ops), - patch("pyinfra.operations.apt", _ops), - patch("pyinfra.operations.dnf", _ops), - patch("pyinfra.operations.python", _ops), + patch("pyinfra.operations.server", MagicMock()), + patch("pyinfra.operations.systemd", MagicMock()), + patch("pyinfra.operations.files", MagicMock()), + patch("pyinfra.operations.git", MagicMock()), + patch("pyinfra.operations.apt", MagicMock()), + patch("pyinfra.operations.dnf", MagicMock()), + patch("pyinfra.operations.python", MagicMock()), patch.object(_pyinfra_local, "include", MagicMock()), ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 30 - 42, Replace the shared _ops MagicMock used in _patchers with a distinct mock for each patched operation module: server, systemd, files, git, apt, dnf, and python. Keep the existing patch targets and behavior unchanged while ensuring each module has isolated call history.tests/test_runes_netsec.py (1)
106-120: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest re-derives
conntrack_max_for's formula instead of calling it.
get_total_memoryis mocked butconntrack_max_foris not, so the assertion is built from an independently reimplemented formula (TOTAL_MEMORY * 1024 * 1024 * 0.02 / 300) rather than the real function's output. If the production formula changes, this test won't detect the drift reliably.♻️ Suggested fix: derive the expected value from the real function
- def test_injects_ram_derived_sizing(self) -> None: + def test_injects_ram_derived_sizing(self) -> None: + from nullforge.runes.netsec import conntrack_max_for with ( patch("nullforge.runes.netsec.module_loaded", return_value=True), patch("nullforge.runes.netsec.get_total_memory", return_value=self.TOTAL_MEMORY), ): result = _resolve_conntrack_sysctls({"net.netfilter.nf_conntrack_udp_timeout": 30}) - ct_max_target = int(self.TOTAL_MEMORY * 1024 * 1024 * 0.02 / 300) + ct_max_target = conntrack_max_for(self.TOTAL_MEMORY) ct_buckets = max(4096, (ct_max_target + 3) // 4)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_runes_netsec.py` around lines 106 - 120, Update test_injects_ram_derived_sizing to derive the expected conntrack target by calling the production conntrack_max_for function with the mocked TOTAL_MEMORY, rather than reimplementing its formula locally. Keep the existing bucket rounding, range, and configured-timeout assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_models_dns.py`:
- Around line 18-26: Update test_quad9_doh_no_ecs and test_quad9_doh_with_ecs to
assert that the filtered doh list is non-empty, or matches the expected server
count, before checking URL contents; retain the existing URL assertions for the
selected DoH endpoints.
In `@tests/test_molds_utils.py`:
- Around line 49-53: Update test_sub_mold_overrides_field to construct the
overlay DnsMold with a non-default mode such as DnsMode.NONE, while keeping the
expected result assertion aligned with that value so the test verifies
merge_features applies the sub-mold override.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 30-42: Replace the shared _ops MagicMock used in _patchers with a
distinct mock for each patched operation module: server, systemd, files, git,
apt, dnf, and python. Keep the existing patch targets and behavior unchanged
while ensuring each module has isolated call history.
In `@tests/test_foundry.py`:
- Around line 15-18: Move the BASE_SYSTEM and ensure_system imports into the
module-level import block alongside BASE_FEATURES and ensure_features, then
remove the duplicated local imports from both test functions while preserving
their existing usage.
In `@tests/test_runes_netsec.py`:
- Around line 106-120: Update test_injects_ram_derived_sizing to derive the
expected conntrack target by calling the production conntrack_max_for function
with the mocked TOTAL_MEMORY, rather than reimplementing its formula locally.
Keep the existing bucket rounding, range, and configured-timeout assertions
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a039af68-916a-4396-95b5-ae9be5dd7a9a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
pyproject.tomltests/__init__.pytests/cli/__init__.pytests/cli/components/__init__.pytests/cli/components/completion/__init__.pytests/cli/components/completion/test_controller.pytests/cli/components/completion/test_powershell.pytests/cli/components/foundry/__init__.pytests/cli/components/foundry/test_controller.pytests/cli/conftest.pytests/cli/core/__init__.pytests/cli/core/test_application.pytests/cli/test_types.pytests/conftest.pytests/test_foundry.pytests/test_foundry_cast.pytests/test_foundry_pyinfra.pytests/test_models_dns.pytests/test_molds_dns.pytests/test_molds_monitoring.pytests/test_molds_netsec.pytests/test_molds_telemt.pytests/test_molds_user.pytests/test_molds_utils.pytests/test_runes_dns.pytests/test_runes_meta.pytests/test_runes_misc.pytests/test_runes_netsec.pytests/test_runes_profiles.pytests/test_runes_telemt.pytests/test_runes_users.pytests/test_smithy_arch.pytests/test_smithy_github.pytests/test_smithy_http.pytests/test_smithy_install.pytests/test_smithy_network.pytests/test_smithy_nezha.pytests/test_smithy_nezha_dashboard.pytests/test_smithy_packages.pytests/test_smithy_service.pytests/test_smithy_sni.pytests/test_smithy_swap.pytests/test_smithy_system.pytests/test_smithy_versions.pytests/test_templates.pytests/test_templates_telemt.py
68b2350 to
ec70c14
Compare
1 participant
Type of change
Description
Why is this change needed?
Related Issues
Testing
uv run poe tests)Checklist
uv run poe check)Summary by CodeRabbit
Tests
Chores