Skip to content

[chores] Separated session redis bucket from cache bucket#733

Open
nemesifier wants to merge 2 commits into
masterfrom
separate-session-storage
Open

[chores] Separated session redis bucket from cache bucket#733
nemesifier wants to merge 2 commits into
masterfrom
separate-session-storage

Conversation

@nemesifier

Copy link
Copy Markdown
Member

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

Not needed, small improvement being done in all repositories.

Description of Changes

Small improvement to the test env which separates cache from session storage to avoid terminating sessions when clearing the cache.

@coderabbitai

coderabbitai Bot commented Jun 9, 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: 4ef46a0d-e750-4ad6-b41e-c1d94100dd98

📥 Commits

Reviewing files that changed from the base of the PR and between cd6a22b and f2f3e04.

📒 Files selected for processing (1)
  • tests/openwisp2/settings.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). (5)
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.

Files:

  • tests/openwisp2/settings.py
🔇 Additional comments (1)
tests/openwisp2/settings.py (1)

351-355: Session backend is DB-backed (not cache-backed), so it’s not coupled to CACHES["default"] (Redis DB 0).
tests/openwisp2/settings.py contains no SESSION_ENGINE / SESSION_CACHE_ALIAS overrides, and the only SESSION_ENGINE assignment found is in openwisp_radius/tests/test_saml/utils.py setting django.contrib.sessions.backends.db; no cache-based session backend usage exists in this repo.


📝 Walkthrough

Walkthrough

This PR adjusts Redis database indices in the non-testing Django settings configuration. The Celery broker URL now points to Redis database 2 instead of 1. Concurrently, Channels configuration moves from database 7 to database 3, and the default cache location changes from a hardcoded localhost URL (redis://127.0.0.1:6379/6) to a parameterized configuration using database 0 (redis://{redis_host}/0). The non-testing Channels config structure was also condensed to a single-line format.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required [chores] prefix and clearly describes the main change: separating session Redis storage from cache storage, which aligns with the changeset modifications to Redis database indices.
Description check ✅ Passed The description includes checked checklist items, references the improvement, and explains the purpose, though it lacks a specific issue link and screenshot section is omitted.
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.
Bug Fixes ✅ Passed PR is a chores change to test configuration, not a bug fix. Check for core functionality bug fixes is inapplicable to configuration-only changes.

✏️ 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 separate-session-storage

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.

@openwisp-companion

Copy link
Copy Markdown
<h3>CI Failures Detected</h3>

Hello @nemesifier,
(Analysis for commit 46a5dec)

  • Test Failure: test_invalid_email_raise_validation_error in test_views.py failed with an ImportError because the django.contrib.sessions.backends module does not define a db attribute. This is likely due to an incorrect SESSION_ENGINE setting.

  • Fix: Ensure settings.SESSION_ENGINE is correctly configured, typically to 'django.contrib.sessions.backends.db'.

  • Test Failure: test_get_is_verified_user_admin_list_avoids_nplus1_queries in test_admin.py failed with an AssertionError because 7 SQL queries were executed, but 8 were expected. This indicates a potential inefficiency or missing query in the test setup or the code being tested.

  • Fix: Review the test to understand why the expected query is not being executed. It might be related to how related objects are prefetched or accessed.

  • Infrastructure Failure: The geckodriver.log file was not found, and the coveralls command failed with exit code 1, indicating a potential issue with the test environment or the coverage reporting tool.

  • Note: This appears to be an infrastructure or transient issue. The CI system will automatically restart the job if this is a recurring problem.

@nemesifier nemesifier force-pushed the separate-session-storage branch from 76c761c to f2f3e04 Compare June 9, 2026 13:59
@coveralls

coveralls commented Jun 9, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 98.135%. remained the same — separate-session-storage into master

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