Conversation
![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
This pull request adds a new test case for sss_ssh_knownhosts to verify hostname and IP address resolution. My review found a critical bug in the test's assertion logic, an incorrect type hint, and a misleading test ID. These issues could cause the test to behave incorrectly or be difficult to debug, and I have provided suggestions to fix them.
src/tests/system/tests/test_tools.py
Outdated
| assert result is not None, f"sss_ssh_knownhosts did not run properly!" | ||
| assert isinstance(result, list), f"Expected output to be a list of lines, got {type(result)} instead!" | ||
| if host[1] == 0: | ||
| assert any("0x0040" not in line for line in result), f"{host[0]} should have been found but was not!" |
There was a problem hiding this comment.
The assertion logic to check for a successfully found host is incorrect. any("0x0040" not in line for line in result) will pass if any line in the output doesn't contain "0x0040", even if an error line with "0x0040" exists. This can cause the test to pass when it should fail. The logic should be inverted to ensure no line contains the error code.
| assert any("0x0040" not in line for line in result), f"{host[0]} should have been found but was not!" | |
| assert not any("0x0040" in line for line in result), f"{host[0]} should have been found but was not!" |
There was a problem hiding this comment.
The debug level 0x0040 is used by several messages which may indicated an error but not the error you are looking for. Maybe you should look for the "Name or service not known" string instead.
There was a problem hiding this comment.
The messages are in two different outputs, stdout and stderr. We can assert by that, take a look.
The test is passing.
============================= test session starts ==============================
collecting ...
Selected tests will use the following hosts:
client: client.test
kdc: kdc.test
collected 6 items
tests/test_tools.py::test_tools__sss_cache_expired_does_not_print_unrelated_message (client)
tests/test_tools.py::test_tools__sss_ssh_knownhosts_resolves_hostnames_and_ips[client.io] (client)
tests/test_tools.py::test_tools__sss_ssh_knownhosts_resolves_hostnames_and_ips[1.1.1.1] (client)
tests/test_tools.py::test_tools__sss_ssh_knownhosts_resolves_hostnames_and_ips[client.test] (client)
tests/test_tools.py::test_tools__sss_ssh_knownhosts_resolves_hostnames_and_ips[asdf.test] (client)
tests/test_tools.py::test_tools__sss_ssh_knownhosts_resolves_hostnames_and_ips[super.bad.hostname] (client)
============================== 6 passed in 12.81s ==============================
PASSED [ 16%]PASSED [ 33%]PASSED [ 50%]PASSED [ 66%]PASSED [ 83%]PASSED [100%]
Process finished with exit code 0
src/tests/system/tests/test_tools.py
Outdated
| assert result is not None, f"sss_ssh_knownhosts did not run properly!" | ||
| assert isinstance(result, list), f"Expected output to be a list of lines, got {type(result)} instead!" | ||
| if host[1] == 0: | ||
| assert any("0x0040" not in line for line in result), f"{host[0]} should have been found but was not!" |
There was a problem hiding this comment.
The debug level 0x0040 is used by several messages which may indicated an error but not the error you are looking for. Maybe you should look for the "Name or service not known" string instead.
danlavu
force-pushed
the
tests-sss_ssh_knownhosts
branch
2 times, most recently
from
479a3fa to
a84b507
Compare
danlavu
force-pushed
the
tests-sss_ssh_knownhosts
branch
2 times, most recently
from
9f3cce3 to
73e24a5
Compare
danlavu
force-pushed
the
tests-sss_ssh_knownhosts
branch
from
73e24a5 to
212c9fb
Compare
Reviewed-by: Anuj Borah <aborah@redhat.com> Reviewed-by: Alejandro López <allopez@redhat.com>
sssd-bot
force-pushed
the
tests-sss_ssh_knownhosts
branch
from
212c9fb to
3e19e0d
Compare
4 participants
No description provided.