letsencrypt: Update and fix tests involving domain changes

- Remove checking of the sender is 'test' and use mocking.

- Remove need for tests being run as root.

- Simplify log messages, avoid pylint warnings.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

plinth/modules/letsencrypt/__init__.py

@@ -163,9 +163,8 @@ def on_domain_added(sender, domain_type='', name='', description='',
             return False
 
     try:
-        # Obtaining certs during tests or empty names isn't expected to succeed
+        if name:
-        if sender != 'test' and name:
+            logger.info('Obtaining certificate for %s', name)
-            logger.info('Obtaining a Let\'s Encrypt certificate for %s', name)
             certificate_obtain(name)
         return True
     except ActionError:
@@ -178,15 +177,13 @@ def on_domain_removed(sender, domain_type, name='', **kwargs):
         return False
 
     try:
-        # Revoking certs during tests or empty names isn't expected to succeed
+        if name:
-        if sender != 'test' and name:
+            logger.info('Revoking certificate for %s', name)
-            logger.info("Revoking the Let\'s Encrypt certificate for " + name)
             certificate_revoke(name)
         return True
     except ActionError as exception:
-        logger.warning(
+        logger.warning('Failed to revoke certificate for %s: %s', name,
-            ('Failed to revoke certificate for domain {domain}: {error}'
+                       exception.args[2])
-             ).format(domain=name, error=exception.args[2]))
         return False
 
 

plinth/modules/letsencrypt/tests/test_domain_name_changes.py

@@ -18,21 +18,72 @@
 Tests for letsencrypt module.
 """
 
+from unittest.mock import call, patch
+
 import pytest
 
+from plinth.modules.names.components import DomainType
+
 from .. import on_domain_added, on_domain_removed
 
-pytestmark = pytest.mark.usefixtures('needs_root', 'needs_sudo')
+
+@pytest.fixture(name='domain_types')
+def fixture_domain_types():
+    """Create a domain types required for tests."""
+    DomainType('domain-type-tor', 'Tor Hidden Service', 'tor:index',
+               can_have_certificate=False)
+    DomainType('domain-type-test', 'Test Domain Type', 'test:index')
 
 
+@pytest.mark.usefixtures('domain_types')
 def test_add_onion_domain():
+    """Test that .onion domains are ignored when added/removed."""
     assert not on_domain_added('test', 'domain-type-tor', 'ddddd.onion')
+    assert not on_domain_removed('test', 'domain-type-tor', 'ddddd.onion')
 
 
+@patch('plinth.modules.letsencrypt.get_status')
+@patch('plinth.modules.letsencrypt.certificate_obtain')
 @pytest.mark.usefixtures('load_cfg')
-def test_add_valid_domain():
+@pytest.mark.parametrize('domain,status_input,obtain,result', [
-    assert on_domain_added('test', 'domainname', 'subdomain.domain.tld')
+    ('domain1.tld', {
+        'certificate_available': True,
+        'validity': 'not-valid'
+    }, True, True),
+    ('domain2.tld', {
+        'certificate_available': False,
+        'validity': 'valid'
+    }, True, True),
+    ('domain3.tld', {
+        'certificate_available': True,
+        'validity': 'valid'
+    }, False, False),
+    ('', {
+        'certificate_available': False,
+        'validity': 'valid'
+    }, False, True),
+])
+def test_add_valid_domain(certificate_obtain, get_status, domain, status_input,
+                          obtain, result):
+    """Test adding a domain that can have certificates."""
+    get_status.return_value = {'domains': {domain: status_input}}
+    assert result == on_domain_added('test', 'domain-type-test', domain)
+    if obtain:
+        certificate_obtain.assert_has_calls([call(domain)])
+    else:
+        certificate_obtain.assert_not_called()
 
 
-def test_remove_domain():
+@patch('plinth.modules.letsencrypt.certificate_revoke')
-    assert on_domain_removed('test', '', 'somedomain.tld')
+@pytest.mark.usefixtures('load_cfg')
+@pytest.mark.parametrize('domain,revoke,result', [
+    ('domain1.tld', True, True),
+    ('', False, True),
+])
+def test_remove_domain(certificate_revoke, domain, revoke, result):
+    """Test removing a domain that can certificates."""
+    assert result == on_domain_removed('test', 'domain-type-test', domain)
+    if revoke:
+        certificate_revoke.assert_has_calls([call(domain)])
+    else:
+        certificate_revoke.assert_not_called()