refactor(tests): Improve test structure

Author: CuriousLearner

tests/test_additional_coverage.py

@@ -1,7 +1,4 @@
-"""
-Additional tests to increase coverage for remaining uncovered lines
-Targeting: __init__.py (40-42), admin_base.py (223-224, 246-247, 337, 349-351, 356, 359), models.py (206-215)
-"""
+"""Tests for edge cases and error handling."""
 
 import sys
 from unittest.mock import MagicMock, patch
@@ -18,16 +15,15 @@
 
 
 class TestVersionParsing(TestCase):
-    """Test version parsing edge cases - covers __init__.py lines 40-42"""
+    """Test version parsing edge cases."""
 
     def test_version_with_build_metadata(self):
-        """Test version parsing with build metadata to cover line 40"""
+        """Test version parsing with build metadata."""
 
-        # Test the exact code path from __init__.py
-        # We need to test the actual regex parsing logic
+        # Test regex parsing logic for version strings
         import re
 
-        # Test a version string that matches the pattern with build metadata
+        # Test with build metadata
         version_string = "1.2.3-alpha.1+build.123"
         pattern = (
             r"(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?"
@@ -40,21 +36,21 @@ def test_version_with_build_metadata(self):
             version_info = (int(major), int(minor), int(patch))
             if prerelease:
                 version_info += (prerelease,)
-            if build:  # This covers line 40
+            if build:
                 version_info += (build,)
         else:
-            version_info = (0, 1, 0)  # This covers lines 41-42
+            version_info = (0, 1, 0)
 
         # Verify build metadata was included
         assert len(version_info) == 5
         assert version_info[-1] == "build.123"
 
     def test_version_parsing_fallback(self):
-        """Test version parsing fallback to cover lines 41-42"""
+        """Test version parsing with invalid input."""
 
         import re
 
-        # Test with invalid version string that won't match regex
+        # Invalid version string
         version_string = "invalid-version"
         pattern = (
             r"(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?"
@@ -70,14 +66,14 @@ def test_version_parsing_fallback(self):
             if build:
                 version_info += (build,)
         else:
-            version_info = (0, 1, 0)  # This covers lines 41-42
+            version_info = (0, 1, 0)
 
         # Should fallback to default
         assert version_info == (0, 1, 0)
 
 
 class TestAdminBaseExceptions(TestCase):
-    """Test admin base exception handling to increase coverage"""
+    """Test admin exception handling."""
 
     def setUp(self):
         self.admin = BaseAnonymizationAdmin(MaskingRule, AdminSite())
@@ -87,7 +83,7 @@ def setUp(self):
         self.request.META = {}
 
     def test_dry_run_batch_with_database_error(self):
-        """Test dry run batch database error - covers lines 223-224"""
+        """Test dry run batch operation with database error."""
 
         rule = MaskingRule.objects.create(
             table_name="test_table", column_name="test_column", function_expr="anon.fake_email()", enabled=True
@@ -101,13 +97,13 @@ def test_dry_run_batch_with_database_error(self):
 
             result = self.admin._execute_dry_run_batch(queryset, self.admin.apply_rule_operation, "apply")
 
-            # Should handle the exception and return it in errors (covers lines 223-224)
+            # Should handle the exception and return it in errors
             assert result["applied_count"] == 0
             assert len(result["errors"]) > 0
             assert "Database error" in result["errors"][0]
 
     def test_transaction_batch_with_database_error(self):
-        """Test transaction batch database error - covers lines 246-247"""
+        """Test transaction batch operation with database error."""
 
         rule = MaskingRule.objects.create(
             table_name="test_table", column_name="test_column", function_expr="anon.fake_email()", enabled=True
@@ -122,13 +118,13 @@ def test_transaction_batch_with_database_error(self):
 
             result = self.admin._execute_transaction_batch(queryset, self.admin.apply_rule_operation, "apply")
 
-            # Should handle the exception and return it in errors (covers lines 246-247)
+            # Should handle the exception and return it in errors
             assert result["applied_count"] == 0
             assert len(result["errors"]) > 0
             assert "Transaction failed" in result["errors"][0]
 
     def test_enable_rules_no_effect(self):
-        """Test enable rules when no rules need enabling - covers line 337"""
+        """Test enable operation when all rules are already enabled."""
 
         # Create rules that are already enabled
         rule1 = MaskingRule.objects.create(
@@ -150,11 +146,11 @@ def test_enable_rules_no_effect(self):
         with patch.object(self.admin, "message_user") as mock_message:
             self.admin.enable_rules_operation(self.request, queryset)
 
-            # Should show warning message (covers line 337)
+            # Should show warning message
             mock_message.assert_called_with(self.request, "No rules were enabled", level=messages.WARNING)
 
     def test_disable_rules_with_save_failure(self):
-        """Test disable rules with save failure - covers lines 349-351"""
+        """Test disable operation when save fails."""
 
         rule = MaskingRule.objects.create(
             table_name="test_table", column_name="test_column", function_expr="anon.fake_email()", enabled=True
@@ -168,11 +164,11 @@ def test_disable_rules_with_save_failure(self):
                 with patch.object(self.admin, "message_user"):
                     self.admin.disable_rules_operation(self.request, queryset)
 
-                    # Should log the error (covers lines 349-351)
+                    # Should log the error
                     mock_logger.error.assert_called()
 
     def test_disable_rules_no_enabled_rules(self):
-        """Test disable rules when no rules are enabled - covers line 359"""
+        """Test disable operation when all rules are already disabled."""
 
         # Create rules that are already disabled
         rule = MaskingRule.objects.create(
@@ -187,12 +183,12 @@ def test_disable_rules_no_enabled_rules(self):
         with patch.object(self.admin, "message_user") as mock_message:
             self.admin.disable_rules_operation(self.request, queryset)
 
-            # Should show warning message (covers line 359)
+            # Should show warning message
             mock_message.assert_called_with(self.request, "No rules were disabled", level=messages.WARNING)
 
 
 class TestSignalDatabaseOperations(TestCase):
-    """Test signal database operations - covers models.py lines 206-215"""
+    """Test signal database operations."""
 
     def test_signal_database_execution_success(self):
         """Test successful database operation in signal"""
@@ -243,7 +239,7 @@ def test_signal_database_execution_with_exception(self):
         rule.enabled = False
 
         # Let the signal execute naturally - it will hit the database
-        # and handle the error gracefully (covers lines 216-219)
+        # and handle the error gracefully
         from django_postgres_anon.models import handle_rule_disabled
 
         # This should execute the database code and handle any exceptions

tests/test_admin.py

@@ -363,7 +363,7 @@ def test_admin_changelist_view_functions_correctly(admin_setup):
         pass
 
 
-# Additional tests for admin_base.py coverage
+# Additional admin tests
 
 
 @pytest.mark.django_db

tests/test_commands.py

@@ -573,7 +573,7 @@ def test_anon_drop_interactive_confirmation_accepted(_mock_input):
         output = call_command_with_output("anon_drop", "--table", "auth_user")
         assert "removal" in output.lower()
         # Verify mock was used
-        assert True  # Mock may or may not be called depending on implementation
+        pass
 
     except CommandError as e:
         # May fail due to missing extension
@@ -791,7 +791,7 @@ def test_anon_fix_permissions_command():
             output = str(mock_stdout.write.call_args_list)
             assert "Failed" in output or "failed" in output.lower()
 
-    # Test with database error in permission fix (essential for error handling coverage)
+    # Test with database error in permission fix
     from io import StringIO
 
     from django.db import DatabaseError
@@ -809,53 +809,13 @@ def test_anon_fix_permissions_command():
     MaskedRole.objects.all().delete()
 
 
-@pytest.mark.django_db
-def test_create_masked_role_behavior():
-    """Test create_masked_role behavioral outcomes"""
-    from django_postgres_anon.utils import create_masked_role
-
-    # Behavioral test: create_masked_role should handle various scenarios gracefully
-    # and return a boolean indicating success/failure
-
-    # Test 1: Creating a role (may succeed or fail depending on DB permissions)
-    result = create_masked_role("behavioral_test_role")
-    assert isinstance(result, bool)  # Should always return a boolean
-
-    # Test 2: Creating a role with inheritance (should handle missing base role gracefully)
-    result_with_inheritance = create_masked_role("test_role_with_inheritance", inherit_from="nonexistent_base")
-    assert isinstance(result_with_inheritance, bool)  # Should handle gracefully
-
-
-@pytest.mark.django_db
-def test_role_switching_behavior():
-    """Test role switching behavioral outcomes"""
-    from django_postgres_anon.context_managers import database_role
-    from django_postgres_anon.utils import switch_to_role
-
-    # Behavioral test: switch_to_role should return boolean indicating success
-    result = switch_to_role("test_role", auto_create=False)
-    assert isinstance(result, bool)
-
-    # Behavioral test: switch_to_role with auto_create should handle creation attempts
-    result_with_create = switch_to_role("test_role_auto", auto_create=True)
-    assert isinstance(result_with_create, bool)
-
-    # Behavioral test: database_role context manager should handle nonexistent roles
-    try:
-        with database_role("nonexistent_role"):
-            pass
-    except RuntimeError:
-        # This is expected behavior for nonexistent roles
-        pass
-
-
 @pytest.mark.django_db
 def test_masked_role_record_management():
-    """Test behavioral aspects of masked role record management"""
+    """Test masked role record management"""
     from django_postgres_anon.context_managers import _update_masked_role_record
     from django_postgres_anon.models import MaskedRole
 
-    # Behavioral test: updating role record for existing applied role should not create duplicates
+    # Updating role record for existing applied role should not create duplicates
     MaskedRole.objects.create(role_name="existing_role", is_applied=True)
     initial_count = MaskedRole.objects.filter(role_name="existing_role").count()
 
@@ -865,34 +825,5 @@ def test_masked_role_record_management():
     final_count = MaskedRole.objects.filter(role_name="existing_role").count()
     assert final_count == initial_count
 
-    # Behavioral test: updating role record should handle errors gracefully (no exceptions)
-    try:
-        _update_masked_role_record("any_role_name")
-        # Should complete without raising exceptions
-    except Exception:
-        pytest.fail("_update_masked_role_record should handle errors gracefully")
-
     # Cleanup
     MaskedRole.objects.all().delete()
-
-
-@pytest.mark.django_db
-def test_role_creation_with_inheritance():
-    """Test role creation behavioral aspects with inheritance"""
-    from django_postgres_anon.utils import create_masked_role, switch_to_role
-
-    # Behavioral test: Role creation with inheritance should handle missing base roles gracefully
-    result = create_masked_role("test_inheritance_role", inherit_from="nonexistent_base_role")
-    assert isinstance(result, bool)  # Should return boolean regardless of base role existence
-
-    # Behavioral test: Role creation with valid inheritance should work
-    result_valid = create_masked_role("test_role_2", inherit_from="postgres")  # postgres typically exists
-    assert isinstance(result_valid, bool)
-
-    # Behavioral test: Masked roles should attempt to set appropriate search paths
-    # This tests the "mask" in role name behavior without mocking internals
-    result_masked = switch_to_role("mask_test_role", auto_create=False)
-    assert isinstance(result_masked, bool)
-
-    result_regular = switch_to_role("regular_test_role", auto_create=False)
-    assert isinstance(result_regular, bool)

tests/test_core.py

@@ -271,9 +271,9 @@ def test_masked_role_string_representation(self):
 class TestAnonConfiguration:
     """Test anonymization configuration behavior"""
 
-    def test_config_has_essential_properties(self):
-        """Configuration provides all essential settings"""
-        essential_properties = [
+    @pytest.mark.parametrize(
+        "prop",
+        [
             "default_masked_role",
             "masked_group",
             "anonymized_data_role",
@@ -282,15 +282,15 @@ def test_config_has_essential_properties(self):
             "validate_functions",
             "allow_custom_functions",
             "enable_logging",
-        ]
-
-        for prop in essential_properties:
-            assert hasattr(anon_config, prop)
+        ],
+    )
+    def test_config_has_essential_properties(self, prop):
+        """Configuration provides all essential settings"""
+        assert hasattr(anon_config, prop)
 
-    def test_config_provides_sensible_defaults(self):
-        """Configuration has reasonable default values"""
-        # These should not be None
-        non_null_defaults = [
+    @pytest.mark.parametrize(
+        "prop",
+        [
             "default_masked_role",
             "masked_group",
             "anonymized_data_role",
@@ -299,11 +299,12 @@ def test_config_provides_sensible_defaults(self):
             "validate_functions",
             "allow_custom_functions",
             "enable_logging",
-        ]
-
-        for prop in non_null_defaults:
-            value = getattr(anon_config, prop)
-            assert value is not None, f"Config {prop} should have a default value"
+        ],
+    )
+    def test_config_provides_sensible_defaults(self, prop):
+        """Configuration has reasonable default values"""
+        value = getattr(anon_config, prop)
+        assert value is not None, f"Config {prop} should have a default value"
 
     def test_config_types_are_correct(self):
         """Configuration values have expected types"""