Code Quality Improvement Plan

This document outlines a concrete, incremental plan to improve code quality across the repository with a focus on:

• Eliminating wildcard (*) imports with a single allowed exception
• Eliminating relative imports in favor of absolute imports
• Enforcing import style and symbol access patterns
• Eliminating non-essential use of reflection helpers (getattr, hasattr, setattr) including common Django patterns
• Creating centralized configuration module to replace direct Django settings access with typed, validated functions
• Aligning with quick-rules.md and project conventions in CLAUDE.md
• Updating contributor documentation with best practices and examples
• Establishing enforcement via tooling and CI
• Optional: Adding type hints to improve static analysis and maintainability

References:

• See CLAUDE.md for architecture- and project-specific conventions
• See quick-rules.md for OpenStack Python quick rules

Current Linters and Hooks (as of this commit)

• Ruff is configured in pyproject.toml with line length 79 and Python 3.10 target. Selected rules include E4, E7, E9, F, S, U, W, C90. Security rules S101, S104, S105, S106, S110, S311 are ignored by policy, and McCabe complexity is capped at 20.
• Pre-commit runs:
  • Core hygiene hooks (trailing whitespace, mixed line endings, etc.)
  • remove-tabs (non-SVG)
  • hacking (OpenStack style) with docs excluded
  • ruff-check with --fix --unsafe-fixes
  • autopep8 on Python files
  • codespell (using doc/dictionary.txt for ignored words)
  • sphinx-lint and doc8 for docs
• Note: Star import exceptions should be modeled via per-file ignores. Today, only watcher/tests/* is configured to ignore S rules. For this plan, we will explicitly whitelist the test settings file for F403/F405.

Policy: Imports and Symbol Access

• Avoid wildcard imports (from module import *).
  • Sole exception: watcher_dashboard/test/settings.py where star imports are acceptable in the test setup (as already permitted by flake8 ignore F405).
• Avoid relative imports - use absolute imports from the project root.
  • Bad: from . import utils or from ..common import client
  • Good: from watcher_dashboard import utils or import watcher_dashboard.common.client as client_mod
• Prefer importing modules or submodules, not individual functions, classes, or constants. Access members via the module or its alias.
  • Good: import module.submodule as mod; then mod.ClassName, mod.func()
  • Avoid: from module.submodule import ClassName, SOME_CONST
• Submodule import clarification:
  • Acceptable: from watcher_dashboard.content import audits (submodule)
  • Acceptable: from watcher_dashboard.content.audits import forms as audit_forms (submodule with alias)
  • NOT acceptable: from watcher_dashboard.content.audits.forms import CreateAuditForm (importing class/function)
  • Rule: If the import target is itself a module/submodule that you'll call functions on, it's acceptable. If you're importing a symbol (class, function, constant) from a module, use module-level import instead.
• Typing-only imports carve-out: Direct symbol imports are permitted only under if TYPE_CHECKING: blocks to support type hints and avoid import cycles. Prefer from __future__ import annotations to reduce runtime import needs. This is the sole exception to the "no direct symbol imports" rule.
• Keep imports explicit, deterministic, and minimal; avoid dynamic import tricks that hinder static analysis.
• Import ordering and grouping: The hacking pre-commit hook enforces OpenStack import order per quick-rules.md:
  1. Standard library imports
  2. Third-party imports (including from third-party import X)
  3. Local project imports
  4. Blank line between each group
  5. Within each group, alphabetically sorted
• Respect line length (79 chars) and use delayed logging formatting.

Policy: Reflection Helpers

Summary: The long-term goal is to eliminate all reflection helpers except where attribute names are truly dynamic (runtime-determined). Common Django patterns like getattr(settings, 'KEY', default) are acceptable during transition (Phase 4a) but will be eliminated via a centralized config module (Phase 4b).

• Strongly discourage non-required usage of getattr, hasattr, setattr.
  • Only use when the attribute name is inherently dynamic (e.g., user input, feature flags, plugin slots, API version compatibility). Document the rationale at the call site.
  • Replace all static or predictable attribute access via normal Python member access, direct dictionary access, or explicit mappings.
  • Avoid hasattr for flow control when simple is not None or explicit interfaces suffice.
• Short-term acceptable use cases during transition (will be eliminated in Phase 4b):
  • Django settings access with defaults: getattr(settings, 'KEY', default)
  • API version feature detection: hasattr(client, 'new_api_method')
  • Dynamic form field access in loops: getattr(form.cleaned_data, field_name)
  • Plugin system attribute access where names come from configuration
  • Note: These patterns, while common in Django, hinder static analysis and should be replaced with explicit, type-safe alternatives.
• Long-term architecture goal:
  • Create watcher_dashboard.config module to centralize all configuration access
  • Provide explicit functions for retrieving, defaulting, and validating config
  • Replace all direct Django settings access with typed config module functions
  • Enable static analysis and eliminate reflection where possible
• Permanently acceptable use cases (will remain after all phases):
  • API microversion feature detection where method availability varies by version
  • Plugin/extension systems where attribute names come from external config
  • Truly dynamic attribute access where the name is determined at runtime
  • These cases must have inline comments explaining the dynamic nature
• Unacceptable use cases (refactor immediately):
  • Static attribute names known at development time
  • Object attributes that are part of the documented interface
  • Dictionary-like access (use dict.get() or dict[] instead)

Pattern Library

Incorrect vs. Correct examples for common cases.

Imports:

# Incorrect
from pkg.mod import Thing, HELPER
from pkg.mod import *  # not allowed (except test settings)

# Correct
import pkg.mod as mod

obj = mod.Thing()
value = mod.HELPER

Views/Forms/Utilities accessing project APIs:

# Incorrect
from watcher_dashboard.common.client import get_client
client = get_client(request)

# Correct
import watcher_dashboard.common.client as client_mod
client = client_mod.get_client(request)

Reflection helpers:

# Incorrect: static attribute name via getattr
name = getattr(user, 'username')

# Correct: direct attribute access
name = user.username

# Incorrect: hasattr flow control for known attribute
if hasattr(conf, 'enabled'):
    if getattr(conf, 'enabled'):
        do_thing()

# Correct: direct access with clear defaults
if conf.enabled:
    do_thing()

# Short-term acceptable: Django settings with default
# This is a temporary exception during Phase 4a
policy_files = getattr(settings, 'POLICY_FILES', {})  # TODO: migrate to config module

# Long-term preferred: Centralized config module (Phase 4b goal)
import watcher_dashboard.config as watcher_config

policy_files = watcher_config.get_policy_files()  # Explicit, typed, validated

# Short-term acceptable: API microversion feature detection
# Will remain acceptable as this is truly dynamic version compatibility
if hasattr(client, 'audit_create_with_start_end'):  # OK: version compat
    result = client.audit_create_with_start_end(params)
else:
    result = client.audit_create(params)

# Short-term acceptable: Dynamic form field iteration
# Consider refactoring to explicit field access where possible
for field_name in required_fields:
    # OK: field_name from loop variable
    value = getattr(self.cleaned_data, field_name, None)
    if not value:
        raise ValidationError(f'{field_name} is required')

# Permanently acceptable: truly dynamic attribute access
# When the attribute name comes from external input/config
attr_name = plugin_config.primary_attribute  # dynamic by design
value = getattr(obj, attr_name, None)  # acceptable with comment

Submodule imports:

# Incorrect: importing symbols (classes/functions/constants)
from watcher_dashboard.content.audits.forms import CreateAuditForm
from watcher_dashboard.api.watcher import Audit, Goal

# Correct: import submodule, access symbols via module
from watcher_dashboard.content.audits import forms as audit_forms
from watcher_dashboard.api import watcher

form = audit_forms.CreateAuditForm()
audit = watcher.Audit.create(request, **params)

# Also acceptable: import submodule directly
from watcher_dashboard.content import audits

form = audits.forms.CreateAuditForm()

Typing-only imports for type hints:

from __future__ import annotations
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    # Allowed exclusively for typing; not imported at runtime
    from watcher_dashboard.api.watcher import Audit

import watcher_dashboard.api.watcher as watcher_api

def get_audit(audit_id: str) -> "Audit":
    return watcher_api.Audit.get(audit_id)

Configuration access (Phase 4b migration):

# Current (Phase 4a): Acceptable temporarily with TODO comment
from django.conf import settings

def insert_watcher_policy_file():
    # TODO: migrate to config module
    policy_files = getattr(settings, 'POLICY_FILES', {})
    policy_files['infra-optim'] = 'watcher_policy.json'
    setattr(settings, 'POLICY_FILES', policy_files)

# After Phase 4b: Using centralized config module
import watcher_dashboard.config as watcher_config

def insert_watcher_policy_file():
    """Register Watcher policy file with Django settings.
    
    Note: This function still modifies settings but retrieves
    config through the centralized module.
    """
    policy_files = watcher_config.get_policy_files()
    policy_files['infra-optim'] = watcher_config.get_watcher_policy_file()
    # Settings mutation still needed here; but retrieval is centralized

# Better Phase 4b: Config module handles the logic
import watcher_dashboard.config as watcher_config

def setup_application():
    """Initialize application configuration."""
    watcher_config.register_watcher_policy()  # Logic moved to config module


# In watcher_dashboard/config.py:
def register_watcher_policy() -> None:
    """Register Watcher policy file with Django.
    
    This function should be called during application initialization.
    """
    from django.conf import settings
    policy_files = getattr(settings, 'POLICY_FILES', {})
    policy_files['infra-optim'] = 'watcher_policy.json'
    setattr(settings, 'POLICY_FILES', policy_files)

Feature flags (manila-ui pattern, enhanced):

# manila-ui current pattern (incomplete - no validation/type hints):
from django.conf import settings
from horizon.utils import memoized

@memoized.memoized
def is_share_groups_enabled():
    manila_config = getattr(settings, 'OPENSTACK_MANILA_FEATURES', {})
    return manila_config.get('enable_share_groups', True)

# Enhanced pattern for watcher-dashboard (Phase 4b goal):
from django.conf import settings
from horizon.utils import memoized


@memoized.memoized
def is_continuous_audit_enabled() -> bool:
    """Check if continuous audit feature is enabled.
    
    :returns: True if continuous audits are enabled, False otherwise
    :raises TypeError: If setting cannot be converted to bool
    """
    watcher_features = getattr(settings, 'WATCHER_FEATURES', {})
    if not isinstance(watcher_features, dict):
        raise TypeError(
            f"WATCHER_FEATURES must be a dict, got {type(watcher_features)}"
        )
    
    enabled = watcher_features.get('enable_continuous_audit', True)
    
    # Validate bool conversion
    if not isinstance(enabled, bool):
        if isinstance(enabled, str):
            enabled_lower = enabled.lower()
            if enabled_lower in ('true', 'yes', '1', 'on'):
                return True
            elif enabled_lower in ('false', 'no', '0', 'off'):
                return False
            else:
                raise TypeError(f"Cannot convert {enabled!r} to bool")
        else:
            raise TypeError(
                f"enable_continuous_audit must be bool or str, "
                f"got {type(enabled)}"
            )
    
    return enabled


# Usage in views/forms:
import watcher_dashboard.config as watcher_config

def get_context_data(self, **kwargs):
    context = super().get_context_data(**kwargs)
    # Clear, typed, validated access
    context['continuous_audit_enabled'] = (
        watcher_config.is_continuous_audit_enabled()
    )
    return context

Repository Audit and Remediation Steps

Phase 1 — Discovery (read-only)

• Enumerate all star imports and direct symbol imports (functions/classes/ constants) across the repo.
• Enumerate getattr, hasattr, setattr usages and classify each as required (dynamic) or removable (static).
• Produce a short report (path, line, category) to guide edits.

Phase 2 — Policy Updates (no code behavior changes)

• Add/confirm flake8 configuration to discourage unwanted patterns:
  • Keep ignore = F405,W504 only for tests; prefer per-file ignore.
  • Add per-file-ignores for watcher_dashboard/test/settings.py:F405.
  • Optionally enable H* hacking rules consistent with quick-rules.md.
• Ensure pre-commit runs flake8, isort (configured to keep module-only imports), and other relevant hooks.

Phase 3 — Import Cleanup

• Replace all from X import Y where Y is a function/class/constant with module-level imports and qualified access, except when Y is a submodule.
• Remove all wildcard imports except the allowed test settings file.
• Update call sites to use module.symbol access.

Phase 4a — Immediate Reflection Cleanup

• Replace static getattr/hasattr/setattr with direct access or explicit conditionals where the attribute name is known at development time.
• For truly dynamic cases and short-term acceptable patterns, add inline comment justifying usage and ensure safe defaults are provided to getattr.
• Mark Django settings access with # TODO: migrate to config module comments.
• Preserve API version feature detection patterns as these are legitimately dynamic.

Phase 4b — Config Module Creation and Migration

• Create watcher_dashboard/config.py module to centralize configuration access.
• Real-world precedent: The manila-ui project uses this pattern for feature flags. We enhance their approach by adding type hints and validation.
• Design principles:
  • Provide explicit, typed functions for each configuration option
  • Include validation and type checking where appropriate
  • Provide sensible defaults matching current behavior
  • Enable static analysis and IDE autocomplete
  • Document each config option with docstrings
• Implementation approach:
  • Audit all getattr(settings, ...) calls in the codebase
  • Create corresponding functions in config module (e.g., get_policy_files() -> dict)
  • Add type hints to all config functions (Phase 4b pairs well with Phase 8)
  • Implement validation for complex config options
  • Update all Django settings access to use config module
  • Remove # TODO: migrate to config module comments after migration
• Example config module structure (inspired by manila-ui features.py):

  # watcher_dashboard/config.py
  """Centralized configuration access for Watcher Dashboard.
  
  This module provides typed, validated access to Django settings,
  eliminating the need for getattr() reflection helpers throughout
  the codebase.
  
  Pattern inspired by manila-ui but enhanced with:
  - Type hints for all return values
  - Validation before returning values
  - Comprehensive docstrings
  """
  from django.conf import settings
  from horizon.utils import memoized
  
  
  @memoized.memoized
  def get_policy_files() -> dict[str, str]:
      """Get the policy files configuration.
      
      :returns: Dictionary mapping service names to policy file names
      :raises TypeError: If POLICY_FILES is not a dictionary
      """
      policy_files = getattr(settings, 'POLICY_FILES', {})
      if not isinstance(policy_files, dict):
          raise TypeError(
              f"POLICY_FILES must be a dict, got {type(policy_files)}"
          )
      return policy_files
  
  
  @memoized.memoized
  def get_watcher_policy_file() -> str:
      """Get the Watcher policy file name.
      
      :returns: Policy file name for Watcher service
      """
      policy_files = get_policy_files()
      return policy_files.get('infra-optim', 'watcher_policy.json')
  
  
  @memoized.memoized
  def get_watcher_microversion_default() -> str:
      """Get the default Watcher API microversion.
      
      :returns: Default API microversion string (e.g., '1.0')
      :raises ValueError: If version string is empty
      """
      version = getattr(settings, 'WATCHER_API_VERSION', '1.0')
      if not isinstance(version, str) or not version:
          raise ValueError(
              f"WATCHER_API_VERSION must be a non-empty string, "
              f"got {version!r}"
          )
      return version
  
  
  @memoized.memoized
  def is_continuous_audit_enabled() -> bool:
      """Check if continuous audit feature is enabled.
      
      Example of feature flag pattern from manila-ui, enhanced with
      validation.
      
      :returns: True if continuous audits are enabled, False otherwise
      :raises TypeError: If setting value cannot be converted to bool
      """
      watcher_features = getattr(settings, 'WATCHER_FEATURES', {})
      if not isinstance(watcher_features, dict):
          raise TypeError(
              f"WATCHER_FEATURES must be a dict, got "
              f"{type(watcher_features)}"
          )
      
      enabled = watcher_features.get('enable_continuous_audit', True)
      
      # Validate that value can be converted to bool
      if not isinstance(enabled, bool):
          # Try to convert string values to bool
          if isinstance(enabled, str):
              enabled_lower = enabled.lower()
              if enabled_lower in ('true', 'yes', '1', 'on'):
                  return True
              elif enabled_lower in ('false', 'no', '0', 'off'):
                  return False
              else:
                  raise TypeError(
                      f"Cannot convert string {enabled!r} to bool"
                  )
          else:
              raise TypeError(
                  f"enable_continuous_audit must be bool or str, "
                  f"got {type(enabled)}"
              )
      
      return enabled
  
  
  def register_watcher_policy() -> None:
      """Register Watcher policy file with Django.
      
      This function should be called during application initialization.
      Note: This is one of the few places where settings mutation is
      necessary and getattr/setattr are acceptable.
      """
      from django.conf import settings
      policy_files = getattr(settings, 'POLICY_FILES', {})
      policy_files['infra-optim'] = 'watcher_policy.json'
      setattr(settings, 'POLICY_FILES', policy_files)

• Benefits:
  • Eliminates reflection for configuration access
  • Enables static analysis and type checking
  • Centralizes defaults and validation logic
  • Improves discoverability (IDE autocomplete)
  • Easier to test and mock in unit tests
  • Clearer documentation of available config options
  • Proven pattern: Used successfully in manila-ui and other OpenStack Horizon plugins
• Notes:
  • Phase 4b pairs excellently with Phase 8 (Type Hints). The config module should be fully type-hinted from creation, serving as a model for the rest of the codebase.
  • Consider implementing Phase 4b and Phase 8 together for the config module.
  • The @memoized.memoized decorator from horizon.utils.memoized provides efficient caching of config lookups, similar to manila-ui.

Phase 5 — Documentation & Examples

• Expand contributor docs to describe these practices:
  • Update doc/source/contributor/index.rst to link to this guide.
  • Add a new doc/source/contributor/code_quality.rst summarizing policies and examples; cross-link to code-quality.md and quick-rules.md.
• Add a brief section in README.rst pointing to contributor guidelines.

Phase 6 — CI Gating

• Update tox.ini:
  • Add per-file-ignores = watcher_dashboard/test/settings.py:F405.
  • Keep W504 ignore as documented.
  • Ensure [testenv:pep8] runs pre-commit and flake8 consistently.
• Add a CI job to fail on newly introduced star imports or direct symbol imports outside allowed exceptions.

Phase 7 — Enforcement and Ongoing Maintenance

• Add a lightweight custom check script (optional) to scan for banned patterns and run in pre-commit/CI.
• Document exceptions in-code with justification comments and limit scope.
• Periodically re-run discovery to ensure drift is caught early.

Phase 8 — Type Hints (Optional Enhancement)

• Gradually add type hints to improve code maintainability and enable static analysis.
• Target Python 3.10+ syntax (using | for unions, list[str] instead of List[str]).
• Implementation approach:
  • Start with API layer (watcher_dashboard/api/watcher.py) - add return types to wrapper methods
  • Add type hints to client helper (watcher_dashboard/common/client.py)
  • Type-hint form classes and validation methods in content modules
  • Add type hints to utility functions (watcher_dashboard/utils/)
• Configuration:
  • Add mypy to test-requirements.txt
  • Create mypy.ini or add [tool.mypy] section to pyproject.toml
  • Start with check_untyped_defs = false and gradually tighten
  • Add mypy pre-commit hook (optional, non-blocking initially)
• Do not require 100% coverage immediately; focus on new code and critical paths first.
• Add py.typed marker file when sufficient coverage is achieved (>70% of public API).

Tooling Configuration (proposed)

Ruff (pyproject.toml current):

[tool.ruff]
line-length = 79
target-version = "py310"

[tool.ruff.lint]
select = ["E4", "E7", "E9", "F", "S", "U", "W", "C90"]
ignore = [
  "S101", "S311", "S104", "S105", "S106", "S110",
]

[tool.ruff.lint.per-file-ignores]
"watcher/tests/*" = ["S"]
# Add the explicit star-import exception for test settings:
"watcher_dashboard/test/settings.py" = ["F403", "F405"]

[tool.ruff.lint.mccabe]
max-complexity = 20

Future enforcement (planned):

• After Phase 3 removes relative imports, enable Ruff tidy-imports rule TID252 (via flake8-tidy-imports) to gate on absolute imports only. Example delta to pyproject.toml once the codebase is clean:

[tool.ruff.lint]
select = ["E4", "E7", "E9", "F", "S", "U", "W", "C90", "TID"]

Pre-commit (.pre-commit-config.yaml current):

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v5.0.0
    hooks:
      - id: trailing-whitespace
      - id: mixed-line-ending
        args: ['--fix', 'lf']
        exclude: '.*\.(svg)$'
      - id: check-byte-order-marker
      - id: check-ast
      - id: debug-statements
      - id: check-json
        files: .*\.json$
      - id: check-yaml
        files: .*\.(yaml|yml)$
      - id: check-executables-have-shebangs
      - id: check-shebang-scripts-are-executable
      - id: check-added-large-files
      - id: check-case-conflict
      - id: detect-private-key
      - id: check-merge-conflict
        exclude: '.*\.(rst|inc)$'
  - repo: https://github.com/Lucas-C/pre-commit-hooks
    rev: v1.5.5
    hooks:
      - id: remove-tabs
        exclude: '.*\.(svg)$'
  - repo: https://opendev.org/openstack/hacking
    rev: 7.0.0
    hooks:
      - id: hacking
        additional_dependencies: []
        exclude: '^(doc|releasenotes|tools)/.*$'
  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.12.1
    hooks:
      - id: ruff-check
        args: ['--fix', '--unsafe-fixes']
  - repo: https://github.com/hhatto/autopep8
    rev: v2.3.2
    hooks:
      - id: autopep8
        files: '^.*\.py$'
  - repo: https://github.com/codespell-project/codespell
    rev: v2.4.1
    hooks:
      - id: codespell
        args: ['--ignore-words=doc/dictionary.txt']
  - repo: https://github.com/sphinx-contrib/sphinx-lint
    rev: v1.0.0
    hooks:
      - id: sphinx-lint
        args: [--enable=default-role]
        files: ^doc/|^releasenotes/|^api-guide/
        types: [rst]
  - repo: https://github.com/PyCQA/doc8
    rev: v1.1.2
    hooks:
      - id: doc8

Notes:

• Prefer Ruff as the primary linter; hacking complements OpenStack style.
• Star-import exception is enforced via the per-file ignore above.
• The hacking hook automatically enforces import ordering per OpenStack standards; Ruff import rules (I) are not needed.

Mypy configuration (for Phase 8 — Type Hints):

[tool.mypy]
python_version = "3.10"
warn_return_any = true
warn_unused_configs = true
disallow_untyped_defs = false
check_untyped_defs = false
warn_redundant_casts = true
warn_unused_ignores = true
show_error_codes = true
namespace_packages = true
explicit_package_bases = true

[[tool.mypy.overrides]]
module = [
    "horizon.*",
    "openstack_dashboard.*",
    "watcherclient.*",
]
ignore_missing_imports = true

Add to pre-commit (optional, for Phase 8):

  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v1.11.2
    hooks:
      - id: mypy
        additional_dependencies: [types-PyYAML]
        files: ^watcher_dashboard/(api|common|utils)/.*\.py$
        # Start with limited scope, expand gradually

Acceptance Criteria

Core Requirements (Phases 1-7):

• No wildcard imports remain except in watcher_dashboard/test/settings.py.
• No relative imports; all imports are absolute from project root.
• No direct function/class/constant imports remain; all access is via module or module alias (submodule imports allowed per clarified policy).
• All non-dynamic reflection helper uses are removed (Phase 4a complete).
• Centralized watcher_dashboard.config module created and all Django settings access migrated to use explicit typed functions (Phase 4b complete).
• Only truly dynamic reflection remains (API version detection, plugin systems); all instances have inline comments justifying dynamic access.
• No # TODO: migrate to config module comments remain.
• Contributor docs updated and linked from the main docs.
• CI gates on violations and pre-commit runs locally without errors.
• Import ordering follows OpenStack standards (enforced by hacking hook).

Optional Enhancement (Phase 8):

• Type hints added to API layer, client helper, and utility modules.
• Mypy configuration in place with appropriate strictness level.
• py.typed marker added when >70% public API coverage achieved.

Testing Strategy

Goals:

• Establish a comprehensive stdlib-based unit test suite that runs quickly and deterministically in CI.
• Add a future Playwright-powered end-to-end (E2E) suite to validate critical flows across Horizon + Watcher Dashboard.
• Keep all existing test configuration intact; enhance via additional envs and documentation.

References:

• Pragmatic testing strategy (adapted):
  • https://raw.githubusercontent.com/SeanMooney/grian-dev/refs/heads/master/scratch/planning/testing-strategy.md
• Horizon plugin system implications for testing:
  • https://raw.githubusercontent.com/SeanMooney/grian-dev/refs/heads/master/scratch/analysis/horizon-plugin-system-reference.md

Current Unit Test Baseline (as-is)

Observed coverage and patterns in watcher_dashboard/test/:

• Test harness and mixins:

  • test/helpers.py defines APITestCase stubbing watcher_dashboard.api.watcher.watcherclient via setUp/tearDown, and loads structured test data (test/test_data/*).
  • Mixins extend Horizon’s helpers.TestCase/APITestCase.

• API layer unit tests:

  • test/api_tests/test_client.py validates API microversion selection for Audit.create and default behavior for Action.start.
  • test/api_tests/test_watcher.py extensively covers list/get/create/update/ delete across Goals, Strategies, Audit Templates, Audits, Action Plans, Actions; includes parameter serialization and start/end time handling.

• Utilities/UI unit tests:

  • test/test_formset_table.py verifies formset table population behavior.

• Integration/selenium scaffolding (excluded by default):

  • test/integration_tests/ contains Horizon-driven selenium tests (page objects for Audit Templates/Audits, config in horizon.conf, panel flows), and a Jasmine runner in test/selenium.py. These remain but are excluded by default via tox settings (--exclude-tag integration).

Conclusion: API wrapper coverage is solid; helpers and data loaders are in place. Gaps remain in forms/views/tables actions unit tests per content module, and broader negative-path coverage.

Unit Tests (stdlib-based)

Principles (adapted for gating CI):

• Every commit keeps tests green; prefer incremental value with passing tests.
• Test-First for well-defined behavior; Test-With during exploration.
• Bug reproducer pattern: write a test that documents the current incorrect behavior with a FIXME comment and commented-out correct assertions; then fix the bug and update the test atomically.

Scope and Structure:

• Keep tests within watcher_dashboard/test/ using Django TestCase and unittest + unittest.mock.
• Avoid integration tests by default (consistent with current tox settings); use the existing integration tag only when necessary.
• Organize by content module and layer:
  • test/api_tests: API wrappers and client helpers (mock watcherclient).
  • test/content/*: views, tables, forms, tabs per resource.
  • test/utils: utilities and helpers.

Patterns and Conventions:

• Use unittest.mock.patch(..., autospec=True) consistently (per quick-rules.md).
• Prefer module imports in tests mirroring production import policy.
• For forms:
  • Validate SelfHandlingForm handlers and YamlValidator usage.
  • Test success, validation errors, and exception handling via Horizon exceptions.
• For views:
  • Test get_context_data, permission checks, and template rendering with minimal fixtures.
• API wrapper tests:
  • Mock watcher_dashboard.common.client.get_client or watcher_dashboard.common.client.watcherclient and assert correct calls by microversion and parameters.
• Horizon-specific hygiene:
  • Clear horizon.utils.memoized caches between tests when necessary.
  • Confirm panel registration, URL conf inclusion, and policy gating where applicable (see plugin reference above).

Leverage Existing Patterns:

• Retain APITestCase stub of watcherclient from test/helpers.py and the test/test_data/* loaders to keep unit tests deterministic and fast.
• Mirror existing api_tests structure for new modules to keep consistency.

Tox/CI (non-destructive enhancements):

• Keep [testenv] defaults as-is. Optionally add a unit alias env that runs the same Django test command while remaining non-voting initially.
• Maintain --exclude-tag integration in default test runs.
• Use tox -e cover locally to inspect coverage; aim to increase gradually.

Acceptance Targets (incremental):

• Phase U1: Establish baseline tests for all six content modules’ Index/Detail views, one happy-path form submission per module, and API wrapper list/get.
• Phase U2: Add negative/error-path tests for forms and API wrappers.
• Coverage milestones: 60% (U1), 70% (U2), 80%+ (stretch).

File/Area Priorities (near-term):

• Forms: watcher_dashboard/content/*/forms.py (create/update, YAML params).
• Views: watcher_dashboard/content/*/views.py (Index/Detail workflows).
• Tables actions: watcher_dashboard/content/*/tables.py (row/table actions).
• API: add negative-path tests complementing existing positive-path coverage.

End-to-End Tests (Playwright) — Future Plan

Motivation:

• Current validation of UI flows is manual. Introduce automated browser tests to catch regressions across templates, static assets, and Horizon routing.

Tooling:

• Playwright Python: https://github.com/microsoft/playwright-python/tree/main
• Playwright MCP (for AI-assisted authoring): https://github.com/microsoft/playwright-mcp

Structure and Conventions:

• Place E2E tests under e2e/ (e.g., e2e/tests/).
• Prefer headless runs in CI; allow headed locally for debugging.
• Base URL configured via env var (e.g., E2E_BASE_URL=http://127.0.0.1:8000).

Minimal Scenarios (Phase E1):

• Smoke navigation for each module: Goals, Strategies, Audit Templates, Audits, Action Plans, Actions — ensure Index pages load and key UI elements render.
• Authentication flow using development credentials or mocked SSO if available.

Core Flows (Phase E2):

• Representative create/update/delete for at least one resource (e.g., Audit Templates), guarded by fixtures to avoid backend side-effects.
• Pagination and filtering checks on tables.

Environment and Orchestration:

• Local dev server started via ./run_tests.sh --runserver 127.0.0.1:8000 with watcher_dashboard.test.settings and ephemeral DB (sqlite).
• Seed minimal data via fixtures or API mocks exposed behind a feature flag.

Tox/CI Integration (additive, non-breaking):

• Add [testenv:e2e] (future) that installs Playwright, ensures browsers are available (playwright install --with-deps), starts the server in the background, runs E2E tests, and then tears down.
• Begin as non-voting in CI. Promote to voting once stable.

Reporting and Stability:

• Record videos and traces on failures for review.
• Tag E2E tests to allow selective runs (smoke vs. full).

Acceptance Targets (incremental):

• E1: Smoke tests cover navigation for all six modules; runs in CI (non-voting).
• E2: At least one core CRUD flow automated; CI remains stable.
• E3: Promote E2E to voting for smoke subset.

Selenium → Playwright Minimal Port (transition plan)

Scope (keep selenium intact initially):

• Do not remove selenium-based tests in watcher_dashboard/test/selenium.py and watcher_dashboard/test/integration_tests/*.
• Introduce a minimal Playwright smoke suite that replicates the most valuable selenium flows first, then expand.

Minimal Playwright Parity Targets (Phase P0):

• Replicate Audit Templates smoke flow from integration_tests/tests/test_audit_template_panel.py:
  • Load Audit Templates page
  • Create a new audit template (random name)
  • Verify template is listed
  • (Optional) Delete template and verify removal
• Replicate Audits page load smoke from pages/admin/optimization/auditspage.py:
  • Navigate to Audits page
  • Verify table renders and key UI elements are present

Structure:

• Directory: e2e/tests/ for Playwright tests.
• Shared helpers: e2e/support/ (auth/login, navigation, fixtures).
• Use role- and text-based locators for robustness.

Example Playwright test (Python):

import os
import re
from playwright.sync_api import Page, expect

BASE_URL = os.environ.get("E2E_BASE_URL", "http://127.0.0.1:8000")

def login(page: Page) -> None:
    page.goto(f"{BASE_URL}/auth/login/")
    page.get_by_label("Username").fill("admin")
    page.get_by_label("Password").fill("secretadmin")
    page.get_by_role("button", name=re.compile("sign in", re.I)).click()
    expect(page).to_have_title(re.compile("OpenStack Dashboard", re.I))

def test_audit_templates_smoke(page: Page) -> None:
    login(page)
    # Navigate to Admin → Optimization → Audit Templates
    page.get_by_role("link", name=re.compile("Admin")).click()
    page.get_by_role("link", name=re.compile("Optimization")).click()
    page.get_by_role("link", name=re.compile("Audit Templates")).click()
    expect(page).to_have_title(re.compile("Audit Templates", re.I))

    # Create a new audit template
    page.get_by_role("button", name=re.compile("Create", re.I)).click()
    name = f"audit_template_smoke"
    page.get_by_label(re.compile("Name")).fill(name)
    page.get_by_label(re.compile("Description")).fill("E2E smoke")
    page.get_by_label(re.compile("Goal")) .select_option("SERVER_CONSOLIDATION")
    page.get_by_role("button", name=re.compile("Create", re.I)).click()

    # Verify it appears in the table
    expect(page.get_by_role("row", name=re.compile(name))).to_be_visible()

Tox/CI integration (additive):

• Add [testenv:e2e] to install and run Playwright tests; keep non-voting.
• Ensure playwright install --with-deps runs in the environment before tests.
• Start Horizon dev server (./run_tests.sh --runserver 127.0.0.1:8000) in background for the duration of tests.

Acceptance for P0:

• Playwright smoke suite replicates selenium Audit Templates create/list and Audits page load checks.
• Runs reliably locally and in CI (non-voting), producing traces on failures.
• No removals of existing selenium tests during transition.

Out-of-Scope (for this plan)

• Behavioral refactors beyond import and reflection cleanup.
• Changes to external API surfaces or public contracts.