tests: add pysss_nss_idmap system test#8133
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a new system test for pysss_nss_idmap, which is a good addition to ensure the functionality is covered. The tests are comprehensive. My main feedback is to refactor the assertions to be more robust and readable by parsing the command output into Python objects instead of comparing raw strings. This will make the tests easier to maintain in the long run.
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
The ast module is needed to safely evaluate the string representation of Python literals from the command output. This is a more robust way to check test results than string concatenation.
| import pytest | |
| import pytest | |
| import ast |
| output = client.host.conn.run( | ||
| f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyname("{group.name}"))'""" | ||
| ) | ||
| assert output.stdout == "{'" + group.name + "': {'sid': '" + group_sid + "', 'type': 2}}" | ||
|
|
||
| output = client.host.conn.run( | ||
| f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbygroupname("{group.name}"))'""" | ||
| ) | ||
| assert output.stdout == "{'" + group.name + "': {'sid': '" + group_sid + "', 'type': 2}}" | ||
|
|
||
| output = client.host.conn.run( | ||
| f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyusername("{group.name}"))'""" | ||
| ) | ||
| assert output.stdout == "{}" | ||
|
|
||
| output = client.host.conn.run(f"python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyid({group_id}))'") | ||
| assert output.stdout == "{" + str(group_id) + ": {'sid': '" + group_sid + "', 'type': 2}}" | ||
|
|
||
| output = client.host.conn.run( | ||
| f"python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbygid({group_id}))'" | ||
| ) | ||
| assert output.stdout == "{" + str(group_id) + ": {'sid': '" + group_sid + "', 'type': 2}}" | ||
|
|
||
| output = client.host.conn.run( | ||
| f"python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyuid({group_id}))'" | ||
| ) | ||
| assert output.stdout == "{}" | ||
|
|
||
| output = client.host.conn.run( | ||
| f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getidbysid("{group_sid}"))'""" | ||
| ) | ||
| assert output.stdout == "{'" + group_sid + "': {'id': " + str(group_id) + ", 'type': 2}}" | ||
|
|
||
| output = client.host.conn.run( | ||
| f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getnamebysid("{group_sid}"))'""" | ||
| ) | ||
| assert output.stdout == "{'" + group_sid + "': {'name': '" + group.name + "', 'type': 2}}" |
There was a problem hiding this comment.
Similar to the user operations test, using ast.literal_eval() to parse the output and compare with a dictionary will make these assertions more robust and readable.
output = client.host.conn.run(
f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyname(\"{group.name}\"))'"""
)
assert ast.literal_eval(output.stdout) == {group.name: {"sid": group_sid, "type": 2}}
output = client.host.conn.run(
f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbygroupname(\"{group.name}\"))'"""
)
assert ast.literal_eval(output.stdout) == {group.name: {"sid": group_sid, "type": 2}}
output = client.host.conn.run(
f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyusername(\"{group.name}\"))'"""
)
assert ast.literal_eval(output.stdout) == {}
output = client.host.conn.run(f"python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyid({group_id}))'")
assert ast.literal_eval(output.stdout) == {group_id: {"sid": group_sid, "type": 2}}
output = client.host.conn.run(
f"python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbygid({group_id}))'"
)
assert ast.literal_eval(output.stdout) == {group_id: {"sid": group_sid, "type": 2}}
output = client.host.conn.run(
f"python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getsidbyuid({group_id}))'"
)
assert ast.literal_eval(output.stdout) == {}
output = client.host.conn.run(
f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getidbysid(\"{group_sid}\"))'"""
)
assert ast.literal_eval(output.stdout) == {group_sid: {"id": group_id, "type": 2}}
output = client.host.conn.run(
f"""python -c 'import pysss_nss_idmap; print(pysss_nss_idmap.getnamebysid(\"{group_sid}\"))'"""
)
assert ast.literal_eval(output.stdout) == {group_sid: {"name": group.name, "type": 2}}fd84e84 to
e795f7f
Compare
I'm not familiar with it, but everything I'm reading about it makes sense to use it, and I do like the assertions a lot better in the provided examples. I'll defer this to @pbrezina. |
d1a4f9a to
2f11c5d
Compare
danlavu
left a comment
There was a problem hiding this comment.
This is way better. A couple of implementation questions in line.
| 3. no result | ||
| :customerscenario: False | ||
| """ | ||
|
|
There was a problem hiding this comment.
It's mostly about conformity, so if we extended the framework, we could keep user/group creation as part of the test.
There was a problem hiding this comment.
I do not understand this comment?
There was a problem hiding this comment.
I was thinking two different things and jumbled them into one sentence, I'm sorry. This is no longer relevant because we're not updating the framework now, but I was suggesting that.
The bulk the user setup code is getting the user and group sid. If we added user.sid/group.sid. We then can conform to other tests and add some readability. Because it wouldn't be a lot of lines to then retain the explicit user/group setup in the test code.
f5d64b3 to
b854636
Compare
fd2d84c to
6271313
Compare
The new system test is mostly a 1:1 replacement of the exisintg integration test. Reviewed-by: Dan Lavu <dlavu@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Dan Lavu <dlavu@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
6271313 to
ac89ea8
Compare
The new system test is mostly a 1:1 replacement of the exisintg integration test.