Tests: Clean sweep in logging suite by jakub-vavra-cz · Pull Request #8512 · SSSD/sssd

jakub-vavra-cz · 2026-03-11T14:16:57Z

Revisiting changes from #7891.

alexey-tikhonov · 2026-03-13T10:24:34Z

/gemini review

gemini-code-assist

Code Review

The pull request primarily refactors and expands SSSD logging tests. It clarifies documentation regarding default debug levels, renames several existing tests for better readability, and introduces new tests to verify that configured domain debug levels are honored and that debug levels can be changed at runtime using sssctl. Additionally, it improves existing tests for domain configuration errors and offline scenarios by making assertions more precise and including checks for syslog messages. A review comment highlights an inaccuracy in a test's docstring regarding the expected debug level, which should be updated to reflect the correct cumulative bitmask.

src/tests/system/tests/test_logging.py

alexey-tikhonov · 2026-03-16T10:25:50Z

src/tests/system/tests/test_logging.py

+@pytest.mark.importance("low")
+@pytest.mark.topology(KnownTopologyGroup.AnyProvider)
+@pytest.mark.skip(reason="There are some error messages that need to have log level increased.")
+def test_logging__user_logins_are_not_written_to_logs(client: Client, provider: GenericProvider):


What is the convention: '_' or '__'?
Above you used

test_logging_debug_level_is_written_to_logs

-- single '_'

As I undestand it, before "__" is common prefix denoting test area that can be used to select tests instead of having many (outdated) pytest markes.

Why then test_logging__user_logins_are_not_written_to_logs but test_logging_debug_level_is_written_to_logs?

That is a typo, the naming convention is filename__test_case_name

src/tests/system/tests/test_logging.py

alexey-tikhonov · 2026-03-16T11:14:34Z

In general looks good to me, a couple of remarks inline.

Merged tests for sssd offline message in logs and syslog. Split backend offline(unreachable) and dns resolution error scenario. Check that user login does not generate logs on default debug level extended to all providers. Added link to debug level documentation. Dropped references to (now-irrelevant) bugzillas in rewritten tests. Skipping test_logging__user_logins_are_not_written_to_logs as updating log level of the messages is low priority.

danlavu

A few nitpicks, some comments, but overall, it's great, thank you.

danlavu · 2026-03-24T03:13:50Z

src/tests/system/tests/test_logging.py

 @pytest.mark.importance("low")
 @pytest.mark.topology(KnownTopology.Client)
-def test_logging__default_settings_does_not_log_user_logins(client: Client):
+def test_logging_debug_level_is_written_to_logs(client: Client):


missing _ , test_logging__

danlavu · 2026-03-24T03:15:27Z

src/tests/system/tests/test_logging.py

+@pytest.mark.integration
+@pytest.mark.importance("low")
+@pytest.mark.topology(KnownTopology.Client)
+def test_logging_change_debug_level_during_runtime(client: Client):


missing _ , test_logging__

danlavu · 2026-03-24T03:18:01Z

src/tests/system/tests/test_logging.py

+    client.sssd.start(debug_level=None)
+
+    log_str = client.fs.read(client.sssd.logs.domain())
+    sssctl_set = client.host.conn.run("sssctl debug-level --domain local 9")


I think this can be replaced with client.sssctl.debug_level(level=9, domain="local")

danlavu · 2026-03-24T03:20:04Z

src/tests/system/tests/test_logging.py

+@pytest.mark.importance("low")
+@pytest.mark.topology(KnownTopologyGroup.AnyProvider)
+@pytest.mark.skip(reason="There are some error messages that need to have log level increased.")
+def test_logging__user_logins_are_not_written_to_logs(client: Client, provider: GenericProvider):


That is a typo, the naming convention is filename__test_case_name

danlavu · 2026-03-24T03:20:47Z

src/tests/system/tests/test_logging.py

+        2. Configure SSSD for system authentication
+        3. Clear cache and logs and start SSSD with default debug level
+    :steps:
+        1. Store current logs and authenticate as a user.


nitpick, no period at the end, as long as it's consistent in the file.

danlavu · 2026-03-24T03:20:59Z

src/tests/system/tests/test_logging.py

+        3. Clear cache and logs and start SSSD with default debug level
+    :steps:
+        1. Store current logs and authenticate as a user.
+        2. Compare stored logs with the current ones.


danlavu · 2026-03-24T03:25:47Z

src/tests/system/tests/test_logging.py

+        2. The login event did not generate any new log lines
+    :customerscenario: False
+    """
+    # IPA


I understand that the error message is different. Does the test need to be updated based on the provider? If we leave the test as is, the comments aren't helpful.

danlavu · 2026-03-24T03:26:33Z

src/tests/system/tests/test_logging.py

-@pytest.mark.ticket(bz=1416150)
 @pytest.mark.topology(KnownTopology.LDAP)
-def test_logging__default_settings_logs_to_syslog_when_ldap_is_offline(client: Client):
+def test_logging_offline_errors_are_written_to_logs_and_syslog(client: Client, ldap: LDAP):


missing _ , test_logging__

jakub-vavra-cz added the Tests label Mar 11, 2026

jakub-vavra-cz force-pushed the sweep_logging branch 3 times, most recently from 136163d to 8a09651 Compare March 13, 2026 09:24

jakub-vavra-cz requested a review from alexey-tikhonov March 13, 2026 09:59

jakub-vavra-cz marked this pull request as ready for review March 13, 2026 09:59

jakub-vavra-cz added the Waiting for review label Mar 13, 2026

jakub-vavra-cz assigned alexey-tikhonov Mar 13, 2026

jakub-vavra-cz added backport-to-sssd-2-9 backport-to-sssd-2-10 backport-to-sssd-2-11 backport-to-sssd-2-12 labels Mar 13, 2026

gemini-code-assist bot reviewed Mar 13, 2026

View reviewed changes

src/tests/system/tests/test_logging.py Outdated Show resolved Hide resolved

src/tests/system/tests/test_logging.py Show resolved Hide resolved

jakub-vavra-cz force-pushed the sweep_logging branch from 8a09651 to 2f64763 Compare March 13, 2026 13:13

jakub-vavra-cz requested a review from danlavu March 16, 2026 09:13

jakub-vavra-cz assigned danlavu Mar 16, 2026

alexey-tikhonov reviewed Mar 16, 2026

View reviewed changes

src/tests/system/tests/test_logging.py Outdated Show resolved Hide resolved

alexey-tikhonov reviewed Mar 16, 2026

View reviewed changes

src/tests/system/tests/test_logging.py Outdated Show resolved Hide resolved

alexey-tikhonov reviewed Mar 16, 2026

View reviewed changes

src/tests/system/tests/test_logging.py Outdated Show resolved Hide resolved

alexey-tikhonov reviewed Mar 16, 2026

View reviewed changes

src/tests/system/tests/test_logging.py Show resolved Hide resolved

alexey-tikhonov reviewed Mar 16, 2026

View reviewed changes

src/tests/system/tests/test_logging.py Outdated Show resolved Hide resolved

jakub-vavra-cz force-pushed the sweep_logging branch from 2f64763 to 46a098f Compare March 17, 2026 14:03

jakub-vavra-cz requested a review from alexey-tikhonov March 18, 2026 06:08

alexey-tikhonov approved these changes Mar 20, 2026

View reviewed changes

danlavu reviewed Mar 24, 2026

View reviewed changes

Conversation

jakub-vavra-cz commented Mar 11, 2026

Uh oh!

alexey-tikhonov commented Mar 13, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexey-tikhonov commented Mar 16, 2026

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants