Tests: Clean sweep in logging suite#8512
Conversation
jakub-vavra-cz
force-pushed
the
sweep_logging
branch
3 times, most recently
from
136163d to
8a09651
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
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.
jakub-vavra-cz
force-pushed
the
sweep_logging
branch
from
8a09651 to
2f64763
Compare
|
|
||
| The default debug level 2. | ||
| It is handled and reported as a bitmask. | ||
| See: https://github.com/SSSD/sssd/blob/master/src/util/debug.h#L101 |
There was a problem hiding this comment.
Maybe better
https://github.com/SSSD/sssd/blob/77fc6ff1d83f1d8e20f519df7194a95d0abf7491/src/util/debug.h#L101
Otherwise link will become wrong once header is updated in the repo.
| @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): |
There was a problem hiding this comment.
What is the convention: '_' or '__'?
Above you used
test_logging_debug_level_is_written_to_logs
-- single '_'
| :expectedresults: | ||
| 1. Logs messages contain default debug level 0x0070 | ||
| 2. Debug level change command succeeds | ||
| 3. Logs messages contain default debug level 0x2f7f0 |
There was a problem hiding this comment.
Test check sssctl_get.stdout, not 'logs'.
| result = client.sssctl.domain_status(client.sssd.default_domain) | ||
| if result is not None and "Online status: Offline" in result.stdout: | ||
| break | ||
| time.sleep(10) |
There was a problem hiding this comment.
This makes it very likely test will wait 10 secs.
I suggest changing to 1..2 secs (you can increase number of iterations if you believe that under the load it will take longer than (1..2)*9 seconds to starts)
| client.sssd.sssd["services"] = "nss, pam, ssh" | ||
| client.sssd.clear(logs=True, config=False) | ||
| client.sssd.start(debug_level=None) | ||
| time.sleep(30) |
There was a problem hiding this comment.
What is the purpose of this long sleep()?
| log = client.journald.journalctl(grep="Backend is offline", unit="sssd") | ||
| if log.rc == 0: | ||
| break | ||
| time.sleep(10) |
There was a problem hiding this comment.
The same - poll more often.
|
In general looks good to me, a couple of remarks inline. |
3 participants
Revisiting changes from #7891.