Skip to content

[tests] Avoid Celery logging interference in connection tests#1394

Merged
nemesifier merged 1 commit into
masterfrom
fix-connection-task-logger-test
Jun 5, 2026
Merged

[tests] Avoid Celery logging interference in connection tests#1394
nemesifier merged 1 commit into
masterfrom
fix-connection-task-logger-test

Conversation

@nemesifier
Copy link
Copy Markdown
Member

The connection task test patched all Logger.info calls, so Celery's eager task success log could become the final mocked call and fail the assertion when the test was reused by extended apps.

Patch only the connection task logger to keep the assertion focused on the application log emitted by update_config.

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • N/A I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • N/A I have updated the documentation.

Reference to Existing Issue

Small fix to test suite.

The connection task test patched all Logger.info calls, so Celery's
eager task success log could become the final mocked call and fail the
assertion when the test was reused by extended apps.

Patch only the connection task logger to keep the assertion focused on
the application log emitted by update_config.
@nemesifier nemesifier self-assigned this Jun 5, 2026
@nemesifier nemesifier added the bug label Jun 5, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 5, 2026

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: 2467e8d1-c355-44cd-8efc-360d514edc9d

📥 Commits

Reviewing files that changed from the base of the PR and between 977b7b6 and e0ee749.

📒 Files selected for processing (1)
  • openwisp_controller/connection/tests/test_tasks.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: CodeQL
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Analyze (python)
  • GitHub Check: Analyze (actions)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}

📄 CodeRabbit inference engine (Custom checks)

**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously

Files:

  • openwisp_controller/connection/tests/test_tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}

📄 CodeRabbit inference engine (Custom checks)

Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries

Files:

  • openwisp_controller/connection/tests/test_tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}

📄 CodeRabbit inference engine (Custom checks)

Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable

Files:

  • openwisp_controller/connection/tests/test_tasks.py
**/*.{py,html}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework

Files:

  • openwisp_controller/connection/tests/test_tasks.py
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it

Files:

  • openwisp_controller/connection/tests/test_tasks.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

  • openwisp_controller/connection/tests/test_tasks.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.

Applied to files:

  • openwisp_controller/connection/tests/test_tasks.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

  • openwisp_controller/connection/tests/test_tasks.py
🔇 Additional comments (1)
openwisp_controller/connection/tests/test_tasks.py (1)

97-97: LGTM!


📝 Walkthrough

Walkthrough

This PR updates a test fixture in the connection tasks test module to correctly mock the logger instance. The test_update_config_skipped_for_deactivated_device test was patching the generic logging.Logger.info class method, but now targets the specific logger instance used by openwisp_controller.connection.tasks. This ensures the mock properly intercepts the logging calls made by the code under test.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Bug Fixes ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format with [tests] type prefix and provides a descriptive, specific summary of the change addressing logging interference issues.
Description check ✅ Passed The description covers the issue, solution, and checklist requirements, though some template sections are marked N/A or incomplete.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-connection-task-logger-test

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.

@nemesifier nemesifier merged commit e0ee749 into master Jun 5, 2026
20 checks passed
@nemesifier nemesifier deleted the fix-connection-task-logger-test branch June 5, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant